Skip to content

Implement a GenericCloner test module #47504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Mar 4, 2025

PR description:

Specialise Handle and OrphanHandle for WrapperBase: this lets users produce and consume collections via their wrapper, using their run time type information (e.g. type name, type id, ...) instead of the compile time types. Implement integration test for this specialisations.

Introduce a MemcpyTraits structure, that should be specialised for types that can be safely memcpyed, without requiring a full ROOT streamer de/serialisation.
Specialise the MemcpyTraits for arithmetic types and vectors of arithmetic types, and all PortableObject and PortableCollection types.

Implement edmtest::GenericCloner: this EDProducer will consume and clone all the event products declared by its configuration; a direct memcpy will be used for types that support the MemcpyTraits interface; the ROOT dictionaries will be use to serialise and deserialise the other tyepes.

PR validation:

Unit tests pass.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

May be backported to 15.0.x as part of the MPI work.

fwyzard added 3 commits March 4, 2025 13:49
This EDProducer will clone all the event products declared by its
configuration, using their ROOT dictionaries.
@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2025

cms-bot internal usage

@fwyzard
Copy link
Contributor Author

fwyzard commented Mar 4, 2025

@makortel what do you think of this approach ?

I don't mind moving stuff around, of course: consider this more of a prototype.

@fwyzard
Copy link
Contributor Author

fwyzard commented Mar 4, 2025

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2025

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47504/43956

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2025

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47504/43957

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 4, 2025

A new Pull Request was created by @fwyzard for master.

It involves the following packages:

  • DataFormats/Common (core)
  • DataFormats/Portable (heterogeneous)
  • FWCore/Framework (core)
  • FWCore/Integration (core)
  • FWCore/TestModules (core)

@Dr15Jones, @cmsbuild, @fwyzard, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@makortel, @missirol, @mmusich, @rovere, @wddgit this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

Copy link
Contributor Author

@fwyzard fwyzard Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this could actually go directly under DataFormats/Common, only the specialisation of the Event methods need to go in FWCore/Framework.
Let me know if I should split and rearrange this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this could actually go directly under DataFormats/Common, only the specialisation of the Event methods need to go in FWCore/Framework.
Let me know if I should split and rearrange this.

return {object->metadata().size()};
}

static std::vector<std::pair<void*, std::size_t>> regions(PortableDeviceCollection<T, TDev>& object) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the specialisation for PortableDeviceCollection work, some kind of pass-through specialisation for edm::DeviceProduct is still needed.

@cmsbuild
Copy link
Contributor

Pull request #47504 was updated. @Dr15Jones, @cmsbuild, @fwyzard, @makortel, @smuzaffar can you please check and sign again.

@fwyzard fwyzard force-pushed the implement_generic_product_tools_v2 branch from 4b85303 to d32b15b Compare March 11, 2025 10:47
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47504/44032

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@fwyzard fwyzard force-pushed the implement_generic_product_tools_v2 branch from d32b15b to 4988fc2 Compare March 11, 2025 11:06
@fwyzard
Copy link
Contributor Author

fwyzard commented Mar 11, 2025

please test

@fwyzard
Copy link
Contributor Author

fwyzard commented Mar 11, 2025

@makortel @Dr15Jones this is now close enough to what I think I need, that I'd be happy for a first review.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47504/44033

@cmsbuild
Copy link
Contributor

Pull request #47504 was updated. @Dr15Jones, @fwyzard, @makortel, @smuzaffar can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 44KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7fe69b/44911/summary.html
COMMIT: 4988fc2
CMSSW: CMSSW_15_1_X_2025-03-10-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47504/44911/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 4 lines from the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3920300
  • DQMHistoTests: Total failures: 57
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3920223
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 214 log files, 184 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants