-
Notifications
You must be signed in to change notification settings - Fork 2
CPU vs. GPU for LST in HLT and updates to the offline #215
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
base: master
Are you sure you want to change the base?
Conversation
slava77
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see alpakaValidationLST already added to the alpakaValidation chain.
Or is it logically not practical? (I didn't think all variants well enough).
It sounds like it can be done with no (?) cost. If it's chained by default:
- If the
trackingLSTis not in the producer side of the configuration, a clone of tracks would be compared. This is OK, but wasteful. - Alternatively
alpakaValidationLST & trackingLSTcould be used on the producer sequence and product addition side. This way if thetrackingLSTwas not specified we'd be leaving/allowing the validation module silently fail with empty/missing inputs. This will be lighter on the resources.
| @@ -12,3 +12,7 @@ | |||
| from Configuration.ProcessModifiers.seedingLST_cff import seedingLST | |||
| from Configuration.ProcessModifiers.hltTrackingMkFitInitialStep_cff import hltTrackingMkFitInitialStep | |||
| (trackingLST & seedingLST & hltTrackingMkFitInitialStep).toModify(hltInitialStepMkFitSeeds, seeds = "hltInitialStepTrajectorySeedsLST") | |||
|
|
|||
| hltInitialStepMkFitSeedsSerialSync = hltInitialStepMkFitSeeds.clone( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the idea was to not cover the seedingLST part: just use the candidate directly from LST and only pass them through a fit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to have both: the trackingLST only would be the more "direct" comparison, while there would also be the option to compare the soon-to-be default tracking sequence, to test what actually will be running at HLT. The latter includes seedingLST, hence the above modification.
The value I see in these different sequences, and also having them separately from offline, is to probe changes at the configuration level which can matter, like the propagation of triplet pixel seeds, their duplicate cleaning or not within LST, etc..
HLTrigger/Configuration/python/HLT_75e33/modules/hltInitialStepTrackCandidates_cfi.py
Outdated
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltInitialStepTracks_cfi.py
Outdated
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/modules/hltInitialStepTrajectorySeedsLST_cfi.py
Outdated
Show resolved
Hide resolved
HLTrigger/Configuration/python/HLT_75e33/sequences/HLTInitialStepSequence_cfi.py
Outdated
Show resolved
Hide resolved
Indeed, when I get everything fully working, I want to add it there. I will do it already in the next commit, so that do not forget.
I wrote the tracking sequence modifications in such a way so that the
Based on the above, I think that there is no duplication of tracks but rather a (hopefully) silent failure of the comparison due to missing inputs in all cases of improper procModifier combination. Even that can be avoided by adding
|
|
@slava77 please take a look and let me know of any comments you may have. I will fix them together with the resolution of the conflict and some commit squashing. |
slava77
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving, but leaving a few points to either avoid copy-paste or extremely long (no-space) names/strings
| class UpgradeWorkflow_lstOnGPUIters01TrackingOnlyAlpakaValidationLST(UpgradeWorkflowTracking): | ||
| def setup__(self, step, stepName, stepDict, k, properties): | ||
| if 'Reco' in step: stepDict[stepName][k] = merge([self.step3, stepDict[step][k]]) | ||
| elif 'HARVEST' in step: stepDict[stepName][k] = merge([{'-s': 'HARVESTING:@trackingOnlyValidation+@trackingOnlyDQM', '--procModifiers': 'alpakaValidationLST,trackingIters01,trackingLST'}, stepDict[step][k]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always puzzled by why we need to repeat so much compared to an existing workflow.
Can this inherit from UpgradeWorkflow_lstOnGPUIters01TrackingOnly and just merge '--procModifiers': 'alpakaValidationLST' (and update only the suffix and offset)?
| '-s':'HARVESTING:@hltValidation' | ||
| } | ||
|
|
||
| upgradeWFs['HLTTiming75e33SingleIterLSTAlpakaValidationLST'] = deepcopy(upgradeWFs['HLTTiming75e33']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| upgradeWFs['HLTTiming75e33SingleIterLSTAlpakaValidationLST'] = deepcopy(upgradeWFs['HLTTiming75e33']) | |
| upgradeWFs['HLTTiming75e33SingleIterLSTAlpakaValidationLST'] = deepcopy(upgradeWFs['HLTTiming75e33AlpakaSingleIterLST']) |
similar to the other comment about copy-paste: can this allow to reduce the necessary details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't like to change just the structure of HLT ones for the time being. But it is a good point, and I will keep it in mind for the clean up when we get the new tracking baseline in.
| upgradeWFs['HLTTiming75e33SingleIterCAExtLSTSeedingMkFitBuildingAlpakaValidationLST'].step2 = { | ||
| # This workflow is meant to and only works for the tracking validation | ||
| '-s':'DIGI:pdigi_valid,L1TrackTrigger,L1,L1P2GT,DIGI2RAW,HLT:75e33_timing,VALIDATION:hltMultiTrackValidation', | ||
| '--procModifiers': 'alpakaValidationLST,singleIterPatatrack,phase2CAExtension,trackingLST,seedingLST,trackingMkFitCommon,hltTrackingMkFitInitialStep', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seeing how long the modifier list is, I'd propose to define a modifier chain named e.g. hltSingleIterTrackingBaseline and have something more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The (my) hope is to move the new tracking baseline to default not too far in the future, so I wouldn't like to add a ModifierChain (and hence a new file) for that if it is to be removed in a couple of weeks.
| lstInput = "hltInputLSTSerialSync", | ||
| lstPixelSeeds = "hltInputLSTSerialSync" | ||
| ) | ||
| (singleIterPatatrack & seedingLST & trackingLST & hltTrackingMkFitInitialStep).toModify(hltInitialStepTrackCandidatesSerialSync, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
singleIterPatatrack & seedingLST & trackingLST & hltTrackingMkFitInitialStep is repeated twice and by itself is pretty large, consider to define a shorthand _pataLSTMKF = singleIterPatatrack & seedingLST & trackingLST & hltTrackingMkFitInitialStep and use it in both places
| @@ -106,6 +115,18 @@ | |||
|
|
|||
| (singleIterPatatrack & trackingLST & seedingLST & hltTrackingMkFitInitialStep).toReplaceWith(HLTInitialStepSequence, _HLTInitialStepSequenceSingleIterPatatrackLSTSeedingMkFitTracking) | |||
|
|
|||
| (alpakaValidationLST & singleIterPatatrack & trackingLST & seedingLST & hltTrackingMkFitInitialStep).toReplaceWith(HLTInitialStepSequence, cms.Sequence( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
singleIterPatatrack & trackingLST & seedingLST & hltTrackingMkFitInitialStep is used 3 times in the file, consider a shorthand (by the time it wraps over one line I went over a threshold to make a comment )
d08a0a7 to
b4abc99
Compare
|
@slava77 I should have implemented most of your comments and replied to the rest. Let me know if all looks good now, and I can proceed to the cmssw PR. |
slava77
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to go, considering followup comments
b4abc99 to
6fd4c49
Compare
2cf3f6b to
0dd7b24
Compare
…to the offline corresponding workflow
0dd7b24 to
b807f98
Compare
The goal of this PR is to introduce two HLT workflows to monitor the agreement between LST on CPU and LST on GPU:
alpakaValidationLST,singleIterPatatrack,trackingLST.singleIterPatatrack,phase2CAExtension,trackingLST,seedingLST,trackingMkFitCommon,hltTrackingMkFitInitialStep.The additional CPU reconstruction (
SerialSync) and comparison plots are implemented with a new procModifier,alpakaValidationLST. This procModifier needs to be run only in the procModifier combinations mentioned above to take effect, otherwise it produces neither the additional products nor the comparison plots. It is also included in thealpakaValidationmodifier chain.The analyzer that produces the comparison plots has been improved with a new parameter option to skip luminosity and PU plots, as these are not always available in the current workflows for Phase 2 HLT.
With the introduction of the
alpakaValidationLSTmodifier, the offline workflow testing LST on CPU vs. LST on GPU can be made explicit. The code is changed so that workflow 0.704 runs the offline reconstruction without any additional CPU reconstruction, while a new workflow, 0.7041, runs the comparison.Some screenshots of the content of the DQM file:


