Draft: setting custom ComboHypo names as from the core menu code
This change tries to fix the Control Flow rule that wants unique sequences of Filter + menuSequence + ComboHypo. The BLS signature has custom ComboHypos that break this rule because they reuse the same instance of CH (same name, same DH) for all the steps. After discussions, we agree that this exception of the rule can be accepted because the custom BLS CH are able to distinguish the input data handles, and the current code is safe.
Take it as draft for future developments.
related to !74723
Merge request reports
Activity
mentioned in merge request !74723
added 24.0 Trigger TriggerMenu review-pending-level-1 labels
CI Result FAILURE (hash bcc17e25)Athena externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
For experts only: Jenkins output (remote access info)added review-user-action-required label
removed review-pending-level-1 label
This merge request affects 4 packages:
- Trigger/TrigHypothesis/TrigBphysHypo
- Trigger/TrigSteer/DecisionHandling
- Trigger/TriggerCommon/TriggerJobOpts
- Trigger/TriggerCommon/TriggerMenuMT
Affected files list will not be printed in this case
Adding @sutt ,@fpastore ,@abarton ,@lidiaz ,@shanisch ,@martindl ,@lyubushk ,@tamartin ,@fwinkl as watchers
added full-unit-tests review-pending-level-1 labels and removed review-user-action-required label
CI Result FAILURE (hash 9d98597c)Athena AthAnalysis externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 1
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output (remote access info) CI Result FAILURE (hash 17464686)Athena AthAnalysis externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 1
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output (remote access info) CI Result FAILURE (hash 4c639e03)Athena AthAnalysis externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output (remote access info)Hi @tamartin, I have the fix for this again, but I just realised that with all these modifications we go for a critical change, and I need advice on how to solve it:
- basically now we have multiple instances of the Bxx-ComboHypo, one per step, instead of one per job. This is the correct way, since we cannot guarantee the correct merging of decisions. Their name will be
ComboHypo_StepXXX_bmumux_bmumux
instead ofComboHypo_bmumux_bmumux
- they must have unique WH, so that instead of
TrigBphysCollectionKey = HLT_Bmumux
, we will haveTrigBphysCollectionKey = HLT_StepXXX_Bmumux
(basically one for the single leg, one for the double leg processing). This happens for each BxxComboHypo (many of them) - these BphysCollections are final EDM, declared in the TriggerEDMRun3.py
And the question is: should I change the EDM file or do we have other tricks to merge these collections into the final one declared in EDM? Maybe also need advice from some B-phys experts like @abarton
- basically now we have multiple instances of the Bxx-ComboHypo, one per step, instead of one per job. This is the correct way, since we cannot guarantee the correct merging of decisions. Their name will be
Hi @fpastore, I'm looking in https://test-atrvshft.web.cern.ch/test-atrvshft/ART_build/23.0/Athena/x86_64-centos7-gcc11-opt/2024-10-13T0501/TriggerTest/test_trig_data_v1Dev_build/HLTMenu_Dev_pp_run3_v1_TriggerValidation_prescale_23.0.57.json
I think I see the same as you do, we have both
Step5_bmumux
forHLT_2muX_...
chains andStep5_merged_bmumux_bmumux
forHLT_muX_muY_...
chains, where the algorithms which are scheduled by both steps are identical (except for the initial Filters). And they both share the same CH even though they technically have different signatures.But now we come to this more thorny issue of the custom BLS combo hypo, this alg has a special-case in that it is also forming BLS physics objects from the output of the muon Hypo. Hence it is writing a physics collection, and adding
"feature"
links in the navigation to these objects in addition to performing all of the CH duties over the BLS objects.We only have tricks to merge physics collections from inside EventView instances, and I don't think we want to be using that here.
Keeping the existing EDM entry name for (e.g.) the
2muX
chains and adding one new one for themuX_muY
chains could be an option? Feature access via the TDT would be fine as the different CH nodes would be linked to their respective physics collections, offline BLS monitoring & physics code might need to adapt however to the two containers. If we do this, I would make the output-collection-config python quite explicit, such that if in the future a chain is added to the menu which attempts to generate a third chain signature here then it would print a clear error message saying what needs to be done in the EDM.That's just my initial idea though, maybe there are better solutions here...
cc also @lyubushk
Hi, thinking more on this change, I have to admit that this is an example of a broken HLT-CF law (one CH per Filter) that indeed does not make any difference. Indeed the
BmumuxComboHypo
is getting as inputs the outputs ofBmumuxStreamerHypoAlg
which is run in both single and double leg sequences, and thus the decisions are merged in the InputMaker of the (common) sequence. The reason on this change indeed is because I'm trying to remove the inputMakers in the empty steps, and this of course is not compatible with the current system. Thus I have to think how to handle this conflict, but I can try to avoid to make this change here, since I understand that this can have a lot of consequences that maybe are not needed (see also next point).This makes me thinking more on another optimisation that I was thinking for the future, that is to reduce the ComboHypos (that are around thousand in the menu) by having only one instance of ComboHypo ‘type’ per each OR-step (the OR of all the chain steps). The CH is combining legs with a dictionary that map DH with decisions, so one single CH could make the job, with one big dictionary for all the decisions and the DHs of the steps in the step-OR.
Comments, @tamartin ?
Hi @fpastore - one CH instance per step? So something like 10-20 CH for the entire menu?
In theory it can be done. The logic is totally generic over signatures (bar BLS :) ). but I think from memory that the current level of I/O mapping we have in the CH implementation will not support it. I think we only support mapping within one chain signature per CH, so the config of a CH for
[e,mu]
is distinct from (and incompatible with)[e,tau]
and from[e,tau,tau]
, etc.. I would need to look again.If we were to do this, then we still need both of these steps to continue to use the custom
BmumuxComboHypo
. But I guess the argument there is that this is now OK as we broke the one-F-to-one-CH rule?yes, I think we don’t need this fix. Without thinking tooo far (maybe it’s to early to think of a unique CH per OR-step), I think this BLS break-of-the-rule demonstrates that the rule "unique Filter->step->CH" is not necessary. Basically if we remove this rule we can reduce the number of the CHs because we use the same CH for all leg combinations (single, double,3,...). On average we would at least half the number of CHs! The idea is to use one CH per ChainStep, BUT multiple Filters per ChainStep, so the rule becomes “multiFilters->step->CH”. I don't recall now if there are cases against it, but it would need a critical change in the CF building (when the filter is used to identify the step) and in the alignment code, when the legs are merged (and the mergedXXX-CH are created). I think reducing the number of CHs is quite important given that they are large consumers of CPU time (because they are a loooot).
On the other hand, if we break this rule, probably the removal of the IM in the empty steps is not possible anymore, but I have to think about it.
The BLS CH is niche enough that it doesn't even need to attempt to do a leg mapping, it knows it is using the output of the muon Hypo for both the
[mu]
chains and the[mu,mu]
chains and as such it hard-codes to read everything from input #0 https://acode-browser1.usatlas.bnl.gov/lxr/source/athena/Trigger/TrigHypothesis/TrigBphysHypo/src/TrigBmumuxComboHypo.cxx#0105Suggest consider merging generic CHs at a later time, we first need to understand how the input mapping to the CH changes when in the empty steps we don't always have an IM sitting in front and merging.
Dear @fpastore and @tamartin , I am sorry for my very late reply.
I have almost nothing to add to what Tim has already said. There is only one thing I would like to note. TrigBmumuxComboHypo can be rather CPU consuming (calling vertex fitter for 2mu+tracks combinations). So that, running it twice for the same HLT muons (first time for chains like HLT_2mu4_bBmumux, and then for chains like HLT_mu6_mu4_bBmumux) would double the time. The overlap between two EDM bmumux collections would be large, but TrigBphys objects are small, so this can be bearable.
The final HLT response will be correct.
sorry I deleted the comment, because I realised it can be confusing and creating big alert!
I’m still investigating.
Edited by Francesca PastoreThanks @fpastore - I think we're definitely in "core SW presentation" territory here with these discussions and observations made so far :)
Hi @tamartin, my changes for the empty sequences requested a shuffle of operations in the CF building, so I basically anticipated the merging of the CHs before they actually were finally configured. I found the bug, so there is no need to change any of these BLS CH behaviour. The system is able to handle common CHs.
I’m trying to make the code cleaner and more documented, so that we don’t end up in such discussions again. I can report the work at the meeting, once I have all solved.