Skip to content

Rewrite ConfigManager (second attempt)

Koen Wolters requested to merge kwolters/allpix-squared:configmanager-new into master

New proposal replacing !111 (closed). This currently mostly implement the same functionality as in the first attempt (including the little bit unrelated added tests) and allows for more flexiblity, see initial discussion here.

The parsing of CLI arguments is now done by a separate class, OptionParser which makes it a bit more obvious that these aren't directly applied to the configs but have to be manually added later by calling applyOptions(). Having this in the ConfigManager always confused me.

Furthermore, this implements changes which fix #112 (closed) and add the version number as well as all global configuration parameters to the data files written by the ROOTObjectWriter.

In this MR the ConfigManager stores the global configuration and the configuration of all modules. This allows the framework and individual modules to change and access centrally stored configuration objects. Most importantly this gives output writers the opportunity to output the final configuration with default keys and the like.

Some points for discussion

  • The OptionParser is now almost completely handled in the ConfigManager again. The getOptionParser is only used to provide parsing options. I think we should probably remove the getOptionParser and move the option parsing to loadOptions (we can keep the class itself).
  • This changes the API to move the ownership of the modules configuration from the modules themselves to the config manager. This is technically not so nice from a modularity perspective, but I think this is only a minor conceptual thing and the extra flexibility of this approach outweighs that.
  • It is probably good to also add a getDetectorConfigurations and move the reading and parsing of the detector file from the GeometryManager to the ConfigManager. More questionable if we should integrate the configurations for the detector models to the config manager as well (I think for now we shouldn't).
  • As in the previous approach I use the empty section now as a name for the global configuration and also changed the exceptions to comply to that. An unnamed configuration is not very nice, but I think this approach still makes more sense.
  • The configuration objects are stored with their original name. This is done to keep in line with the previous implementation and ensures that the name as it appears in the configuration file is printed in case of errors. This means however that the unique name cannot be accessed directly and therefore I added a required key 'unique_name' to every instance configuration to be used for this. This is not a very elegant approach though... any other ideas?
Edited by Koen Wolters

Merge request reports