Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Configuration/HLT/python/autoHLT.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
'relval2025' : '2025v12',
'relvalRun4' : '75e33',
'relvalRun4_timing' : '75e33_timing',
'relvalRun4_trk' : '75e33_trackingOnly',
'relvalRun4_scouting' : 'NGTScouting',
'test' : 'GRun',
}
1 change: 1 addition & 0 deletions Configuration/PyReleaseValidation/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ The offsets currently in use are:
* 0.703: LST tracking (Phase-2 only), initialStep+HighPtTripletStep only, on CPU
* 0.704: LST tracking (Phase-2 only), initialStep+HighPtTripletStep only, on GPU (if available)
* 0.75: HLT phase-2 timing menu
* 0.7501: HLT phase-2 tracking-only menu
* 0.751: HLT phase-2 timing menu Alpaka variant
* 0.752: HLT phase-2 timing menu ticl_v5 variant
* 0.753: HLT phase-2 timing menu Alpaka, single tracking iteration variant
Expand Down
1 change: 1 addition & 0 deletions Configuration/PyReleaseValidation/python/relval_Run4.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
numWFIB.extend([24834.911]) #D98 XML, to monitor instability of DD4hep

# Phase-2 HLT tests
numWFIB.extend([prefixDet+34.7501])# HLTTrackingOnly75e33
numWFIB.extend([prefixDet+34.751]) # HLTTiming75e33, alpaka
numWFIB.extend([prefixDet+34.752]) # HLTTiming75e33, ticl_v5
numWFIB.extend([prefixDet+34.753]) # HLTTiming75e33, alpaka,singleIterPatatrack
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1890,6 +1890,15 @@ def condition(self, fragment, stepList, key, hasHarvest):
'-s':'HARVESTING:@hltValidation'
}

upgradeWFs['HLTTrackingOnly75e33'] = deepcopy(upgradeWFs['HLTTiming75e33'])
upgradeWFs['HLTTrackingOnly75e33'].suffix = '_HLT75e33TrackingOnly'
upgradeWFs['HLTTrackingOnly75e33'].offset = 0.7501
upgradeWFs['HLTTrackingOnly75e33'].step2 = {
'-s':'DIGI:pdigi_valid,DIGI2RAW,L1TrackTrigger,L1,L1P2GT,HLT:75e33_trackingOnly,VALIDATION::hltMultiTrackValidation+hltMultiPVValidation',
'--datatier':'GEN-SIM-DIGI-RAW,DQMIO',
'--eventcontent':'FEVTDEBUGHLT,DQMIO'
}

upgradeWFs['HLTTiming75e33TiclV5'] = deepcopy(upgradeWFs['HLTTiming75e33'])
upgradeWFs['HLTTiming75e33TiclV5'].suffix = '_HLT75e33TimingTiclV5'
upgradeWFs['HLTTiming75e33TiclV5'].offset = 0.752
Expand Down
1 change: 1 addition & 0 deletions Configuration/PyReleaseValidation/scripts/runTheMatrix.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ def runSelected(opt):
'muonmc' : [5.1, 124.4, 124.5, 20, 21, 22, 23, 25, 30], #MC

'ph2_hlt' : [29634.75, # HLT phase-2 timing menu
29634.7501, # HLT phase-2 tracking-only menu
29634.751, # HLT phase-2 timing menu Alpaka variant
29634.752, # HLT phase-2 timing menu ticl_v5 variant
29634.753, # HLT phase-2 timing menu Alpaka, single tracking iteration variant
Expand Down
9 changes: 9 additions & 0 deletions HLTrigger/Configuration/python/HLT_75e33/paths/MC_TRK_cfi.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import FWCore.ParameterSet.Config as cms

from ..sequences.HLTBeginSequence_cfi import *
from ..sequences.HLTTrackingSequence_cfi import *

MC_TRK = cms.Path(
HLTBeginSequence
+ HLTTrackingSequence
)
2 changes: 2 additions & 0 deletions HLTrigger/Configuration/python/HLT_75e33_cff.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@
fragment.load("HLTrigger/Configuration/HLT_75e33/paths/MC_Ele5_Open_L1Seeded_cfi")
fragment.load("HLTrigger/Configuration/HLT_75e33/paths/MC_Ele5_Open_Unseeded_cfi")
fragment.load("HLTrigger/Configuration/HLT_75e33/paths/MC_JME_cfi")
fragment.load("HLTrigger/Configuration/HLT_75e33/paths/MC_TRK_cfi")
fragment.load("HLTrigger/Configuration/HLT_75e33/psets/CkfBaseTrajectoryFilter_block_cfi")
fragment.load("HLTrigger/Configuration/HLT_75e33/psets/ckfBaseTrajectoryFilterP5_cfi")
fragment.load("HLTrigger/Configuration/HLT_75e33/psets/CkfTrajectoryBuilder_cfi")
Expand Down Expand Up @@ -365,6 +366,7 @@

fragment.MC_JME,
fragment.MC_BTV,
fragment.MC_TRK,
Copy link
Contributor

Choose a reason for hiding this comment

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

Both MC_JME and MC_BTV already include HLTTrackingSequence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to follow the pattern that exists in Run3 (and probably was in Run2), to have a tracking-only path, similar to MC_ReducedIterativeTracking_v*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rovere
Is there a specific proposal for your comment?
IIUC, the HLT path design is that paths should be written as if they can be running independently. So, if I were to select or time MC_TRK, I can not assume specifics of the implementation of some other path.

Your comment appears to imply that a path that at the moment is logically a subset of an existing path, such path is not allowed. Then, logically, it sounds like paths that include the most of modules are preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ciao @slava77 my only "doubt" is related to having different MC paths in the "central" Menu that, in the end, will do/trigger "full" reco (or at least full tracking) on all events, no matter what.
Having one additional MC path that does only tracking, like the one you are proposing, in the regular 75e33 Menu will be, in this case, cost free, but I don't see how this will be useful/be used on a real life example?

And, in fact, you have added a dedicated "menu" to run that path only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the full menu, I was following the same path as in HLT_GRun_cff or HLT_FULL_cff, which has a similar pattern of MC_ paths. These MC paths are often useful to estimate the performance of the specific parts of the reconstruction.

If your comment is arguing against a definition of a separate "menu", I would take an alternative solution using hltGetConfiguration --paths ..., but I'm not sure it exists.

Still, to be clear, what is your proposal?
Am I reading correctly that you are against having an HLT workflow that runs only tracking?

Copy link
Contributor

Choose a reason for hiding this comment

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

aren't the timing tools allowing to measure the time of a path (non-exclusive) from the full menu (HLT_75e33_cff as in this case)? If so, I still think it's useful to keep this in the main menu.

I am not aware of that. Do you have more details about it?

Do I understand correctly that in HLT_Full you'd argue for removal of MC_ReducedIterativeTracking_v* if there was no hltGetConfiguration --paths available?

That would definitely make it less useful. The other reason to keep in the main menu is to exercise it via MC relvals over all the events without filters. The run-3 menu is more complicated and I am not sure any other path would run exactly that in the shadow.
In any case to be honest Tracking POG hasn't made any real maintenance of the MC_ReducedIterativeTracking_v* in the past several years (all relevant changes were directly / indirectly implemented by TSG) so at the end of the day it is a maintenance burden.

Can you ensure now that the content of MC_JME or MC_BTV would not deviate in the future from MC_TRK?

it doesn't need to be and not we won't ensure it.
Also to be clear I am not advocating that MC_TRK is useless per se, it's just that at the moment is less useful that it can be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the proposal is remove MC_TRK from the HLT_75e33_cff.py menu until the above becomes available, because it's useless (you cannot isolate the path unless you define a whole new menu as a workaround, and the sequence is already in the shadow of other paths already running).

(the discussion apparently goes back to the above, as it sounds like with some vague arguments the MC_TRK is not acceptable in HLT_75e33_cff.py)
Just to be sure do I remove both the addition of the path to the schedule and load of the path from this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

@slava77 it is not that is not acceptable. At the current state of development it is simply useless. You cannot directly use it, you cannot directly time it.
I'm not in a position to guarantee that the other MC paths will stay unchanged and will keep full tracking as is.
But with the other changes proposed in this PR, that will be irrelevant.
So, to be clear: my proposal it not to include the MC_TRK path directly into this file.

When we will be in a position to isolate paths, we can revisit 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.

aren't the timing tools allowing to measure the time of a path (non-exclusive) from the full menu (HLT_75e33_cff as in this case)? If so, I still think it's useful to keep this in the main menu.

I am not aware of that. Do you have more details about it?

I had in mind the regular TimerService path time_real; I usually see these in DQM files of the timing reports. (e.g. some random run3 timing test)

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we will be in a position to isolate paths, we can revisit this.

for the timing purposes, I can see with the FastTimer DQM output timing by path enabled by

process.FastTimerService.enableDQM = True
process.FastTimerService.enableDQMbyPath = True
process.FastTimerService.enableDQMbyModule = True
process.FastTimerService.dqmModuleTimeResolution = 10
process.FastTimerService.dqmPathTimeResolution = 50
image Incrementally this seems to make sense (BTV and JME overlap), that the total times are meaningful. Or am I misleading myself?

fragment.MC_Ele5_Open_Unseeded,
fragment.MC_Ele5_Open_L1Seeded,

Expand Down
16 changes: 16 additions & 0 deletions HLTrigger/Configuration/python/HLT_75e33_trackingOnly_cff.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import FWCore.ParameterSet.Config as cms

from .HLT_75e33_cff import fragment
Copy link
Contributor

Choose a reason for hiding this comment

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

while this is very convenient for you I guess, it creates a lot of unnecessary obfuscation in the dump of the menu (unneeded sequences). Also it tricks the integration test script to run the whole list of paths in the test of this menu, see section 75e33_trackingOnly in the log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would be a relatively maintainable solution? I thought that a completely independently defined table is not practical for maintenance and it's more appropriate to have a way to pick a path from an existing menu.

Related to the log, it seems like an issue with the tool: only empty L1 paths will run in addition to MC_TRK, which is a minor overhead.

Should I loop over paths and del them here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the log, it seems like an issue with the tool: only empty L1 paths will run in addition to MC_TRK, which is a minor overhead.

this is not what the tool shows. Or do you imply that's what the tools should show?

Should I loop over paths and del them here?

perhaps it's a better solution.
Can you clarify what's the main use case of having an entirely new table (other than injecting in a cmsDriver command)?
As you point out this brings in additional maintenance burden (on TSG)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not what the tool shows. Or do you imply that's what the tools should show?

the tool (hltPhase2UpgradeIntegrationTests, guessed from a git grep) apparently simply edmConfigDumps the config and looks for unsorted_hlt_paths = re.findall(r"process\.(HLT_[A-Za-z0-9_]+|L1T_[A-Za-z0-9_]+|DST_[A-Za-z0-9_]+)", config_content)
(aside: The MC_ paths are not picked up)
This is checked regardless of the schedule (here only the MC_TRK is put on the schedule). This should explain why the paths show up in the output of the tool

Copy link
Contributor

Choose a reason for hiding this comment

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

This should explain why the paths show up in the output of the tool

I know what the tool does since I wrote. It expects a a configuration not clogged with paths that are not run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would you consider using edmConfigDump --prune as a more straightforward solution for the hltPhase2UpgradeIntegrationTests ?
This cleans up unscheduled and unused modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

This cleans up unscheduled and unused modules.

sure. But it does not solve the clutter of unneeded configuration caused by the addition of this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the record, your suggestion at https://github.com/cms-sw/cmssw/pull/48706/files#r2267520688 was implemented at #48721



for p in dir(fragment):
att = getattr(fragment, p)
if isinstance(att, cms.Path) and p not in ["MC_TRK", "HLTriggerFinalPath", "HLTAnalyzerEndpath"]:
delattr(fragment, p)
del att

fragment.schedule = cms.Schedule(*[
fragment.MC_TRK,
fragment.HLTriggerFinalPath,
fragment.HLTAnalyzerEndpath,
])