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

Fix HLT customization for alpaka PF #43999

Conversation

waredjeb
Copy link
Contributor

@waredjeb waredjeb commented Feb 16, 2024

This PR fixes the customization of Alpaka-PF introduced with #43971 with moving the CPU serial module to DQM_HcalReconstruction_v6

GPU vs CPU comparison at DQMOnline will followup, currently we are only saving the needed collections

Validated running:

https_proxy=http://cmsproxy.cms:3128/ hltGetConfiguration /dev/CMSSW_14_0_0/GRun --unprescale --output all --globaltag auto:phase1_2024_realistic --mc --max-events 10 --input /store/mc/Run3Winter24Digi/TT_TuneCP5_13p6TeV_powheg-pythia8/GEN-SIM-RAW/133X_mcRun3_2024_realistic_v8-v2/80000/dc984f7f-2e54-48c4-8950-5daa848b6db9.root --customise HLTrigger/Configuration/customizeHLTforAlpaka.customizeHLTforAlpaka > hlt.py 

Backport:
#44000

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 16, 2024

cms-bot internal usage

@fwyzard
Copy link
Contributor

fwyzard commented Feb 16, 2024

type bugfix

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43999/38919

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @Martin-Grunewald, @mmusich can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@fwyzard
Copy link
Contributor

fwyzard commented Feb 16, 2024

please test

@@ -249,6 +243,21 @@ def replaceItemsInSequence(process, itemsToReplace, replacingSequence):
itemsList = [process.hltParticleFlowRecHitHBHE, process.hltParticleFlowClusterHBHE,process.hltParticleFlowClusterHCAL]
process = replaceItemsInSequence(process, itemsList, process.HLTPFHcalClustering)

process.PFClusterHBHECPUSerial = cms.Sequence(process.hltHBHERecHitToSoA+process.hltPFRecHitSoAProducerHCALCPUSerial+process.hltPFClusterSoAProducerCPUSerial)
Copy link
Contributor

Choose a reason for hiding this comment

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

HLT sequences start with uppercase HLT so rename to HLTPFClusterHBHECPUSerial and propagate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this sequence used anyway?

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 Sequence is then added to DQM_HcalReconstruction_v6

@fwyzard
Copy link
Contributor

fwyzard commented Feb 16, 2024

Looks good to me.

I suggested to @waredjeb to reuse the HCAL DQM path instead of creating a new one to keep the customisation simpler.

dqmHcalRecoPathName = "DQM_HcalReconstruction_v6"
dqmHcalPath= getattr(process, dqmHcalRecoPathName)
dqmHcalRecoPathIndex = dqmHcalPath.index(process.hltHcalConsumerGPU) + 1
dqmHcalPath.insert(dqmHcalRecoPathIndex , process.PFClusterHBHECPUSerial)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, here - please update.

@fwyzard
Copy link
Contributor

fwyzard commented Feb 16, 2024

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43999/38920

@cmsbuild
Copy link
Contributor

Pull request #43999 was updated. @Martin-Grunewald, @mmusich can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-703286/37532/summary.html
COMMIT: 0a56692
CMSSW: CMSSW_14_1_X_2024-02-15-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43999/37532/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@Martin-Grunewald
Copy link
Contributor

+1

@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. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@waredjeb
Copy link
Contributor Author

preparing a fix addressing comment

@waredjeb
Copy link
Contributor Author

Ok since the fix would touch also the RECO I might open a different pull request and this one can get merged if you all agree.

@Martin-Grunewald
Copy link
Contributor

OK with me! (again, for 14_1 and 14_0).

@antoniovilela
Copy link
Contributor

+1


# Add CPUSerial sequences to DQM_HcalReconstruction_v6 Path
dqmHcalRecoPathName = "DQM_HcalReconstruction_v6"
dqmHcalPath= getattr(process, dqmHcalRecoPathName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this getattr() call is causing workflow failures in GPU IB, see #44046

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess something like this

dqmPixelRecoPathName = None
for pathName in process.paths_():
if pathName.startswith('DQM_PixelReconstruction_v'):
dqmPixelRecoPathName = pathName
break
if dqmPixelRecoPathName == None:
return process

is needed.
@waredjeb FYI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for providing the fix with #44048
should I update #44000 with the same fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

should I update #44000 with the same fix?

yes, please

@mmusich
Copy link
Contributor

mmusich commented Feb 21, 2024

type pf

@cmsbuild cmsbuild added the pf label Feb 21, 2024
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.

7 participants