-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make repack process be able to generate new data tiers: L1SCOUT, HLTSCOUT #44381
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44381/39439
|
A new Pull Request was created by @haozturk for master. It involves the following packages:
@davidlange6, @rappoccio, @cmsbuild, @fabiocos, @antoniovilela can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Could someone please review it. It's urgently needed for Tier0 to start testing for Run2024 data taking. |
compressionAlgorithm=cms.untracked.string("LZMA"), | ||
compressionLevel=cms.untracked.int32(4) | ||
) | ||
HLTSCOUTEventContent.outputCommands.extend(HLTriggerRAW.outputCommands) |
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.
we've been discussing internally within TSG that this could be just HLTScouting
instead of HLTriggerRAW
or even better we could add a new HLTriggerHLTSCOUT
to HLTrigger_EventContent_cff.py
replacing HLTScouting
(same keep
instructions but adding a 'drop *_hlt*_*_*'
at the beginning). This would need to be done internally by TSG and can go as a refinement after this PR is integrated.
@cmsbuild, please test |
urgent
|
@mmusich, we were thinking along this line as well, but since it doesn't have a drop statement, we had to proceed with this proposal. Let's start with something that can be used right away and refine later. |
agreed. |
tagging @cms-sw/l1-l2 in case there are comments on the |
I'm going to redirect that question to @dinyar, our scouting expert. |
Otherwise, it looks fine to me. |
Hi @aloeliger, thanks for the tag! I'll pass it right on to @Mmiglio who is working on the CMSSW-based parts of the L1T scouting system. |
Hi, thanks for the tag. It looks good from the L1TScouting perspective as well, but I think that the event content needs to be changed. |
compressionAlgorithm=cms.untracked.string("LZMA"), | ||
compressionLevel=cms.untracked.int32(4) | ||
) | ||
L1SCOUTEventContent.outputCommands.extend(L1TriggerRAW.outputCommands) |
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.
Shall this be changed in a different PR? The L1Scout event content is different from the L1Trigger event content.
For example, we would need something on the line of these commands
"keep *_GmtUnpacker_*_*"
"keep *_CaloUnpacker_*_*",
"keep *_BmtfStubsUnpacker_*_*",
I can pick a different name for the modules if needed.
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-755f0c/38090/summary.html Comparison SummarySummary:
|
@davidlange6, what trivial change you have in mind. Maybe I missed it. In general, we have a long chain of things that we need to wire to make it all work in production. Indeed, L1 scouting seems incomplete and will need to be updated, but this can be done later and we can start testing things already. |
fileName = cms.untracked.string("%s.root" % moduleLabel) | ||
) | ||
|
||
outputModule.dataset = cms.untracked.PSet(dataTier = cms.untracked.string("RAW")) | ||
if dataTier != defaultDataTier: | ||
outputModule.outputCommands = copy.copy(eventContent.outputCommands) |
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.
@drkovalskyi - my suggestion is to remove this line and the previous one, which removes the concern I've raised.
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 think for us it would be ok. We can still use the data tier name to differentiate L1 scouting, HLT scouting, and regular RAW. It also means that T0 will not enforce any content on these new data tiers. What do you think @drkovalskyi?
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 agree. Let's not enforce event content in repacking. It introduces more problems then it solves. We may still need the new EventContent for central production, so I would still keep that part, but what concerns repacking itself, I agree with David's proposal.
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.
they are needed for MC production and/or derived data sets, but those should be correct if defined... ( i guess the one for hlt scouting is already present somewhere as its in miniaod already iiuc.)
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.
For MC uses cases we will have time to fine tune and iterate.
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 open an issue so that you don't forget to clean things up
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've removed these two lines as you suggested.
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 open an issue so that you don't forget to clean things up
Please let us know once you open the issue.
Thanks.
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.
@haozturk could you please follow up on this request
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.
Here is the issue: #44409 Please correct me if there is any missing or incorrect statements there.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44381/39475
|
Pull request #44381 was updated. @antoniovilela, @fabiocos, @cmsbuild, @davidlange6, @rappoccio can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-755f0c/38132/summary.html Comparison SummarySummary:
|
+1 |
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 be automatically merged. |
PR description:
This PR updates the REPACK process such that it is able to generate two new data tiers:
HLTSCOUT
andL1SCOUT
in addition toRAW
. I've updated the event content ofHLTSCOUT
andL1SCOUT
such that they inherit their output commands fromHLTriggerRAW
andL1TriggerRAW
respectively. Additionally, I've included the output commands of L1SCOUT and HLTSCOUT in the output module of the Repack process.I've coordinated with @drkovalskyi @germanfgv @dynamic-entropy on this work.
Related tickets:
PR validation:
Note that I've performed these changes by using "CMSSW_14_0_1" as a base and I've tested them on the same release. The original branch is in this PR. I wanted to rebase my changes into upstream master and re-test, however there has been conflicts during this rebase in files that I haven't changed and don't know how to resolve. The tests I performed and showed below were done in the version that the backport PR uses as source. If anybody can show me how I can test this version, I'm happy to do so. You can find the details of the testing I've performed in the backport PR below:
I used
Configuration/DataProcessing/test/RunRepack.py
for testing. I added a new option to this scriptIf this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
Backport is #44380