JeffersonLab / JANA2

Multi-threaded HENP Event Reconstruction
https://jeffersonlab.github.io/JANA2/
Other
6 stars 9 forks source link

JANA2@master: duplication of std::vector<std::string> default parameters #256

Closed wdconinc closed 10 months ago

wdconinc commented 10 months ago

With JANA2@master (1dc91585389da3ba96147a3fa203e6efae1a0471), the interpretation of the geometry parameter dd4hep::xml_files leads to duplication of the default value.

Steps to reproduce

  1. Compile EICrecon (I was using 8bfe438e821e4e8d7c2832b682b00e570c66b0d3) against JANA2@master
  2. Set geometry to craterlake, export DETECTOR_CONFIG=epic_craterlake
  3. Run eicrecon sim_dis_18x275_minQ2=1000_craterlake.edm4hep.root with that file from https://github.com/eic/EICrecon/actions/runs/6566489975

Expected results

This should load geometry once.

Actual results

Tries (and fails) to load geometry twice (then 4 times, then 8 times, you get the idea).

[JEventProcessorPODIO] [info] Persisting collection 'ZDCEcalClusterAssociations'
[JEventProcessorPODIO] [info] Persisting collection 'ZDCEcalClusters'
[JEventProcessorPODIO] [info] Persisting collection 'ZDCEcalRawHits'
[JEventProcessorPODIO] [info] Persisting collection 'ZDCEcalRecHits'
[JEventProcessorPODIO] [info] Persisting collection 'ZDCEcalTruthClusterAssociations'
[JEventProcessorPODIO] [info] Persisting collection 'ZDCEcalTruthClusters'
PersistencyIO    INFO  +++ Set Streamer to dd4hep::OpaqueDataBlock
Info in <TGeoManager::TGeoManager>: Geometry , Detector Geometry created
Info in <TGeoNavigator::BuildCache>: --- Maximum geometry depth set to 100
[INFO] Loading DD4hep geometry from 2 files
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'
[INFO]   - loading geometry file:  '/opt/local/share/epic/epic_craterlake.xml' (patience ....)
Info in <TGeoManager::SetTopVolume>: Top volume is world_volume. Master volume is world_volume
Info in <TGeoManager::CheckGeometry>: Fixing runtime shapes...
Info in <TGeoManager::CheckGeometry>: ...Nothing to fix
Info in <TGeoManager::CloseGeometry>: Counting nodes...
Info in <TGeoManager::Voxelize>: Voxelizing...
Info in <TGeoManager::CloseGeometry>: Building cache...
Info in <TGeoManager::CountLevels>: max level = 7, max placements = 2989
Info in <TGeoManager::CloseGeometry>: 4859544 nodes/ 2895 volume UID's in Detector Geometry
Info in <TGeoManager::CloseGeometry>: ----------------modeler ready----------------
[INFO]   - loading geometry file:  '/opt/local/share/epic/epic_craterlake.xml' (patience ....)
[ERROR] Problem loading geometry: dd4hep: Attempt to add an already existing object:world_limits.
dd4hep: Error interpreting XML nodes of type <limitset/>
dd4hep: Error interpreting XML nodes of type <limits/>
dd4hep: while parsing /opt/local/share/epic/epic_craterlake.xml
[WARN] Parameter 'dd4hep:xml_files' has conflicting defaults: '/opt/local/share/epic/epic_craterlake.xml,/opt/local/share/epic/epic_craterlake.xml,/opt/local/share/epic/epic_craterlake.xml' vs '/opt/local/share/epic/epic_craterlake.xml'
Info in <TGeoManager::TGeoManager>: Geometry , Detector Geometry created
[INFO] Loading DD4hep geometry from 4 files
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'
[INFO]   - loading geometry file:  '/opt/local/share/epic/epic_craterlake.xml' (patience ....)
Evaluator        WARN  +++ Overwriting variable: name=ElectronBeamEnergy value=5*GeV  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=IonBeamEnergy value=41*GeV  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=FieldScaleFactor value=1.0  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=Q1eR_Gradient value=-0.739741*tesla/meter/GeV*ElectronBeamEnergy  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=Q2eR_Gradient value=0.66821*tesla/meter/GeV*ElectronBeamEnergy  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=Q3eR_Gradient value=0.21674*tesla/meter/GeV*ElectronBeamEnergy  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=B2AeR_B value=0.0106667*tesla/GeV*ElectronBeamEnergy  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=B2BeR_B value=0.0132222*tesla/GeV*ElectronBeamEnergy  Evaluator::Object : existing variable [setVariable error]
Evaluator        WARN  +++ Overwriting variable: name=LumiSweepMag_B value=1/1.2*tesla  Evaluator::Object : existing variable [setVariable error]

Debugging

Info lines:

[INFO] Loading DD4hep geometry from 2 files
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'
[INFO]   - '/opt/local/share/epic/epic_craterlake.xml'

were added with

diff --git a/src/services/geometry/dd4hep/DD4hep_service.cc b/src/services/geometry/dd4hep/DD4hep_service.cc
index 214fcc45..1240afdf 100644
--- a/src/services/geometry/dd4hep/DD4hep_service.cc
+++ b/src/services/geometry/dd4hep/DD4hep_service.cc
@@ -106,6 +106,11 @@ void DD4hep_service::Initialize() {
     try {
         dd4hep::setPrintLevel(static_cast<dd4hep::PrintLevel>(print_level));
         LOG << "Loading DD4hep geometry from " << m_xml_files.size() << " files" << LOG_END;
+        for (auto &filename : m_xml_files) {
+            auto resolved_filename = resolveFileName(filename, detector_path_env);
+            LOG << "  - '" << resolved_filename << "'" << LOG_END;
+        }
+
         for (auto &filename : m_xml_files) {

             auto resolved_filename = resolveFileName(filename, detector_path_env);

Relevant JANA2 calls at https://github.com/eic/EICrecon/blob/acts-upgrade-26/src/services/geometry/dd4hep/DD4hep_service.cc#L64-L84:

    // The current recommended way of getting the XML file is to use the environment variables
    // DETECTOR_PATH and DETECTOR_CONFIG or DETECTOR(deprecated).
    // Look for those first, so we can use it for the default
    // config parameter. (see https://github.com/eic/EICrecon/issues/22)
    auto *detector_config_env = std::getenv("DETECTOR_CONFIG");
    auto *detector_path_env = std::getenv("DETECTOR_PATH");

    std::string detector_config;
    // Check if detector_config_env is set
    if(detector_config_env != nullptr) {
        detector_config = detector_config_env;
    }

    // do we have default file name
    if(!detector_config.empty()) {
        m_xml_files.push_back(std::string(detector_path_env ? detector_path_env : ".") + "/" + detector_config + ".xml");
    }

    // User may specify multiple geometry files via the config. parameter. Normally, this
    // will be a single file which itself has includes for other files.
    app->SetDefaultParameter("dd4hep:xml_files", m_xml_files, "Comma separated list of XML files describing the DD4hep geometry. (Defaults to ${DETECTOR_PATH}/${DETECTOR_CONFIG}.xml using envars.)");

Passing -Pdd4hep:xml_files=$DETECTOR_PATH/$DETECTOR_CONFIG.xml works fine; it's just the defaults that are duplicated on parsing.

wdconinc commented 10 months ago

Hi @AyanRoy16 , I haven't looked through the recent parameter handling fixes you implemented, but that's where I would start. I don't think EICrecon does anything strange, so I expect this can be reproduced in a unit test.

wdconinc commented 10 months ago

https://github.com/JeffersonLab/JANA2/pull/244 is the one I'd bisect around (if I had the time)

nathanwbrei commented 10 months ago

Addressed in PR #257