Skip to content
Snippets Groups Projects

introduce a first version of options for configuration blocks

Merged Nils Erik Krumnack requested to merge krumnack/athena:block_properties into master
All threads resolved!

The basic idea/goal of configuration blocks is that the configuration system can read a configuration text file and then configure the blocks accordingly. This is made a lot simpler if the configurable options on the blocks can be set via a uniform interface.

This is just a first draft of such a mechanism that allows to add the options and set them by name from the python side. All I want from this MR is a basic interface that allows to add the options on the individual blocks and set them through a python interface. That will then allow me to update all the configuration blocks to expose options before I move on.

The mechanism for setting option values is even more rudimentary, doing just the minimal thing to allow setting the properties through a standard interface. This is mostly there to allow me to move forward somehow, but some details may change. The interface I have right now is probably not ideal for anybody, neither for the text nor the python configuration. That probably needs some changing at some point.

There is a much more sophisticated prototype available, but because of its complexity I didn't want to incorporate it into this first draft. Instead the interface I have should be (mostly) compatible with the interface that prototype exposes. It can then be retrofitted in here at some point. The full prototype is here: https://gitlab.cern.ch/jburr/configblock/-/tree/master/

Also: For photons the recomputeIsEM option should probably not be repeated in both the calibration and selection blocks. There needs to be a special method for forwarding options from one block to the next which will come in a future update, or when the full option mechanism gets incorporated here.

Edited by Nils Erik Krumnack

Merge request reports

Pipeline #4864668 passed

Pipeline passed for c8f0d0ed on krumnack:block_properties

Approval is optional

Merged by Tadej NovakTadej Novak 2 years ago (Dec 8, 2022 10:25pm UTC)

Merge details

  • Changes merged into master with 558493d9.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Looks good. Karolos (L2)

  • Jon Burr
  • Jon Burr
  • Jon Burr
  • I'll try to address the comments I got that can be directly implemented, and I hope we can then defer the remaining discussions and get this then merged today or tomorrow.

  • added 1 commit

    • c8f0d0ed - introduce a first version of options for configuration blocks

    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:

    • PhysicsAnalysis/Algorithms/AnalysisAlgorithmsConfig
    • PhysicsAnalysis/Algorithms/EgammaAnalysisAlgorithms
    • PhysicsAnalysis/Algorithms/MuonAnalysisAlgorithms

    Affected files list will not be printed in this case

    Adding @krumnack ,@tadej ,@pagessin as watchers

  • added review-pending-level-1 label and removed review-approved label

  • :white_check_mark: CI Result SUCCESS (hash c8f0d0ed)

    Athena AnalysisBase AthAnalysis
    externals :white_check_mark: :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark: :white_check_mark:
    tests :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: 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-CC7 63222]

  • Contributor

    OK - all my comments are addressed so I'm OK with this now.

  • Jon Burr resolved all threads

    resolved all threads

  • lgtm approving -- L1

  • added review-approved label and removed review-pending-level-1 label

  • Nils Erik Krumnack mentioned in merge request !59093 (merged)

    mentioned in merge request !59093 (merged)

  • Nils Erik Krumnack mentioned in merge request !59099 (merged)

    mentioned in merge request !59099 (merged)

  • Tadej Novak mentioned in commit 558493d9

    mentioned in commit 558493d9

  • merged

  • Sweep summary
    failed:

    • 21.2
  • Nils Erik Krumnack mentioned in merge request !60046 (closed)

    mentioned in merge request !60046 (closed)

  • Please register or sign in to reply
    Loading