ATR-22325: Call HLTTriggerResultGetter from the place where DataSource is properly configured

Move HLTTriggerGetter for MT ESDtoAOD step from skeleton.ESDtoAOD_tf.py to RecExCommon_topOptions.py, at the point where globalflags.DataSource is properly configured. Tagging @elmsheus and @tamartin

This merge request affects 2 files:

• Reconstruction/RecExample/RecExCommon/share/RecExCommon_topOptions.py
• Reconstruction/RecJobTransforms/share/skeleton.ESDtoAOD_tf.py

Adding @goetz as watcher

added Reconstruction master review-pending-level-1 labels

resolved all threads

added 1 commit

• 138540eb - Propagate L1 items from ESD to AOD in athenaMT mode

Compare with previous version

This merge request affects 2 files:

• Reconstruction/RecExample/RecExCommon/share/RecExCommon_topOptions.py
• Reconstruction/RecJobTransforms/share/skeleton.ESDtoAOD_tf.py

Adding @goetz as watcher

CI Result SUCCESS (hash 4a91df01)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 22980]

CI Result SUCCESS (hash 138540eb)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 22993]

added review-user-action-required label and removed review-pending-level-1 label

added 1 commit

• f1fa645c - Improve readability

Compare with previous version

resolved all threads

This merge request affects 2 files:

• Reconstruction/RecExample/RecExCommon/share/RecExCommon_topOptions.py
• Reconstruction/RecJobTransforms/share/skeleton.ESDtoAOD_tf.py

Adding @goetz as watcher

added review-pending-level-1 label and removed review-user-action-required label

CI Result SUCCESS (hash f1fa645c)

	Athena	AthSimulation	AthGeneration	AnalysisBase
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 23143]

added review-approved label and removed review-pending-level-1 label

merged

mentioned in commit 260656bc

added sweep:ignore label

Hi, this change unfortunately breaks standalone running without trigger. Can we catch the case where the TriggerFlags are not around?

See unit test failure in master: https://atlas-computing.web.cern.ch/atlas-computing/links/distDirectory/gitwww/MASTERWebArea/ardoc_web_areamaster64BC7G8AthenaOpt/ARDOC_TestLog_2020-11-09T2101/InnerDetector_InDetValidation_InDetPhysValMonitoring___InDetPhysValMonitoringConf__InDetPhysValMonitoringTest__m.html

reported in https://its.cern.ch/jira/browse/ATR-22351

Hi @okuprash, this is wrong. TriggerFlags.doMT is only meant for steering whether we run Run-2 or Run-3 Trigger. We do not run any Trigger here, as you wrote in the comment below. What the flag actually means is only whether this is AthenaMT or serial athena. It has no relation to EDM. The configuration of trigger content reading/writing in offline reconstruction should depend solely on the EDM version (Run-2 or Run-3 EDM). This can be checked by calling HLTTriggerResultGetter.EDMDecodingVersion and then checking the flag TriggerFlags.EDMDecodingVersion which it sets. The doMT flag is completely irrelevant here. Whether we read Run-2 or Run-3 EDM doesn't depend on the mode of running offline reconstruction. It only depends on the contents of the input file.

Hi @rbielski,

I never claimed/meant that doMT is related to the EDM version. It is the difference in AOD trigger content between MT and serial Reco_tf.py which is being fixed here.

Adding @tamartin to the discussion, who implemented the initial temporary trigger block in https://gitlab.cern.ch/atlas/athena/-/blob/master/Reconstruction/RecJobTransforms/share/skeleton.ESDtoAOD_tf.py#L51 which I split in two pieces, moving the second piece to the place where the globalflags.DataType is set correctly, without changing the logic.

Hi Tim, it seems the flags rec.doTrigger and recAlgs.doTrigger behave differently between MT and serial athena:

• MT ESDtoAOD: both are False (as set in the temporary block)
• Serial ESDtoAOD: recAlgs.doTrigger is False and rec.doTrigger is True. The latter allows scheduling the TriggerGetter and adds HLT, L1, and metadata to the output stream, in https://gitlab.cern.ch/atlas/athena/-/blob/master/Reconstruction/RecExample/RecExCommon/share/RecExCommon_topOptions.py#L556

Maybe it is this behaviour which should be understood/confirmed.

Cheers,
Oleg

Thanks Oleg, so yes, the block which you split was already wrong before. We shouldn't have any difference between running reconstruction in AthenaMT and in serial athena. Probably dates back to early prototyping before we established how to determine what to run.

We have one "nice" block of R3 offline configuration: https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Trigger/TriggerCommon/TriggerJobOpts/python/TriggerConfig.py#0391

But we only use this in trigger jobs which also export POOL - so this is only for R3 MC, and the data are present and propagated forward from the RDO_TRIG.

For bytestream with either R2 or R3 trigger payload, the offline trigger code is run in RAWtoESD and the R3 changes are interwoven with the R2 ones, keyed off of EDMDecodingVersion.

From the conf side, the key places are, HLTTriggerResultGetter: BS Unpacking: https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Trigger/TriggerCommon/TriggerJobOpts/python/HLTTriggerResultGetter.py#0389

Trigger Decision Writing (this is behind a if recAlgs.doTrigger() or TriggerFlags.doTriggerConfigOnly(), inherited from R2) https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Trigger/TriggerCommon/TriggerJobOpts/python/HLTTriggerResultGetter.py#0401

TriggerConfigGetter:

Metadata writing: https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Trigger/TriggerCommon/TriggerJobOpts/python/TriggerConfigGetter.py#0320

---

We then have https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Event/xAOD/xAODTriggerCnv/Root/TriggerMenuMetaDataTool.cxx to propagate metadata POOL->POOL, down the ESD->AOD->... chain.

Though from what I understood, we were also missing the ObjKeyStore side of things too with --threads=1. So the metadata wasn't getting propagated.

So can we just delete this block then? https://gitlab.cern.ch/atlas/athena/-/blame/master/Reconstruction/RecJobTransforms/share/skeleton.ESDtoAOD_tf.py#L60

HLTTriggerResultGetter is smart in that it will only write metadata if it does not already exist (so only during RAWtoESD).

0319         if not self.hasxAODMeta:
0320             self.setupxAODWriting()

The metadata tool (to do the POOL->POOL propagation) is setup via https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Reconstruction/RecExample/RecExCommon/share/RecExCommon_topOptions.py#0950

And the addManyTypesMetaData for the payload could also be moved to RecExCommon_topOptions.py#0950 too?

So can we just delete this block then? https://gitlab.cern.ch/atlas/athena/-/blame/master/Reconstruction/RecJobTransforms/share/skeleton.ESDtoAOD_tf.py#L60

That's what I'm wondering about too - seems to be a right thing to do to be consistent between MT and non-MT. (Though if I remember correctly, I tried to do this in the course of fixing the jira in the title, but ran into other problems)

The question is, should recAlgs.doTrigger and rec.doTrigger be false in all reco cases, or whether there are some cases when either of these should be true?

The only differences should really come from either Run-2 vs Run-3 Trigger EDM or the input file format (RDO without trigger, RDO_TRIG, or BS). If there is any case for differences between the doTrigger flag settings based on these two aspects, then the corresponding flags (EDMDecodingVersion or the common flags for input type) should be used to steer this instead of doMT.

That doMT steering of DQMonFlags in skeleton.ESDtoAOD_tf.py is also shady. I don't know the intention, but this should also depend on EDM version and/or input type.

I think we should just get rid of the abused and confusing doMT flag. It has only two genuine use cases. One is internal to TriggerFlags:
https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Trigger/TriggerCommon/TriggerJobOpts/python/TriggerFlags.py#0945
and the other is here:
https://acode-browser.usatlas.bnl.gov/lxr/source/athena/Reconstruction/RecJobTransforms/share/skeleton.RDOtoRDOtrigger.py#0006

Everything else is wrong:
https://acode-browser.usatlas.bnl.gov/lxr/search?%21v=head&_filestring=&_string=doMT&_casesensitive=1

The internal use case in TriggerFlags can be replaced with

if os.getenv('TDAQ_PARTITION') or jobproperties.ConcurrencyFlags.NumThreads() >= 1:

and the one in RDOtoRDOTrigger with just:

if jobproperties.ConcurrencyFlags.NumThreads() >= 1:

Opened https://its.cern.ch/jira/browse/ATR-22360