Use config flags in analysis config
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
Activity
added full-unit-tests label
This merge request affects 10 packages:
- 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
Affected files list will not be printed in this case
Adding @maszyman ,@ssnyder ,@krumnack ,@gemmeren ,@bdong ,@tadej as watchers
added Analysis CPAlgorithms Core Egamma JetEtmiss Tau Trigger analysis-review-required main labels
- Resolved by Tadej Novak
CI Result FAILURE (hash 831fc972)Athena AthSimulation AthGeneration AnalysisBase 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
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 5834] (remote access info)added 84 commits
-
831fc972...40370048 - 83 commits from branch
atlas:main
- a49687cc - Use config flags in analysis config
-
831fc972...40370048 - 83 commits from branch
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
added Derivation Test labels
CI Result SUCCESS (hash e6edeef1)Athena AthSimulation AthGeneration AnalysisBase AthAnalysis DetCommon 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
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
DetCommon: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 5890] (remote access info)mentioned in issue easyjet/easyjet#209 (closed)
My understanding is that when you use the
initConfigFlags
, this calls functions inAutoConfigFlags
that opens the file and gets information required forPileupReweighting
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 StreblerWould 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.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?
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.
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.
added 204 commits
-
e6edeef1...9ca908cf - 203 commits from branch
atlas:main
- 3e9d5ca9 - Use config flags in analysis config
-
e6edeef1...9ca908cf - 203 commits from branch
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
CI Result SUCCESS (hash 3e9d5ca9)Athena AthSimulation AthGeneration AnalysisBase AthAnalysis DetCommon 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
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
DetCommon: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 6074] (remote access info)mentioned in issue easyjet/easyjet#76