Skip to content
Snippets Groups Projects

Draft: setting custom ComboHypo names as from the core menu code

Open Francesca Pastore requested to merge fpastore/athena:CustomCH into 24.0

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

Edited by Francesca Pastore

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francesca Pastore mentioned in merge request !74723

    mentioned in merge request !74723

  • This merge request affects 2 packages:

    • Trigger/TrigHypothesis/TrigBphysHypo
    • Trigger/TriggerCommon/TriggerMenuMT

    Affected files list will not be printed in this case

    Adding @lidiaz ,@shanisch ,@lyubushk ,@abarton ,@sutt as watchers

  • :x: CI Result FAILURE (hash bcc17e25)

    Athena
    externals :white_check_mark:
    cmake :white_check_mark:
    make :white_check_mark:
    tests :o:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output (remote access info)

  • added 2 commits

    • 47e8528b - fixed hash key with correct CH function name
    • 9d98597c - fix WH names for BPhys CH, removed double run of collectHypoDecisionObjects

    Compare with previous version

  • 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

  • :x: CI Result FAILURE (hash 9d98597c)

    Athena AthAnalysis
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :warning: :white_check_mark:
    tests :o: :white_check_mark:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :warning: Athena: number of compilation errors 0, warnings 1
    :white_check_mark: AthAnalysis: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output (remote access info)

  • added 1 commit

    Compare with previous version

  • :pencil: Build area was cleaned as per request posted in the DB. The full software build will be performed

  • This merge request affects 3 packages:

    • Trigger/TrigHypothesis/TrigBphysHypo
    • Trigger/TriggerCommon/TriggerJobOpts
    • Trigger/TriggerCommon/TriggerMenuMT

    Affected files list will not be printed in this case

    Adding @shanisch ,@lyubushk ,@fpastore ,@lidiaz ,@martindl ,@sutt ,@abarton ,@fwinkl as watchers

  • :x: CI Result FAILURE (hash 17464686)

    Athena AthAnalysis
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :warning: :white_check_mark:
    tests :o: :white_check_mark:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :warning: Athena: number of compilation errors 0, warnings 1
    :white_check_mark: AthAnalysis: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output (remote access info)

  • added 1 commit

    • 4c639e03 - fix B-phys naming and fix crash

    Compare with previous version

  • This merge request affects 3 packages:

    • Trigger/TrigHypothesis/TrigBphysHypo
    • Trigger/TriggerCommon/TriggerJobOpts
    • Trigger/TriggerCommon/TriggerMenuMT

    Affected files list will not be printed in this case

    Adding @shanisch ,@sutt ,@abarton ,@martindl ,@fpastore ,@lyubushk ,@lidiaz ,@fwinkl as watchers

  • :x: CI Result FAILURE (hash 4c639e03)

    Athena AthAnalysis
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark:
    tests :o: :white_check_mark:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :white_check_mark: AthAnalysis: number of compilation errors 0, warnings 0
    :pencil: 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 of ComboHypo_bmumux_bmumux
      • they must have unique WH, so that instead of TrigBphysCollectionKey = HLT_Bmumux , we will have TrigBphysCollectionKey = 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

    • 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 for HLT_2muX_... chains and Step5_merged_bmumux_bmumux for HLT_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 the muX_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

    • hmmm, obviously changing the names may affect things downstream but I will have to check with @lyubushk and @tursom if this can be easily adapted to.

    • 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 of BmumuxStreamerHypoAlg 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 ?

    • This could be allowed:

      image.png

    • 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#0105

      Suggest 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 Pastore
    • ....and actually I found that the CA merging does the trick. So no alert.

    • Thanks @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.

    • Please register or sign in to reply
  • added 1 commit

    Compare with previous version

  • :pencil: Build area was cleaned as per request posted in the DB. The full software build will be performed

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading