Skip to content

[NGT] Update default Phase 2 HLT Muon sequence to use new Standalone seeding module and streamlined Tracker Muon reconstruction#48246

Merged
cmsbuild merged 1 commit intocms-sw:masterfrom
Parsifal-2045:ph2MuonMake777Default
Jun 19, 2025
Merged

[NGT] Update default Phase 2 HLT Muon sequence to use new Standalone seeding module and streamlined Tracker Muon reconstruction#48246
cmsbuild merged 1 commit intocms-sw:masterfrom
Parsifal-2045:ph2MuonMake777Default

Conversation

@Parsifal-2045
Copy link
Copy Markdown
Contributor

PR description:

This PR changes the default Phase 2 HLT Muon reconstruction to the previous .777 workflow. As such it includes:

  • new Standalone Muon seeding module from L1TkMuons
  • streamlined Tracker Muons reconstruction performing the Inside-Out pass first

As part of the integration, the phase2L2AndL3Muons procModifiers has been removed. The other related procModifer phase2L3MuonsOIFirst remains as a quick way to assess the performance of the Outside-In first approach in Tracker Muon reconstruction. The latter procModifier is also enabled by default in .778 workflows (as before)

Slight differences in timing and HLT Muons' physics performance are expected, with slightly higher Standalone performance in the barrel and slightly lower in the endcaps, as seen in the PR that implemented these changes #46897 first.

PR validation:

The PR was tested locally on the standard test suite and a few .778 workflows, such as 29704.778

@cmsbuild
Copy link
Copy Markdown
Contributor

cmsbuild commented Jun 4, 2025

cms-bot internal usage

@Parsifal-2045
Copy link
Copy Markdown
Contributor Author

type ngt

@cmsbuild cmsbuild added the ngt label Jun 4, 2025
@cmsbuild
Copy link
Copy Markdown
Contributor

cmsbuild commented Jun 4, 2025

@cmsbuild
Copy link
Copy Markdown
Contributor

cmsbuild commented Jun 4, 2025

A new Pull Request was created by @Parsifal-2045 for master.

It involves the following packages:

  • Configuration/EventContent (operations)
  • Configuration/ProcessModifiers (operations)
  • Configuration/PyReleaseValidation (upgrade, pdmv)
  • DPGAnalysis/MuonTools (xpog)
  • HLTrigger/Configuration (hlt)
  • Validation/RecoMuon (dqm)

@AdrianoDee, @Martin-Grunewald, @Moanwar, @antoniovagnerini, @antoniovilela, @cmsbuild, @ctarricone, @davidlange6, @DickyChant, @fabiocos, @ftorrresd, @hqucms, @mandrenguyen, @miquork, @mmusich, @rappoccio, @rseidita, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@CeliaFernandez, @Fedespring, @HuguesBrun, @Martin-Grunewald, @SohamBhattacharya, @VourMa, @abbiendi, @andrea21z, @battibass, @calderona, @cericeci, @fabiocos, @jhgoh, @makortel, @missirol, @mmusich, @rociovilar, @rovere, @slomeo, @trocino 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

@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 4, 2025

test parameters:

  • enable = hlt_p2_integration, hlt_p2_timing
  • workflow_opts= -w upgrade
  • workflow = 29634.75, 29634.751, 29634.752, 29634.753, 29634.754, 29634.755, 29634.756, 29634.7561, 29634.7562, 29634.757, 29634.77

@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 4, 2025

@cmsbuild, please test

@cmsbuild
Copy link
Copy Markdown
Contributor

cmsbuild commented Jun 4, 2025

-1

Failed Tests: HLTP2Timing
Size: This PR adds an extra 88KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a54951/46546/summary.html
COMMIT: bb9bc51
CMSSW: CMSSW_15_1_X_2025-06-04-1100/el8_amd64_gcc12
Additional Tests: HLT_P2_INTEGRATION,HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/48246/46546/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 1 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 14 differences found in the comparisons
  • DQMHistoTests: Total files compared: 60
  • DQMHistoTests: Total histograms compared: 4304812
  • DQMHistoTests: Total failures: 4891
  • DQMHistoTests: Total nulls: 77
  • DQMHistoTests: Total successes: 4299824
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 21335.577999999998 KiB( 59 files compared)
  • DQMHistoSizes: changed ( 24834.911,... ): 1255.034 KiB HLT/Muon
  • Checked 245 log files, 204 edm output root files, 60 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 4, 2025

-1
Failed Tests: HLTP2Timing

That fails because the input data here

DATASET="/RelValTTbar_14TeV/CMSSW_14_1_0_pre6-PU_141X_mcRun4_realistic_v1_STD_2026D110_PU-v3/GEN-SIM-DIGI-RAW"

is too old: it misses this fix to the L1T event content that was implemented in CMSSW 15_0_0_pre2 (commit).
Before further testing here we'll need to update the testing code.

@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 5, 2025

Before further testing here we'll need to update the testing code.

I have opened #48253.
In the meanwhile @Parsifal-2045 can you update the PR title to be more descriptive (.777 will mean close to nothing in retrospect when reading the release notes): something like "Make new Standalone muon seeding module from L1TkMuons default in Phase-2 HLT" or similar.

@Parsifal-2045 Parsifal-2045 changed the title [NGT] Make .777 the default Phase 2 HLT Muon reconstruction [NGT] Update default Phase 2 HLT Muon sequence to use new Standalone seeding module and streamlined Tracker Muon reconstruction Jun 5, 2025
@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 6, 2025

@cmsbuild, please test

@cmsbuild
Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48246/45155

@cmsbuild
Copy link
Copy Markdown
Contributor

Pull request #48246 was updated. @Moanwar, @antoniovagnerini, @antoniovilela, @cmsbuild, @ctarricone, @davidlange6, @fabiocos, @mandrenguyen, @rappoccio, @rseidita, @srimanob, @subirsarkar can you please check and sign again.

@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 11, 2025

test parameters:

  • workflow_opts= -w upgrade --nEvents 100
  • workflow = 29642.0

@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 11, 2025

@cmsbuild, please test

@Parsifal-2045
Copy link
Copy Markdown
Contributor Author

The current understanding of the issue is that the FastTimer service is not able to correctly assign the time required to access the L1TkMuons collection (and some related ones, like the corresponding stubs), leading to IO/access time ending up in the first module that consumes the collection.

Overall impact on timing (comparing both configurations with the added consumer)

I wonder then how reliable is the measurement on the overall timing (in which circles report ~100ms overall reduction). IIUC the above comment, the unreliable part is the group assignment (Muon or I/O), or is also the overall size of the pie?

From what I have observed while benchmarking with different configurations (jobs, threads, streams), the overall timing measurements can vary quite a bit depending on the configuration, but are generally stable when fixing a set of parameters (and implementing a workaround specifically for muons), which would lead me to believe the measurements I have provided. Having said this, the changes implemented in this pr deal only with muon reconstruction where the expected improvement is lower than the observed overall, so I'm not exactly sure where the remaining ~60ms are coming from

@cmsbuild
Copy link
Copy Markdown
Contributor

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a54951/46672/summary.html
COMMIT: 12de5c3
CMSSW: CMSSW_15_1_X_2025-06-11-1100/el8_amd64_gcc12
Additional Tests: HLT_P2_INTEGRATION,HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/48246/46672/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a54951/46672/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a54951/46672/git-merge-result

  • DAS Queries: The DAS query tests failed, see the summary page for details.

  • HLT P2 Timing: chart

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 1 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 4178724
  • DQMHistoTests: Total failures: 2381
  • DQMHistoTests: Total nulls: 33
  • DQMHistoTests: Total successes: 4176290
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 19540.276 KiB( 50 files compared)
  • DQMHistoSizes: changed ( 1000.0,... ): 263.889 KiB Tracking/ShortTrackResolution
  • DQMHistoSizes: changed ( 24834.911,... ): 1255.034 KiB HLT/Muon
  • Checked 220 log files, 188 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Copy Markdown
Contributor

mmusich commented Jun 19, 2025

@cms-sw/dqm-l2 @cms-sw/upgrade-l2 please take a look and sign.

@Moanwar
Copy link
Copy Markdown
Contributor

Moanwar commented Jun 19, 2025

+Upgrade

@antoniovagnerini
Copy link
Copy Markdown

DQMHistoSizes: Histogram memory added: 19540.276 KiB( 50 files compared)
The memory consumption fro DQM histograms increases considerably by 20MB, which is above the typical increase more than 10-20% of existing MEs for the subsystem. Is this expected from the authors @Parsifal-2045 from the change alone in the tracker muon reconstruction? In principle, it may not be a showstopper, given that the additions seem to affect only to Phase2 dedicated workflows.

@Parsifal-2045
Copy link
Copy Markdown
Contributor Author

DQMHistoSizes: Histogram memory added: 19540.276 KiB( 50 files compared) The memory consumption fro DQM histograms increases considerably by 20MB, which is above the typical increase more than 10-20% of existing MEs for the subsystem. Is this expected from the authors @Parsifal-2045 from the change alone in the tracker muon reconstruction? In principle, it may not be a showstopper, given that the additions seem to affect only to Phase2 dedicated workflows.

It does indeed affect only Phase2 workflows, since the changes mostly come from the addition of two new collections to monitor in the Phase2 HLT Muon default reconstruction, as written here

def _modify_for_IO_first(validator):
validator.associatormap += ['Phase2tpToL2MuonToReuseAssociation', 'Phase2tpToL3IOTkFilteredAssociation']
validator.label += ['hltPhase2L3MuonFilter:L2MuToReuse', 'hltPhase2L3MuonFilter:L3IOTracksFiltered']
validator.muonHistoParameters.extend([staMuonHistoParameters, trkMuonHistoParameters])

Some other spurious differences outside Phase 2 HLT Muons should not be caused by this PR

@antoniovagnerini
Copy link
Copy Markdown

+dqm

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

@mandrenguyen
Copy link
Copy Markdown
Contributor

+1

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.

8 participants