Update packages needed to make ConfigFlag dual use in Athena and AnalysisBase
Note: this is a second attemp at !54147 (merged) which was reverted since it removed athena.py from AthAnalysis. I incorrectly used XAOD_ANALYSIS assuming it only referred to AnalysisBase (not the case; it also includes AthAnalysis). Here I've changed it to XAOD_STANDALONE, and added the proper labels to do a full suite a build checks.
This MR includes 3 new packages in AnalysisBase:
Control/AthenaCommon
Control/AthenaConfiguration
Tools/PyUtilsWhile AthenaCommon isn't strictly necessary, it pulls in Logging, Constants and SystemOfUnits which reduces the number of try ... except statements in AthenaConfiguration. The compilation is updated to only pull in these three files when in AnalysisBase, otherwise try statements in AnaAlgorithm think it's running in Athena (since this package is available) and set things up incorrectly in AnalysisBase.
Checks for the Gaudi environment are added to MetaReader so it also works when running in AnalysisBase. The main difference is to rely on FileMetaData instead of EventStreamInfo for filling many of the basic properties, e.g., isMC.
AllConfigFlags is largely kept the same. This means AnalysisBase has many flags loaded that are not relevant. If some of the irrelevant (for AB) flags are called, they will cause a crash since this results in an import of modules only available in Athena. Since they are lazy loaded, things work fine as long as these flags aren't called by a user.
Finally, six is removed (dual compatability for python 2 and 3) since it is no longer needed in 22.*, and this keeps us from adding it as a dependency to AnalysisBase.
Merge request reports
Activity
added NewConfig full-unit-tests labels
assigned to @cjmeyer
added Build Core JetEtmiss Tools analysis-review-required master review-pending-level-1 labels
72 if "HLTResult_EF" in collections: 73 return 1 74 elif "TrigNavigation" in collections: 75 return 2 76 elif any("HLTNav_Summary" in s for s in collections): 77 return 3 78 elif not flags.Input.Collections: 79 # Special case for empty input files (can happen in merge jobs on the grid) 80 # The resulting version doesn't really matter as there's nothing to be done, but we want a valid configuration 81 return 3 82 83 return default_version 84 85 acf.addFlag('Trigger.EDMVersion', lambda prevFlags: EDMVersion(prevFlags)) 86 87 acf.addFlag("Sim.ISF.Simulator", lambda prevFlags: GetFileMD(prevFlags.Input.Files).get('simFlavour', 'Unkown')) 
Bit of a snag here: the
SimEnumslive in the SimulationConfig package. I think it's quite useful to have the simulation flavor in AnalysisBase python configuration, which means we need to:- Pull in
SimulationConfig/SimEnums.pytoAnalysisBasesomehow; or - Keep
simFlavoura simple string (based on what's in the file?) forAnalysisBase.
Thoughts?
- Pull in
-
We can maybe move the enum, but I'd wait that @jchapman is back from holidays. What if we exclude this specific flag for now?
changed this line in version 2 of the diff
Hi @cjmeyer,
I created the
Simulation/SimulationConfigpackage, so that the flag+enum definitions and auto-configuration could be included inAthAnalysis. It should have minimal other dependencies. If there is something that makes the package incompatible withAnalysisBasethen let me know and we can try to figure something out.Cheers,
John
-
Hi @cjmeyer, thanks for following up so quickly!
I'm mainly worried about readability of
AllConfigFlags.pyafter this change. The other comments are minor.I also wonder about the duplication of python files between
AthenaCommonandAnaAlgorithmpackages. While I do not want to delay this much further, I'm afraid that we will end up with just more duplicated code. Tagging @khoo and @krumnack on their opinon on that.Also in the future I recommend opening multiple smaller MRs (this one could easily be split into metadata,
AthenaCommonand flags ones). This usually makes the review process much faster. - Resolved by Tadej Novak
Thanks for the comments @tadej!
For
AllConfigFlags.py: along the lines of yourInputconfig comments, I can alternatively keep the this file more-or-less as it currently is. Many of the options would be irrelevant, but it would require fewer changes / differences between projects. The only dangerous config flags are a few that call functions (likegetInitialTimeStampsFromRunNumbers()) which won't work inAnalysisBase. However, since the config flags aren't populated unless requested by a user, it will only crash if someone asks for something they shouldn't. I would also need to add a "return" before the portion that contains calls to modules that are only available in Athena, and instead populate only the subset I currently do.How this is done probably comes down to design philosophy. I'm happy to go with whatever core developers feel is best here.
- Last reply by Chris Meyer
added 1 commit
- 6eedfd62 - Include comments from MR, fix AB compilation issue
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 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 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
The CI is reporting the error:
File "/var/lib/jenkins/workspace/CI-MERGE-REQUEST-CC7/master/Tools/PyUtils/bin/apydep.py", line 15, in <module> import pygraphviz ModuleNotFoundError: No module named 'pygraphviz'and I see there were some changes related to that package in the CMakeLists, so could you please look into fixing this?
Albert (L1)added review-user-action-required label and removed review-pending-level-1 label
added 266 commits
-
7f27130d...65fc58eb - 265 commits from branch
atlas:master - 436c812a - Update packages needed to make ConfigFlag dual use in Athena and AnalysisBase
-
7f27130d...65fc58eb - 265 commits from branch
added 1 commit
- dfdde9ae - Fix CMake files so that AthAnalysis compiles properly
added 5 commits
Toggle commit list-
Sorry for the mess above. I pushed a bad
git rebase(didn't squash things far enough back and lost changes) and had to reset it. All comments should now be addressed, although I'm leaving one open with @tadej to keep things from merging.I've fixed the compilation issues from the previous CI and things look good locally. Removing Draft now to re-run CI tests.
added review-pending-level-1 label and removed review-user-action-required label
-
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
From L1 side, this looks fine. I'll set review-pending-expert, so Tadej can comment. -L1
added review-pending-expert label and removed review-pending-level-1 label
-
For the first version this looks OK for me now.
I think we should also sweep to 22.0 to avoid conflicts in the future.
removed review-pending-expert label
added alsoTargeting:22.0 review-pending-level-1 labels
added review-approved label and removed review-pending-level-1 label
added analysis-review-approved label and removed analysis-review-required label
mentioned in commit d6b963a4
added sweep:done label
added sweep:failed label
mentioned in commit tadej/athena@e5d079e6
