Skip to content
Snippets Groups Projects

Use config flags in analysis config

Closed Tadej Novak requested to merge tadej/athena:cp-algs-flags into main

Make config flags the main way to steer analysis config. This will help people working on analysis and Athena to feel at home and prevent accumulation of arguments of the ConfigAccumulator.

The naming is not fixed, I will probably also convert more items to flags. This is mainly to start the discussion and freeze the API as soon asp possible. I will probably then break this into multiple MRs.

Also some core flags should be renamed (see https://its.cern.ch/jira/browse/ATEAM-964).

Tagging @krumnack, @jolamber, @gwatts, @ekourlit for core AMG. Also tagging interested parties @ravinab, @omajersk, @tstreble, @khoo, @jchapman.

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
  • :x: CI Result FAILURE (hash 831fc972)

    Athena AthSimulation AthGeneration AnalysisBase AthAnalysis
    externals :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    tests :o: :white_check_mark: :white_check_mark: :white_check_mark: :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: AthSimulation: number of compilation errors 0, warnings 0
    :white_check_mark: AthGeneration: number of compilation errors 0, warnings 0
    :white_check_mark: AnalysisBase: number of compilation errors 0, warnings 0
    :white_check_mark: AthAnalysis: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 5834] (remote access info)

  • Tadej Novak added 84 commits

    added 84 commits

    Compare with previous version

  • Tadej Novak added 1 commit

    added 1 commit

    • e6edeef1 - Use config flags in analysis config

    Compare with previous version

  • Author Developer

    Jenkins please retry a build

  • This merge request affects 12 packages:

    • AtlasTest/CITest
    • Control/AthenaConfiguration
    • PhysicsAnalysis/Algorithms/AnalysisAlgorithmsConfig
    • PhysicsAnalysis/Algorithms/AsgAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/EgammaAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/FTagAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/JetAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/MetAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/MuonAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/TauAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/TriggerAnalysisAlgorithms
    • PhysicsAnalysis/DerivationFramework/DerivationFrameworkPhys

    Affected files list will not be printed in this case

    Adding @fwinkl ,@gemmeren ,@maszyman ,@ssnyder ,@pagessin ,@jcatmore ,@dshope ,@bdong ,@tadej ,@zmarshal ,@krumnack ,@emmat as watchers

  • :white_check_mark: CI Result SUCCESS (hash e6edeef1)

    Athena AthSimulation AthGeneration AnalysisBase AthAnalysis DetCommon
    externals :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    tests :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :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: AthSimulation: number of compilation errors 0, warnings 0
    :white_check_mark: AthGeneration: number of compilation errors 0, warnings 0
    :white_check_mark: AnalysisBase: number of compilation errors 0, warnings 0
    :white_check_mark: AthAnalysis: number of compilation errors 0, warnings 0
    :white_check_mark: DetCommon: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 5890] (remote access info)

    • Author Developer

      Hi @krumnack, @jolamber, any feedback here? It may be good to merge this before the tutorial as it's slightly changing the API.

    • Author Developer

      Pinging @krumnack and @jolamber again.

    • My understanding is that when you use the initConfigFlags, this calls functions in AutoConfigFlags that opens the file and gets information required for PileupReweighting and maybe other blocks to work properly. Will this code run the same when running on the grid and there is no local sample to get these values compared to when we run locally?

    • Some input file will always be used. That's where the metadata will be read from to configure the flags.

      Edited by Thomas Strebler
    • Author Developer

      We need to change grid driver, that's true. But in that case anyways now the user has to set all the properties (now flags). So besides renaming, nothing will change.

    • Would it be useful to add options in the run script arg parser for all flags that are required to correctly configure the PileupReweighting block when submitting to the grid? This is just to document what information would need to be provided by the user. It could also be added to the SW documentation as well/instead.

    • Author Developer

      We do not have a central argument parser, but we should probably indeed set it up.

    • So the change to allow running the configuration on the grid node will require a somewhat bigger change to how jobs are submitted, and that will also include a way for the configuration scripts (as well as central EventLoop components) to add some command line options.

      That shouldn't keep us from setting up a central argument parser, but we should make sure that whatever we build doesn't clash or get obsoleted by the changes coming to EventLoop.

      As for this MR: This seems mostly like an internal change, and as such it is fine by me. The only part that has me concerned is the renaming of data types. Do the old names still work, or did they get completely replaced?

    • Author Developer

      It's not internal. Now options are set in a different way on the accumulator.

      Regarding data types, we changed them many months ago so probably it's fine to drop the legacy naming at this point. Of course when setting the flags one can still do a conversion layer in front, but changing 3 variables is much lower overhead for analyses than switching to use flags.

      Before we merge this I'm happy to provide a list of flags that are currently used and we list them somewhere for people to set if they use grid driver as before.

    • Ok, in that case let me take another look. I'm a bit oversubscribed right now with the analysis tutorial, but that's hopefully over tomorrow if not today.

    • So, for the data types: If that's been changed many months ago and it would be hard to support to support the old names, we can probably live with dropping the old names.

      What I understand less is why we can't support the old constructor parameters or accessors on ConfigAccumulator, at least for a transition period. As it stands user code that works before the change won't work afterwards, and vice versa. Usually we try to have at least a transition period in which both the old and new way work.

      The main point of having accessors instead of just having the users access data members directly is that they can be updated to different backends without the user being affected. So, I'm not quite sure why the moment we change the backend we rip out everything that could smooth out that change.

      I'm of course a fan of updating everything in the repository, but I'm not sure at this point we can rely on all relevant code being in the repository. Particularly when it comes to instantiating ConfigAccumulator that's something users actually have to do to use the block configuration.

    • Please register or sign in to reply
  • Tadej Novak added 204 commits

    added 204 commits

    Compare with previous version

  • Author Developer

    Jenkins please retry a build

  • This merge request affects 12 packages:

    • AtlasTest/CITest
    • Control/AthenaConfiguration
    • PhysicsAnalysis/Algorithms/AnalysisAlgorithmsConfig
    • PhysicsAnalysis/Algorithms/AsgAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/EgammaAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/FTagAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/JetAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/MetAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/MuonAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/TauAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/TriggerAnalysisAlgorithms
    • PhysicsAnalysis/DerivationFramework/DerivationFrameworkPhys

    Affected files list will not be printed in this case

    Adding @maszyman ,@ssnyder ,@krumnack ,@pagessin ,@emmat ,@zmarshal ,@jcatmore ,@bdong ,@fwinkl ,@tadej ,@dshope ,@gemmeren as watchers

  • :white_check_mark: CI Result SUCCESS (hash 3e9d5ca9)

    Athena AthSimulation AthGeneration AnalysisBase AthAnalysis DetCommon
    externals :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    tests :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :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: AthSimulation: number of compilation errors 0, warnings 0
    :white_check_mark: AthGeneration: number of compilation errors 0, warnings 0
    :white_check_mark: AnalysisBase: number of compilation errors 0, warnings 0
    :white_check_mark: AthAnalysis: number of compilation errors 0, warnings 0
    :white_check_mark: DetCommon: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 6074] (remote access info)

  • mentioned in issue easyjet/easyjet#76

  • Tadej Novak marked this merge request as ready

    marked this merge request as ready

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