Skip to content
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

Move pixelTracksCPU module definition inside SwitchProducerCUDA #37562

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Apr 13, 2022

PR description:

While developing #36938 further (what is now in #37563), I came across this one case where a case-EDProducer of a SwitchProducer was duplicated between the python module (PixelTracks_cff) and SwitchProducerCUDA (pixelTracksSoA) instead of cloning the module. With my developments the duplication lead to failures in python. This PR clones the module when adding it in SwitchProducerCUDA. This PR moves the definition of what was pixelTracksCPU module inside the definition of pixelTracksSoA SwitchProducerCUDA.

PR validation:

The PixelTracks_cff file passes python with my developments.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37562/29293

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

  • RecoPixelVertexing/PixelTrackFitting (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mmusich, @mtosi, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@makortel
Copy link
Contributor Author

makortel commented Apr 13, 2022

@cmsbuild, please abort

On further look I think this is not the correct fix. (edit: corrected in a later force-push)

@makortel makortel changed the title Clone pixelTracksCPU for SwitchProducerCUDA Move pixelTracksCPU module definition inside SwitchProducerCUDA Apr 13, 2022
@makortel makortel force-pushed the fixSwitchProducerPixelTrackscff branch from be9cd78 to d942c99 Compare April 13, 2022 23:45
@@ -132,7 +129,7 @@

# "Patatrack" sequence running on GPU (or CPU if not available)
from Configuration.ProcessModifiers.gpu_cff import gpu
(pixelNtupletFit & gpu).toModify(pixelTracksCPU,
(pixelNtupletFit & gpu).toModify(pixelTracksSoA.cpu,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pixelTracksCPU appeared to be used only in this file, and this customization and the run3_common.toModify(pixelTracksSoA.cpu, on line 109/106 appeared to assume the pixelTracksCPU and pixelTracksSoA.cpu point to the same module object.

@makortel
Copy link
Contributor Author

assign heterogeneous

@makortel
Copy link
Contributor Author

enable gpu

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37562/29294

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

Pull request #37562 was updated. @makortel, @slava77, @clacaputo, @cmsbuild, @fwyzard, @jpata can you please check and sign again.

@jpata
Copy link
Contributor

jpata commented Apr 14, 2022

assign hlt

@cmsbuild
Copy link
Contributor

New categories assigned: hlt

@missirol,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

@missirol
Copy link
Contributor

+hlt

  • the tests passed, and the fix seems uncontroversial
  • not sure what I can add from HLT (isn't this part of an offline reco wf with 'heterogenous parts'? a priori unrelated to HLT?)

@fwyzard
Copy link
Contributor

fwyzard commented Apr 14, 2022

assign hlt

not sure why, as this is purely an offline reconstruction configuration ?

@jpata
Copy link
Contributor

jpata commented Apr 14, 2022

maybe I was confused, but I had the impression this runs at HLT / affects HLT. apologies if I was mistaken!

@jpata
Copy link
Contributor

jpata commented Apr 14, 2022

+reconstruction

  • technical config reorganization, no changes

@fwyzard
Copy link
Contributor

fwyzard commented Apr 14, 2022

Ah... @jpata no, the HLT configuration is fully contained in the HLTrigger/Configuration/python files.
This should only be used by the xxx.502/xxx.506 offline workflows and their variations.

@fwyzard
Copy link
Contributor

fwyzard commented Apr 14, 2022

@makortel what did the previous version do ?
Did process.pixelTracksCPU and process.pixelTracksSoA@cpu identify the same module at the framework level, or two different modules ?

@makortel
Copy link
Contributor Author

what did the previous version do ?
Did process.pixelTracksCPU and process.pixelTracksSoA@cpu identify the same module at the framework level, or two different modules ?

I think that they would lead to two modules being instantiated in the C++ side (except as things are process.pixelTracksCPU would not be instantiated in practice as long as it is not part of any Task/ConditionalTask/Sequence/Path/EndPath/Schedule).

In the python side, it would be some mixture of one and two modules

  • any parameter change in either would be visible in other (as they are the same python object)
  • process.pixelTracksSoA.cpu.label_() would return pixelTracksCPU
  • in config dump they would be visible as two separate modules (without the connection of being the same object)

@fwyzard
Copy link
Contributor

fwyzard commented Apr 14, 2022

Thanks for the summary and explanation !

@fwyzard
Copy link
Contributor

fwyzard commented Apr 14, 2022

+heterogeneous

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Apr 15, 2022

+1

@cmsbuild cmsbuild merged commit 8105500 into cms-sw:master Apr 15, 2022
@makortel makortel deleted the fixSwitchProducerPixelTrackscff branch April 15, 2022 00:50
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.

6 participants