-
Notifications
You must be signed in to change notification settings - Fork 4.7k
[NGT] Add workflow to produce Phase2 NANOAOD with L1 and HLT objects #51004
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
Open
elenavernazza
wants to merge
5
commits into
cms-sw:master
Choose a base branch
from
cms-ngt-hlt:Nano_L1_HLT
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0d5b238
Add L1 + HLT NanoAOD workflow
elenavernazza b6de68e
Apply procModifier to enable L1 + HLT NanoAOD
elenavernazza 74ef3bd
Protect track associators from missing collection
elenavernazza 31b674f
Add .774 workflow to ph2_hlt test matrix
elenavernazza 2cbc11f
Clarify comment
elenavernazza File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import FWCore.ParameterSet.Config as cms | ||
|
|
||
| # This modifier is for running NANOAOD production from L1 and HLT steps, | ||
| # whitout running RECO and PAT steps. | ||
| # It excludes all GEN sequences that depend on PAT collections. | ||
|
|
||
| nano_l1_hlt = cms.Modifier() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Thinking about manual configuration, will this step run if the user forgets to specify
@Phase2L1DPGwithGen, despite havingnano_l1_hltactive?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.
why would a user do that? Following wrong recipes results typically in wrong results, no?
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.
Sure; I was just suggesting that adding some protection might help.
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
NANO:@NGTScoutingVal+@Phase2L1DPGwithGenstep should be the only ingredient required to produce the L1+HLT NanoAOD sample.The
nano_l1_hltprocModifier is introduced as a protection against missing PAT collections that are required in some of the L1Nano sequences. The alternative would have been to reorganize the L1Nano configuration itself; however, this would have modified the standard L1Nano format used by existing L1 workflows and users, which I wanted to avoid.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.
@elenavernazza It is clear what
nano_l1_hltachieves. My comment was rather addressing future users that might use this new process modifier without fully knowing the required inputs. I don't know what happens if the user specifies onlyNANO:@NGTScoutingVal, but the potential crash might be cryptic. I was suggesting you could add some checks / protections that throw a meaningful error message if the user forgets to add@Phase2L1DPGwithGen.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.
No, why should it?
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.
Please see my 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.
by this token, should you test all possible workflow combinations in the matrix ?
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 never suggested this.
I tested
NANO:@NGTScoutingValwithnano_l1_hlt, since the bot does not test it, and since I thought that it could lead to a crash, due to the absence of@Phase2L1DPG.There is no crash, so all is well.
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.
It is straightforward to see from the code changes that this should not lead to a runtime crash.
The only modifications affecting
NANO:@NGTScoutingValare inHLTrigger/NGTScouting/python/HLTNanoProducer_cff.pyand are limited to:dstTriggerAcceptFilterfilter for the production of the HLT tablesAs already stated in the PR description, these changes simply allow all events to be written to the output NanoAOD, independently of the L1/HLT decision. No producer dependencies or runtime execution paths are modified in a way that could plausibly introduce a crash.
Because of that, I am a bit puzzled that this specific combination of steps/process modifiers was singled out in the review, while there are already many other untested combinations present in CMSSW that would be equally (or more) likely to expose runtime issues.