From acbc8e2a3dd2ed8fb665fdfbfe106f1ecba5f70b Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Mon, 24 Jul 2023 16:18:05 +0200
Subject: [PATCH 01/10] Create conditions with a null payload for missing files
 or conditions

---
 Core/include/Core/ConditionIdentifier.h  |   2 -
 Core/include/Core/ConditionsRepository.h |  22 +++-
 Core/src/ConditionsGitReader.cpp         |  47 +++----
 Core/src/ConditionsLoader.cpp            | 153 +++++++++++++++++------
 Core/src/ConditionsRepository.cpp        |  11 +-
 Core/src/DetectorDataService.cpp         |  13 +-
 6 files changed, 175 insertions(+), 73 deletions(-)

diff --git a/Core/include/Core/ConditionIdentifier.h b/Core/include/Core/ConditionIdentifier.h
index dc5195de9..be653bc71 100644
--- a/Core/include/Core/ConditionIdentifier.h
+++ b/Core/include/Core/ConditionIdentifier.h
@@ -14,8 +14,6 @@
 //==========================================================================
 #pragma once
 
-#include "Core/ConditionsRepository.h"
-
 #include "DD4hep/Conditions.h"
 #include "DD4hep/DetElement.h"
 #include "DDCond/ConditionsContent.h"
diff --git a/Core/include/Core/ConditionsRepository.h b/Core/include/Core/ConditionsRepository.h
index 19b00f8cf..97814a43e 100644
--- a/Core/include/Core/ConditionsRepository.h
+++ b/Core/include/Core/ConditionsRepository.h
@@ -18,9 +18,19 @@
 
 #include "DDCond/ConditionsContent.h"
 
+#include <boost/multi_index/hashed_index.hpp>
+#include <boost/multi_index/key_extractors.hpp>
+#include <boost/multi_index/ordered_index.hpp>
+#include <boost/multi_index_container.hpp>
+
+namespace {
+  namespace bmi = boost::multi_index;
+}
+
 namespace LHCb::Detector {
 
-  class ConditionIdentifier;
+  dd4hep::Condition::key_type location_key( const ConditionIdentifier* entry );
+
   struct RequestSelector;
 
   /**
@@ -32,7 +42,15 @@ namespace LHCb::Detector {
     friend struct RequestSelector;
 
   public:
-    std::map<dd4hep::Condition::key_type, ConditionIdentifier*> locations;
+    using Locations = boost::multi_index_container<
+        ConditionIdentifier*,
+        bmi::indexed_by<
+            bmi::ordered_unique<
+                bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &location_key>>,
+            bmi::hashed_non_unique<bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>,
+            bmi::hashed_unique<
+                bmi::member<ConditionIdentifier, dd4hep::Condition::key_type, &ConditionIdentifier::hash>>>>;
+    Locations locations;
 
   private:
     /// Default assignment operator
diff --git a/Core/src/ConditionsGitReader.cpp b/Core/src/ConditionsGitReader.cpp
index 27a5d0b93..d077f99ce 100644
--- a/Core/src/ConditionsGitReader.cpp
+++ b/Core/src/ConditionsGitReader.cpp
@@ -95,30 +95,33 @@ int LHCb::Detector::ConditionsGitReader::getObject( const std::string& system_id
 
   // Now loading the condition
   // m_condDB->logger()->level = GitCondDB::Logger::Level::Debug;
-  auto [data, iov] = m_condDB->get( {m_dbtag, path, time_point} );
-  if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) {
-    if ( iov.since != time_point ) {
-      throw std::runtime_error(
-          fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) );
+  try {
+    auto [data, iov] = m_condDB->get( {m_dbtag, path, time_point} );
+
+    if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) {
+      if ( iov.since != time_point ) {
+        throw std::runtime_error(
+            fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) );
+      }
+      iov.until = iov.since + 1;
     }
-    iov.until = iov.since + 1;
-  }
 
-  buffer = std::move( data );
-  // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other,
-  // and make sure we do not use the part of uint64_t that long does not cover
-  ct->valid_since = iov.since; // the IOV start is OK by construction
-  if ( iov.until == GitCondDB::CondDB::IOV::max() ) {
-    ct->valid_until = dd4hep::IOV::MAX_KEY;
-  } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) {
-    throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until,
-                                           dd4hep::IOV::MAX_KEY, m_dbtag, path ) );
-  } else {
-    // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV,
-    // while DD4hep includes it so we have to map [100,200) -> [100,199]
-    ct->valid_until = iov.until - 1;
-  }
-  return 1;
+    buffer = std::move( data );
+    // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other,
+    // and make sure we do not use the part of uint64_t that long does not cover
+    ct->valid_since = iov.since; // the IOV start is OK by construction
+    if ( iov.until == GitCondDB::CondDB::IOV::max() ) {
+      ct->valid_until = dd4hep::IOV::MAX_KEY;
+    } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) {
+      throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until,
+                                             dd4hep::IOV::MAX_KEY, m_dbtag, path ) );
+    } else {
+      // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV,
+      // while DD4hep includes it so we have to map [100,200) -> [100,199]
+      ct->valid_until = iov.until - 1;
+    }
+    return 1;
+  } catch ( std::runtime_error const& e ) { return 0; }
 }
 
 /// Resolve a given URI to a string containing the data
diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp
index a82745164..a9dd636a4 100644
--- a/Core/src/ConditionsLoader.cpp
+++ b/Core/src/ConditionsLoader.cpp
@@ -17,6 +17,7 @@
 #include "Core/ConditionsLoader.h"
 #include "Core/ConditionsReaderContext.h"
 #include "Core/ConditionsRepository.h"
+#include "Core/Keys.h"
 
 // Other dd4hep includes
 #include "Core/UpgradeTags.h"
@@ -31,6 +32,7 @@
 #include "TTimeStamp.h"
 
 #include <Core/yaml_converters.h>
+#include <boost/range/iterator_range_core.hpp>
 #include <exception>
 #include <set>
 #include <sstream>
@@ -38,6 +40,24 @@
 #include <utility>
 #include <vector>
 
+namespace {
+  std::tuple<bool, YAML::Node> empty_condition( const LHCb::Detector::ConditionIdentifier* cond ) {
+    const std::array<dd4hep::Condition::key_type, 2> alignment_keys = {LHCb::Detector::Keys::deltaKey,
+                                                                       LHCb::Detector::Keys::alignmentKey};
+
+    dd4hep::ConditionKey::KeyMaker key( cond->hash );
+
+    bool is_alignment = std::any_of( alignment_keys.begin(), alignment_keys.end(),
+                                     [&key]( auto hash ) { return key.values.item_key == hash; } );
+
+    auto const* payload = is_alignment ? "NotFound: !alignment\n  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n  "
+                                         "rotation: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n"
+                                       : "";
+    auto node = YAML::Load( payload );
+    return {is_alignment, is_alignment ? node["NotFound"] : node};
+  }
+} // namespace
+
 /// Standard constructor, initializes variables
 LHCb::Detector::ConditionsLoader::ConditionsLoader( dd4hep::Detector& description, dd4hep::cond::ConditionsManager mgr,
                                                     const std::string& nam )
@@ -120,8 +140,50 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
   // as done so that we can skip the iteration.
   std::map<dd4hep::Condition::key_type, ToDoItem> todo_list{conditions_to_load.begin(), conditions_to_load.end()};
 
+  auto block_register = [this]( dd4hep::IOV::Key iov_key, const std::vector<dd4hep::Condition>& entity_conditions ) {
+    dd4hep::cond::ConditionsPool* pool = manager().registerIOV( *m_iovType, iov_key );
+    return manager().blockRegister( *pool, entity_conditions );
+  };
+
   try {
     for ( auto [current_key, item] : todo_list ) { // O[N]
+
+      auto register_condition = [&loaded, &todo_list, &current_key = current_key,
+                                 &item = item]( std::vector<dd4hep::Condition>& entity_conditions,
+                                                const ConditionIdentifier* cond_ident, std::string cond_name,
+                                                const YAML::Node& cond_data, std::string doc ) {
+        dd4hep::Condition c = LHCb::YAMLConverters::make_condition( cond_name, cond_data );
+        if ( !c.isValid() ) {
+          // Will eventually have to raise an exception
+          dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Failed to load condition %s from %s",
+                            cond_name.c_str(), doc.c_str() );
+          return false;
+        } else {
+          const auto cond_key = cond_ident->hash;
+
+          c->hash = cond_key;
+          // pass the empty condition to the caller
+          loaded[cond_key] = c;
+
+          // record the condition for bulk registration to the manager
+          entity_conditions.push_back( c );
+
+          // if this is the condition we were looking for,
+          // flag the ToDoItem as done and move to the next condition in the YAML object
+          if ( cond_key == current_key ) {
+            item.done = true;
+            return false;
+          }
+          // otherwise see if we are supposed to load it,
+          // in which case the pending work item can be marked as already dine
+          if ( auto cond_itr = todo_list.find( cond_key ); cond_itr != todo_list.end() ) {
+            cond_itr->second.done = true;
+            return false;
+          }
+          return true;
+        }
+      };
+
       const auto cond_id = item.cond_id;
       // FIXME this early skip for conditions already loaded from a URL does not convince me
       if ( item.done ) { continue; }
@@ -131,6 +193,8 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
         continue;
       }
 
+      const auto* repo = cond_id->repository;
+
       // set::insert returns a pair where the second tells if the insertion
       // took place (true) or the entry was already present (false)
       if ( !processed_urls.insert( cond_id->sys_id ).second ) {
@@ -141,13 +205,13 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
       // ready, set, chrono started!
       TTimeStamp start;
       // Load from the git cond db here XXX
-      // FIXME: if the interface make sense, we should do something in case of failure
+      // If the file can't be loaded, create empty conditions for all
+      // of the conditions expected in this file
       if ( m_reader->load( cond_id->sys_id, &ctxt, buffer ) ) {
         auto ext = std::string_view{cond_id->sys_id};
         ext.remove_prefix( ext.find_last_of( '.' ) );
         if ( ext == ".yml" || ext == ".yaml" ) {
           dd4hep::ConditionKey::KeyMaker kmaker( cond_id->sys_id_hash, 0 );
-          const auto*                    repo = cond_id->repository;
 
           // condition YAML files are expected to contain a mapping <name> -> <condition data>
           auto data = YAML::Load( ( m_useOverlay ) ? m_overlay.get_or_default( cond_id->sys_id, buffer ) : buffer );
@@ -164,65 +228,61 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
 
           // prepare the store for the conditions we will register to the conditions manager
           // ... it would be nice if we had an actual type here and not a handle
-          std::vector<dd4hep::Condition> entity_conditons;
-          entity_conditons.reserve( data.size() );
+          std::vector<dd4hep::Condition> entity_conditions;
+          entity_conditions.reserve( data.size() );
 
           // loop over all conditions in the YAML document
           for ( const auto& cond_data : data ) {
             const std::string cond_name = cond_data.first.as<std::string>();
 
             // see if the condition is in the list of locations(?)
+            // Only the lower 32 bits of kmaker.hash have been set
+            // above, set the upper bits here
             kmaker.values.item_key = dd4hep::detail::hash32( cond_name );
-            auto cond_ident        = repo->locations.find( kmaker.hash ); // O[log(N)]
+
+            auto cond_ident = repo->locations.find( kmaker.hash ); // O[log(N)]
             if ( cond_ident == repo->locations.end() ) {
               dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Got stray condition %s from %s",
                                 cond_name.c_str(), cond_id->sys_id.c_str() );
               continue;
             }
 
-            dd4hep::Condition c = LHCb::YAMLConverters::make_condition( cond_name, cond_data.second );
-            if ( !c.isValid() ) {
-              // Will eventually have to raise an exception
-              dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Failed to load condition %s from %s",
-                                cond_name.c_str(), cond_id->sys_id.c_str() );
-              continue;
-            }
+            bool stray =
+                register_condition( entity_conditions, *cond_ident, cond_name, cond_data.second, cond_id->sys_id );
+            if ( !stray ) continue;
 
-            // FIXME: is this not the same as kmaker.hash?
-            const auto cond_key = cond_ident->second->hash;
-            c->hash             = cond_key;
-            // pass the condition to the caller
-            loaded[cond_key] = c;
-            // record the condition for bulk registration to the manager
-            entity_conditons.push_back( c );
-            // if this is the condition we were looking for,
-            // flag the ToDoItem as done and move to the next condition in the YAML object
-            if ( cond_key == current_key ) {
-              item.done = true;
-              continue;
-            }
-            // otherwise see if we are supposed to load it,
-            // in which case the pending work item can be marked as already dine
-            if ( auto cond_itr = todo_list.find( cond_key ); cond_itr != todo_list.end() ) {
-              cond_itr->second.done = true;
-              continue;
-            }
             // this warning means that we found in the YAML file a condition we were not requesting
             // FIXME I do not see why this is a problem
             dd4hep::printout( dd4hep::WARNING, "ConditionsLoader", "++ Got stray but valid condition %s from %s",
                               cond_name.c_str(), cond_id->sys_id.c_str() );
           }
 
-          // now that we converted the whole file we make a pool for the IOV of the YAML file
-          // and we register the conditions we found
+          // Check if there are conditions in this file that have been
+          // requested, but have not yet been loaded. If so, create
+          // empty conditions for those too.
+          for ( const auto* cond_ident :
+                boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) {
+            auto loaded_it = loaded.find( cond_ident->hash );
+            if ( loaded_it == loaded.end() ) {
+              auto [is_alignment, cond] = empty_condition( cond_ident );
+              register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id );
+              dd4hep::printout( dd4hep::DEBUG, "ConditionsLoader", "++ Created empty%s condition %s in %s",
+                                is_alignment ? " alignment" : "", cond_ident->object.c_str(),
+                                cond_ident->sys_id.c_str() );
+            }
+          }
+
+          // now that we converted the whole file and created empty conditions for those that are missing,
+          // we make a pool for the IOV of the YAML file and we register the conditions; the IOV for missing
+          // conditions should be the same as for those that were found, as they may only appear at the end of
+          // the IOV
           // FIXME why one pool per YAML file? should it not be one pool for all conditions? (given an event time)
           dd4hep::IOV::Key iov_key( ctxt.valid_since, ctxt.valid_until );
           // FIXME where does the IOVType comes from? why it is bound the the ConditionsLoader instance?
-          dd4hep::cond::ConditionsPool* pool = manager().registerIOV( *m_iovType, iov_key );
-          size_t                        ret  = manager().blockRegister( *pool, entity_conditons );
+          size_t ret = block_register( iov_key, entity_conditions );
           // FIXME it looks like blockRegister returns the number of conditions registered and we have to check
           //       that it's what we expect, but clearly we do not know what to do with that information
-          if ( ret != entity_conditons.size() ) {
+          if ( ret != entity_conditions.size() ) {
             // Error!
           }
         }
@@ -237,6 +297,27 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
             loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( ctxt.valid_since ).c_str(),
             valid_end.c_str(), stop.AsDouble() - start.AsDouble() );
         loaded_len = loaded.size();
+      } else {
+        // File containing Conditions not found, create empty
+        // conditions for all requested conditions that are in this
+        // file.
+        std::vector<dd4hep::Condition> entity_conditions;
+        entity_conditions.reserve( repo->locations.get<1>().count( cond_id->sys_id_hash ) );
+
+        for ( const auto* cond_ident :
+              boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) {
+          auto [is_alignment, cond] = empty_condition( cond_ident );
+          register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id );
+        }
+
+        // If the file is not in the DB, it's never going to appear,
+        // so register empty conditions with an infinite IOV
+        dd4hep::IOV::Key iov_key( 0, std::numeric_limits<std::int64_t>::max() );
+        block_register( iov_key, entity_conditions );
+
+        dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ Added %4ld null conds for  %-48s to pool [%6s-%6s]",
+                          loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( 0 ).c_str(), "inf   " );
+        loaded_len = loaded.size();
       }
       if ( print_results ) {
         // for each requested key we actually load (not skipped) we print all loaded conditions
diff --git a/Core/src/ConditionsRepository.cpp b/Core/src/ConditionsRepository.cpp
index f5722b7cf..8a61ac149 100644
--- a/Core/src/ConditionsRepository.cpp
+++ b/Core/src/ConditionsRepository.cpp
@@ -18,11 +18,16 @@
 //==========================================================================
 
 // Framework includes
-#include "Core/ConditionIdentifier.h"
+#include "Core/ConditionsRepository.h"
 #include "DDCond/ConditionsContent.h"
 
 #include <iostream>
 
+dd4hep::Condition::key_type LHCb::Detector::location_key( const ConditionIdentifier* entry ) {
+  dd4hep::ConditionKey::KeyMaker loc( entry->sys_id_hash, entry->object_hash );
+  return loc.hash;
+}
+
 // C/C++ include files
 template <>
 std::string dd4hep::cond::ConditionsContent::LoadInfo<LHCb::Detector::ConditionIdentifier>::toString() const {
@@ -48,7 +53,7 @@ LHCb::Detector::ConditionsRepository::addLocation( dd4hep::DetElement de, dd4hep
   auto*                          info = new LoadInfo<ConditionIdentifier>( ident );
   auto                           ret  = addLocationInfo( km.hash, info );
   dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash );
-  locations.insert( std::make_pair( loc.hash, &info->info ) );
+  locations.insert( &info->info );
   return ret;
 }
 
@@ -67,7 +72,7 @@ LHCb::Detector::ConditionsRepository::addGlobal( dd4hep::DetElement de, dd4hep::
   auto*                          info = new LoadInfo<ConditionIdentifier>( ident );
   auto                           ret  = addLocationInfo( km.hash, info );
   dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash );
-  locations.insert( std::make_pair( loc.hash, &info->info ) );
+  locations.insert( &info->info );
   return ret;
 }
 
diff --git a/Core/src/DetectorDataService.cpp b/Core/src/DetectorDataService.cpp
index 6810a69f9..e03881d0b 100644
--- a/Core/src/DetectorDataService.cpp
+++ b/Core/src/DetectorDataService.cpp
@@ -182,11 +182,10 @@ void LHCb::Detector::DetectorDataService::initialize( const nlohmann::json& conf
       auto cond_key =
           LHCb::Detector::ConditionKey( m_description.detector( std::string{deName} ), std::string{condKey} );
       // Check in the conditions mappings which file this corresponds to
-      if ( auto el = std::find_if( begin( m_all_conditions->locations ), end( m_all_conditions->locations ),
-                                   [cond_key]( const auto& item ) { return item.second->hash == cond_key; } );
-           el != end( m_all_conditions->locations ) ) {
+      if ( auto el = m_all_conditions->locations.get<2>().find( cond_key );
+           el != end( m_all_conditions->locations.get<2>() ) ) {
         overrides.emplace_back(
-            LHCb::Detector::ConditionOverrideEntry{el->second->sys_id, el->second->object, YAML::Load( o.second )} );
+            LHCb::Detector::ConditionOverrideEntry{( *el )->sys_id, ( *el )->object, YAML::Load( o.second )} );
       } else {
         throw std::invalid_argument{"cannot find " + std::string{condKey} + " in " + std::string{deName} +
                                     "'s ConditionsRepository"};
@@ -325,10 +324,8 @@ void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetEle
 void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetElement& de, const std::string& cond,
                                                             const YAML::Node& data ) {
   auto key = LHCb::Detector::ConditionKey( de, cond );
-  if ( auto el = std::find_if( begin( m_all_conditions->locations ), end( m_all_conditions->locations ),
-                               [key]( const auto& item ) { return item.second->hash == key; } );
-       el != end( m_all_conditions->locations ) ) {
-    update_condition( el->second->sys_id, el->second->object, data );
+  if ( auto el = m_all_conditions->locations.get<2>().find( key ); el != end( m_all_conditions->locations.get<2>() ) ) {
+    update_condition( ( *el )->sys_id, ( *el )->object, data );
   } else {
     throw std::invalid_argument{"cannot find " + cond + " in " + de.name() + "'s ConditionsRepository"};
   }
-- 
GitLab


From 13849eda82cc4da367779966a485b9206a161279 Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Mon, 24 Jul 2023 16:18:16 +0200
Subject: [PATCH 02/10] Fix compiler warnings

---
 Core/tests/src/test_demuon.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Core/tests/src/test_demuon.cpp b/Core/tests/src/test_demuon.cpp
index e39db717c..8c005d6d6 100644
--- a/Core/tests/src/test_demuon.cpp
+++ b/Core/tests/src/test_demuon.cpp
@@ -114,7 +114,7 @@ void _iterate_muon( const LHCb::Detector::DeMuon& muon ) {
           // test listOfPhysChannels and related code
           const std::array<LHCb::Detector::DeMuonGasGap, 4> test_all_gaps = det_ch.gaps();
           unsigned int                                      i             = 0;
-          double                                            zin, zout;
+          double                                            zin = 0., zout = 0.;
 
           for ( const auto& igap : test_all_gaps ) {
             i++;
@@ -369,7 +369,7 @@ static long test_load_demuon( dd4hep::Detector& description, int argc, char** ar
           // test listOfPhysChannels and related code
           const std::array<LHCb::Detector::DeMuonGasGap, 4> test_all_gaps = det_ch.gaps();
           unsigned int                                      i             = 0;
-          double                                            zin, zout;
+          double                                            zin = 0., zout = 0.;
 
           for ( const auto& igap : test_all_gaps ) {
             i++;
-- 
GitLab


From a1da50fcb2b0ae6f9eb688f79fc831314d70abbd Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Mon, 31 Jul 2023 14:33:00 +0200
Subject: [PATCH 03/10] [ConditionsGitReader] Limit scope to try block to avoid
 catching the wrong exceptions

---
 Core/src/ConditionsGitReader.cpp | 51 +++++++++++++++++---------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/Core/src/ConditionsGitReader.cpp b/Core/src/ConditionsGitReader.cpp
index d077f99ce..996a1241a 100644
--- a/Core/src/ConditionsGitReader.cpp
+++ b/Core/src/ConditionsGitReader.cpp
@@ -95,33 +95,36 @@ int LHCb::Detector::ConditionsGitReader::getObject( const std::string& system_id
 
   // Now loading the condition
   // m_condDB->logger()->level = GitCondDB::Logger::Level::Debug;
+  std::string            data{};
+  GitCondDB::CondDB::IOV iov{};
+
   try {
-    auto [data, iov] = m_condDB->get( {m_dbtag, path, time_point} );
-
-    if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) {
-      if ( iov.since != time_point ) {
-        throw std::runtime_error(
-            fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) );
-      }
-      iov.until = iov.since + 1;
-    }
+    std::tie( data, iov ) = m_condDB->get( {m_dbtag, path, time_point} );
+  } catch ( std::runtime_error const& e ) { return 0; }
 
-    buffer = std::move( data );
-    // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other,
-    // and make sure we do not use the part of uint64_t that long does not cover
-    ct->valid_since = iov.since; // the IOV start is OK by construction
-    if ( iov.until == GitCondDB::CondDB::IOV::max() ) {
-      ct->valid_until = dd4hep::IOV::MAX_KEY;
-    } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) {
-      throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until,
-                                             dd4hep::IOV::MAX_KEY, m_dbtag, path ) );
-    } else {
-      // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV,
-      // while DD4hep includes it so we have to map [100,200) -> [100,199]
-      ct->valid_until = iov.until - 1;
+  if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) {
+    if ( iov.since != time_point ) {
+      throw std::runtime_error(
+          fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) );
     }
-    return 1;
-  } catch ( std::runtime_error const& e ) { return 0; }
+    iov.until = iov.since + 1;
+  }
+
+  buffer = std::move( data );
+  // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other,
+  // and make sure we do not use the part of uint64_t that long does not cover
+  ct->valid_since = iov.since; // the IOV start is OK by construction
+  if ( iov.until == GitCondDB::CondDB::IOV::max() ) {
+    ct->valid_until = dd4hep::IOV::MAX_KEY;
+  } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) {
+    throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until,
+                                           dd4hep::IOV::MAX_KEY, m_dbtag, path ) );
+  } else {
+    // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV,
+    // while DD4hep includes it so we have to map [100,200) -> [100,199]
+    ct->valid_until = iov.until - 1;
+  }
+  return 1;
 }
 
 /// Resolve a given URI to a string containing the data
-- 
GitLab


From 91d6438da29ff4b2b7cd25ea0dfd0caa647c4b8f Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Mon, 31 Jul 2023 15:36:34 +0200
Subject: [PATCH 04/10] [ConditionsLoader] Print a warning in case a default
 alignment condition is created

---
 Core/src/ConditionsLoader.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp
index a9dd636a4..2014ef685 100644
--- a/Core/src/ConditionsLoader.cpp
+++ b/Core/src/ConditionsLoader.cpp
@@ -266,9 +266,9 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
             if ( loaded_it == loaded.end() ) {
               auto [is_alignment, cond] = empty_condition( cond_ident );
               register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id );
-              dd4hep::printout( dd4hep::DEBUG, "ConditionsLoader", "++ Created empty%s condition %s in %s",
-                                is_alignment ? " alignment" : "", cond_ident->object.c_str(),
-                                cond_ident->sys_id.c_str() );
+              dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::DEBUG, "ConditionsLoader",
+                                "++ Created empty%s condition %s in %s", is_alignment ? " alignment" : "",
+                                cond_ident->object.c_str(), cond_ident->sys_id.c_str() );
             }
           }
 
-- 
GitLab


From 43413a41d8655b173d6575bf467ee0cdc1cd77b9 Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Tue, 1 Aug 2023 13:43:26 +0200
Subject: [PATCH 05/10] [DeLHCb] Default conditions allow removal special
 constructor arguments

---
 Core/src/DetectorDataService.cpp             |  1 -
 Core/tests/src/test_scope.cpp                |  2 +-
 Detector/LHCb/include/Detector/LHCb/DeLHCb.h |  2 +-
 Detector/LHCb/src/DeLHCb.cpp                 | 22 ++++++++------------
 4 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/Core/src/DetectorDataService.cpp b/Core/src/DetectorDataService.cpp
index e03881d0b..b2f39ffd2 100644
--- a/Core/src/DetectorDataService.cpp
+++ b/Core/src/DetectorDataService.cpp
@@ -133,7 +133,6 @@ void LHCb::Detector::DetectorDataService::initialize( const nlohmann::json& conf
   // make sure the detector_names option is in sync
   auto& opts             = *m_description.extension<Options>();
   opts["detector_names"] = m_detectorNames;
-  if ( config.contains( "without_tell40links" ) ) { opts["without_tell40links"] = config.at( "without_tell40links" ); }
 
   // Now setup DeLHCb
   // BEWARE: HAS TO BE DONE BEFORE THE NEXT STEP THAT GATHERS ALL THE CONDITIONS
diff --git a/Core/tests/src/test_scope.cpp b/Core/tests/src/test_scope.cpp
index 2bcfd626f..d576b59d6 100644
--- a/Core/tests/src/test_scope.cpp
+++ b/Core/tests/src/test_scope.cpp
@@ -32,7 +32,7 @@ TEST_CASE( "scope_test" ) {
   description.fromXML( compact_path );
   std::vector<std::string>            detector_list{"/world", "scope"};
   LHCb::Detector::DetectorDataService dds( description, detector_list );
-  dds.initialize( nlohmann::json{{"repository", conditions_path}, {"without_tell40links", true}} );
+  dds.initialize( nlohmann::json{{"repository", conditions_path}} );
 
   auto                        slice = dds.get_slice( 100 );
   dd4hep::DetElement          det   = description.detector( "/world/scope/side1/sensor0" );
diff --git a/Detector/LHCb/include/Detector/LHCb/DeLHCb.h b/Detector/LHCb/include/Detector/LHCb/DeLHCb.h
index 25a9f0756..e5d9c1b7e 100644
--- a/Detector/LHCb/include/Detector/LHCb/DeLHCb.h
+++ b/Detector/LHCb/include/Detector/LHCb/DeLHCb.h
@@ -24,7 +24,7 @@ namespace LHCb::Detector {
   namespace detail {
 
     struct DeLHCbObject : DeIOVObject {
-      DeLHCbObject( const dd4hep::DetElement& de, dd4hep::cond::ConditionUpdateContext& ctxt, bool withTell40Links );
+      DeLHCbObject( const dd4hep::DetElement& de, dd4hep::cond::ConditionUpdateContext& ctxt );
       void               applyToAllChildren( const std::function<void( DeIOVElement<DeIOVObject> )>& ) const override;
       std::vector<DeIOV> children;
       Tell40Links        m_tell40links;
diff --git a/Detector/LHCb/src/DeLHCb.cpp b/Detector/LHCb/src/DeLHCb.cpp
index 83700a2c6..8fdd9390c 100644
--- a/Detector/LHCb/src/DeLHCb.cpp
+++ b/Detector/LHCb/src/DeLHCb.cpp
@@ -18,9 +18,11 @@
 #include <stdexcept>
 
 LHCb::Detector::detail::DeLHCbObject::DeLHCbObject( const dd4hep::DetElement&             de,
-                                                    dd4hep::cond::ConditionUpdateContext& ctxt, bool withTell40Links )
+                                                    dd4hep::cond::ConditionUpdateContext& ctxt )
     : DeIOVObject( de, ctxt, 9000000, false ) {
-  if ( withTell40Links ) { m_tell40links = ctxt.condition( hash_key( de, "Tell40Links" ) ).get<nlohmann::json>(); }
+
+  auto links = ctxt.condition( hash_key( de, "Tell40Links" ) ).get<nlohmann::json>();
+  if ( !links.is_null() ) { m_tell40links = links; }
 }
 
 void LHCb::Detector::detail::DeLHCbObject::applyToAllChildren(
@@ -45,15 +47,13 @@ namespace {
     std::vector<std::string> children_names;
     // Contains the DeIOVs for all the children
 
-    bool withTell40Links{true};
-
     virtual dd4hep::Condition operator()( const dd4hep::ConditionKey&           key,
                                           dd4hep::cond::ConditionUpdateContext& context ) override final {
       if ( key.item_key() == LHCb::Detector::Keys::deKey ) {
         // This is the pointer we need to return at the end of the callback
         auto lhcbdet = dd4hep::Detector::getInstance().world();
         // Creating the Detector element with the desired field
-        auto delhcbobj = new LHCb::Detector::detail::DeLHCbObject( lhcbdet, context, withTell40Links );
+        auto delhcbobj = new LHCb::Detector::detail::DeLHCbObject( lhcbdet, context );
         // Now looking up the children DeIOVs
         for ( const auto& name : children_names ) {
           LHCb::Detector::DeIOV deiov = LHCb::Detector::detail::get_DeIOV_from_context( context, name );
@@ -96,14 +96,10 @@ void LHCb::Detector::setup_DeLHCb_callback( dd4hep::Detector& description ) {
     depbuilder.add( hash_key( description.detector( name ), LHCb::Detector::Keys::deKey ) );
   }
 
-  iovUpdate->withTell40Links =
-      opts && !( opts->contains( "without_tell40links" ) && opts->at( "without_tell40links" ).get<bool>() );
-  if ( iovUpdate->withTell40Links ) {
-    ( *requests )
-        ->addLocation( de, LHCb::Detector::item_key( "Tell40Links" ), "Conditions/LHCb/Online/Tell40Links.yml",
-                       "Tell40Links" );
-    depbuilder.add( hash_key( de, "Tell40Links" ) );
-  }
+  ( *requests )
+      ->addLocation( de, LHCb::Detector::item_key( "Tell40Links" ), "Conditions/LHCb/Online/Tell40Links.yml",
+                     "Tell40Links" );
+  depbuilder.add( hash_key( de, "Tell40Links" ) );
 
   ( *requests )->addDependency( depbuilder.release() );
 }
-- 
GitLab


From 081ac15bcd92a6045d939a4b94381924fc1df569 Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Tue, 1 Aug 2023 13:45:37 +0200
Subject: [PATCH 06/10] Test default alignment conditions by removing some from
 the test conditions. Capture and analyse DD4hep printout in a test that uses
 these conditions.

---
 CMakeLists.txt                                |  2 +-
 Core/tests/src/test_DetectorDataService.cpp   | 33 ++++++++++++
 TestUtils/CMakeLists.txt                      | 12 +++--
 TestUtils/include/Detector/Test/Fixture.h     | 16 +++++-
 TestUtils/src/Fixture.cpp                     | 50 +++++++++++++++++++
 .../Conditions/VP/Alignment/Global.yml/v1     |  4 --
 .../Conditions/VP/Alignment/Global.yml/v2     |  2 -
 .../ConditionsIOV/Conditions/VP/Motion.yml/0  |  4 --
 .../Conditions/VP/Motion.yml/400              |  4 --
 9 files changed, 105 insertions(+), 22 deletions(-)
 create mode 100644 TestUtils/src/Fixture.cpp

diff --git a/CMakeLists.txt b/CMakeLists.txt
index cd21b78eb..a3f05fb15 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -243,7 +243,7 @@ add_detector_plugin(DetectorTestPlugins
     Detector/TestScope/scope_cond.cpp
     Core/tests/src/test_load_scope.cpp
 )
-target_link_libraries(DetectorTestPlugins GitCondDB::GitCondDB ${FS_LIB} DetectorLib)
+target_link_libraries(DetectorTestPlugins GitCondDB::GitCondDB TestUtils ${FS_LIB} DetectorLib)
 
 
 #--------------------------------------------------------------------------
diff --git a/Core/tests/src/test_DetectorDataService.cpp b/Core/tests/src/test_DetectorDataService.cpp
index 088614b69..c24df16b1 100644
--- a/Core/tests/src/test_DetectorDataService.cpp
+++ b/Core/tests/src/test_DetectorDataService.cpp
@@ -19,13 +19,26 @@
 
 #include "Detector/Magnet/DeMagnet.h"
 #include "Detector/VP/DeVP.h"
+#include <Detector/Test/Fixture.h>
 
 #include <cstring>
 #include <exception>
 #include <nlohmann/json.hpp>
+#include <regex>
 #include <string>
 #include <vector>
 
+#include <range/v3/action/split.hpp>
+#include <range/v3/range/primitives.hpp>
+#include <range/v3/view/all.hpp>
+#include <range/v3/view/c_str.hpp>
+#include <range/v3/view/filter.hpp>
+
+namespace {
+  namespace ra = ranges::actions;
+  namespace rv = ranges::views;
+} // namespace
+
 static long test_DetectorDataService( dd4hep::Detector& description, int argc, char** argv ) {
 
   bool        help = false;
@@ -442,11 +455,31 @@ static long test_VP_motion( dd4hep::Detector& description, int argc, char** argv
     return position;
   };
 
+  // Use a fixture to capture DD4hep output so we can check if
+  // warnings about missing alignment conditions were printed
+  Detector::Test::Fixture f{true};
+
   // Getting the first position where motion should be 0
   auto position100 = get_vpleft_pos( 100 );
+
   std::cout << "run 100 - VP Left position: " << position100 << '\n';
 
+  auto dd4hep_output = f.printout.str();
+
+  // Check if warnings were printed about missing alignment conditions
+  auto        lines = ra::split( dd4hep_output, ranges::views::c_str( "\n" ) );
+  std::regex  missing_align{R"del(WARN\s+.+empty alignment condition)del"};
+  std::smatch result;
+  auto        warnings = lines | rv::filter( [&missing_align, &result]( auto line ) {
+                    return std::regex_search( line, result, missing_align );
+                  } );
+  if ( ranges::empty( warnings ) ) {
+    std::cout << "No warnings printed about missing alignment conditions; expected 2" << std::endl;
+    ::exit( EINVAL );
+  }
+
   auto position1000 = get_vpleft_pos( 1000 );
+
   std::cout << "run 1000 - VP Left position: " << position1000 << '\n';
 
   auto diff = position1000 - position100;
diff --git a/TestUtils/CMakeLists.txt b/TestUtils/CMakeLists.txt
index 55d358442..9ba5119de 100644
--- a/TestUtils/CMakeLists.txt
+++ b/TestUtils/CMakeLists.txt
@@ -10,10 +10,12 @@
 ###############################################################################
 
 # Library with helper classes to simplify writing tests
-add_library(TestUtils INTERFACE)
-target_include_directories(TestUtils INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/include)
+add_library(TestUtils SHARED src/Fixture.cpp)
+target_include_directories(TestUtils PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include)
 target_link_libraries(TestUtils
-    INTERFACE
-        ROOT::Core
-        DD4hep::DDCore
+  PRIVATE
+    fmt::fmt
+  PUBLIC
+    ROOT::Core
+    DD4hep::DDCore
 )
diff --git a/TestUtils/include/Detector/Test/Fixture.h b/TestUtils/include/Detector/Test/Fixture.h
index ba629a7f0..f2a7ab498 100644
--- a/TestUtils/include/Detector/Test/Fixture.h
+++ b/TestUtils/include/Detector/Test/Fixture.h
@@ -17,11 +17,18 @@
 #include <string_view>
 
 namespace Detector::Test {
+
+  size_t test_printer( void* fixture, dd4hep::PrintLevel, const char* src, const char* text );
+
   struct Fixture {
     // Instantiate the global dd4hep instance and silence printouts
-    Fixture() : originalLevels{gErrorIgnoreLevel, dd4hep::printLevel()} {
+    Fixture( bool capture_printout = false ) : originalLevels{gErrorIgnoreLevel, dd4hep::printLevel()} {
+
       auto keep_output = std::getenv( "KEEP_OUTPUT" );
-      if ( !keep_output || std::string_view( keep_output ) == "" ) {
+
+      if ( capture_printout ) {
+        dd4hep::setPrinter( this, &test_printer );
+      } else if ( !keep_output || std::string_view( keep_output ) == "" ) {
         // Set the error level to ignore all messages
         // - from DD4hep
         dd4hep::setPrintLevel( static_cast<dd4hep::PrintLevel>( dd4hep::ALWAYS + 1 ) );
@@ -35,6 +42,8 @@ namespace Detector::Test {
       // Restore the error level
       gErrorIgnoreLevel = originalLevels.gErrorIgnoreLevel;
       dd4hep::setPrintLevel( originalLevels.printLevel );
+      // Restore the default printer
+      dd4hep::setPrinter( nullptr, nullptr );
       // destroy the global dd4hep instance
       dd4hep::Detector::destroyInstance();
     }
@@ -44,6 +53,9 @@ namespace Detector::Test {
       dd4hep::PrintLevel printLevel;
     } originalLevels;
 
+    std::mutex        output_mutex;
+    std::stringstream printout;
+
     dd4hep::Detector* descriptionPtr{nullptr};
 
     // helpers to access the detector description
diff --git a/TestUtils/src/Fixture.cpp b/TestUtils/src/Fixture.cpp
new file mode 100644
index 000000000..f2b84d2c9
--- /dev/null
+++ b/TestUtils/src/Fixture.cpp
@@ -0,0 +1,50 @@
+/*****************************************************************************\
+* (c) Copyright 2022 CERN for the benefit of the LHCb Collaboration           *
+*                                                                             *
+* This software is distributed under the terms of the GNU General Public      *
+* Licence version 3 (GPL Version 3), copied verbatim in the file "COPYING".   *
+*                                                                             *
+* In applying this licence, CERN does not waive the privileges and immunities *
+* granted to it by virtue of its status as an Intergovernmental Organization  *
+* or submit itself to any jurisdiction.                                       *
+\*****************************************************************************/
+#include <Detector/Test/Fixture.h>
+#include <fmt/format.h>
+
+namespace {
+
+  // Copied from DD4hep, because it's not available outside it...
+  const char* print_level( dd4hep::PrintLevel lvl ) {
+    switch ( lvl ) {
+    case dd4hep::NOLOG:
+      return "NOLOG";
+    case dd4hep::VERBOSE:
+      return "VERB ";
+    case dd4hep::DEBUG:
+      return "DEBUG";
+    case dd4hep::INFO:
+      return "INFO ";
+    case dd4hep::WARNING:
+      return "WARN ";
+    case dd4hep::ERROR:
+      return "ERROR";
+    case dd4hep::FATAL:
+      return "FATAL";
+    case dd4hep::ALWAYS:
+      return "     ";
+    default:
+      if ( lvl > dd4hep::ALWAYS ) return print_level( dd4hep::ALWAYS );
+      return print_level( dd4hep::NOLOG );
+    }
+  }
+} // namespace
+
+size_t Detector::Test::test_printer( void* fixture, dd4hep::PrintLevel lvl, const char* src, const char* text ) {
+  Fixture*                    f = reinterpret_cast<Fixture*>( fixture );
+  std::lock_guard<std::mutex> lock( f->output_mutex );
+  // std::string s = std::string{src} + " " + text;
+  std::string s = fmt::format( "{:-<16} {} {}", src, print_level( lvl ), text );
+  f->printout << s << "\n";
+  std::cout << s << "\n";
+  return s.size() + 1;
+}
diff --git a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1 b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1
index 32c5f2898..515d5dbe9 100644
--- a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1
+++ b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1
@@ -1,6 +1,2 @@
 VPSystem: !alignment
   position: [1.0 * mm, 0.1 * cm, 1000 * um]
-VPLeft: !alignment
-  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]
-VPRight: !alignment
-  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]
diff --git a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2 b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2
index fcbf9690f..287dec0b3 100644
--- a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2
+++ b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2
@@ -1,6 +1,4 @@
 VPSystem: !alignment
   position: [1.0 * mm, 1.0 * mm, 1.0 * mm]
-VPLeft: !alignment
-  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]
 VPRight: !alignment
   position: [5.0 * mm, 5.0 * mm, 5.0 * mm]
diff --git a/tests/ConditionsIOV/Conditions/VP/Motion.yml/0 b/tests/ConditionsIOV/Conditions/VP/Motion.yml/0
index 8d48475d2..e69de29bb 100644
--- a/tests/ConditionsIOV/Conditions/VP/Motion.yml/0
+++ b/tests/ConditionsIOV/Conditions/VP/Motion.yml/0
@@ -1,4 +0,0 @@
-MotionVPLeft: !alignment
-  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]
-MotionVPRight: !alignment
-  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]
diff --git a/tests/ConditionsIOV/Conditions/VP/Motion.yml/400 b/tests/ConditionsIOV/Conditions/VP/Motion.yml/400
index 8d48475d2..e69de29bb 100644
--- a/tests/ConditionsIOV/Conditions/VP/Motion.yml/400
+++ b/tests/ConditionsIOV/Conditions/VP/Motion.yml/400
@@ -1,4 +0,0 @@
-MotionVPLeft: !alignment
-  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]
-MotionVPRight: !alignment
-  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]
-- 
GitLab


From 3c003d1e6a4b0e67213725223ee81fa2c6e53fe9 Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Wed, 2 Aug 2023 14:30:41 +0200
Subject: [PATCH 07/10] Use tags to identify indices of locations stored in the
 conditions repository.

---
 Core/include/Core/ConditionsRepository.h | 21 ++++++++++++-
 Core/src/ConditionsLoader.cpp            | 13 ++++----
 Core/src/ConditionsRepository.cpp        | 38 +++++++++++-------------
 Core/src/DetectorDataService.cpp         |  9 +++---
 4 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/Core/include/Core/ConditionsRepository.h b/Core/include/Core/ConditionsRepository.h
index 97814a43e..2f5c36071 100644
--- a/Core/include/Core/ConditionsRepository.h
+++ b/Core/include/Core/ConditionsRepository.h
@@ -29,6 +29,14 @@ namespace {
 
 namespace LHCb::Detector {
 
+  // Tag structs to refer to the three indices of the
+  // ConditionsRepository::Locations multi-index container
+  struct by_location_hash {};
+  struct by_filename_hash {};
+  struct by_condition_key {};
+
+  /// Utility function to extract the combined location hash (filename
+  /// and condition name) from a ConditionIdentifier
   dd4hep::Condition::key_type location_key( const ConditionIdentifier* entry );
 
   struct RequestSelector;
@@ -42,13 +50,24 @@ namespace LHCb::Detector {
     friend struct RequestSelector;
 
   public:
+    // A multi-index container that allows (fast) lookup in three
+    // ways. the value is always the ConditionIdentifier.
     using Locations = boost::multi_index_container<
         ConditionIdentifier*,
         bmi::indexed_by<
+            // Analogously to an `std::map` with key the combined hash
+            // of the filename and the condition name
             bmi::ordered_unique<
+                bmi::tag<LHCb::Detector::by_location_hash>,
                 bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &location_key>>,
-            bmi::hashed_non_unique<bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>,
+            // Analogously to an `std::unordered_multimap` with key the hash of the filename
+            bmi::hashed_non_unique<bmi::tag<LHCb::Detector::by_filename_hash>,
+                                   bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>,
+            // Analogously to an `std::unordered_map` with key the
+            // combined hash of the detector element and the condition
+            // name
             bmi::hashed_unique<
+                bmi::tag<LHCb::Detector::by_condition_key>,
                 bmi::member<ConditionIdentifier, dd4hep::Condition::key_type, &ConditionIdentifier::hash>>>>;
     Locations locations;
 
diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp
index 2014ef685..c651b522c 100644
--- a/Core/src/ConditionsLoader.cpp
+++ b/Core/src/ConditionsLoader.cpp
@@ -236,8 +236,8 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
             const std::string cond_name = cond_data.first.as<std::string>();
 
             // see if the condition is in the list of locations(?)
-            // Only the lower 32 bits of kmaker.hash have been set
-            // above, set the upper bits here
+            // Only the upper 32 bits of kmaker.hash have been set
+            // above, set the lower bits here
             kmaker.values.item_key = dd4hep::detail::hash32( cond_name );
 
             auto cond_ident = repo->locations.find( kmaker.hash ); // O[log(N)]
@@ -260,8 +260,8 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
           // Check if there are conditions in this file that have been
           // requested, but have not yet been loaded. If so, create
           // empty conditions for those too.
-          for ( const auto* cond_ident :
-                boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) {
+          for ( const auto* cond_ident : boost::make_iterator_range(
+                    repo->locations.get<by_filename_hash>().equal_range( cond_id->sys_id_hash ) ) ) {
             auto loaded_it = loaded.find( cond_ident->hash );
             if ( loaded_it == loaded.end() ) {
               auto [is_alignment, cond] = empty_condition( cond_ident );
@@ -302,10 +302,11 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
         // conditions for all requested conditions that are in this
         // file.
         std::vector<dd4hep::Condition> entity_conditions;
-        entity_conditions.reserve( repo->locations.get<1>().count( cond_id->sys_id_hash ) );
+        auto const&                    conditions_by_filename_hash = repo->locations.get<by_filename_hash>();
+        entity_conditions.reserve( conditions_by_filename_hash.count( cond_id->sys_id_hash ) );
 
         for ( const auto* cond_ident :
-              boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) {
+              boost::make_iterator_range( conditions_by_filename_hash.equal_range( cond_id->sys_id_hash ) ) ) {
           auto [is_alignment, cond] = empty_condition( cond_ident );
           register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id );
         }
diff --git a/Core/src/ConditionsRepository.cpp b/Core/src/ConditionsRepository.cpp
index 8a61ac149..6e06712d3 100644
--- a/Core/src/ConditionsRepository.cpp
+++ b/Core/src/ConditionsRepository.cpp
@@ -43,16 +43,15 @@ LHCb::Detector::ConditionsRepository::addLocation( dd4hep::DetElement de, dd4hep
                                                    const std::string& sys_id, const std::string& object ) {
   ConditionIdentifier            ident;
   dd4hep::ConditionKey::KeyMaker km( de.key(), item );
-  ident.hash                          = km.hash;
-  ident.detector                      = de;
-  ident.object                        = object;
-  ident.sys_id                        = sys_id;
-  ident.sys_id_hash                   = dd4hep::detail::hash32( sys_id );
-  ident.object_hash                   = dd4hep::detail::hash32( object );
-  ident.repository                    = this;
-  auto*                          info = new LoadInfo<ConditionIdentifier>( ident );
-  auto                           ret  = addLocationInfo( km.hash, info );
-  dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash );
+  ident.hash        = km.hash;
+  ident.detector    = de;
+  ident.object      = object;
+  ident.sys_id      = sys_id;
+  ident.sys_id_hash = dd4hep::detail::hash32( sys_id );
+  ident.object_hash = dd4hep::detail::hash32( object );
+  ident.repository  = this;
+  auto* info        = new LoadInfo<ConditionIdentifier>( ident );
+  auto  ret         = addLocationInfo( km.hash, info );
   locations.insert( &info->info );
   return ret;
 }
@@ -62,16 +61,15 @@ LHCb::Detector::ConditionsRepository::addGlobal( dd4hep::DetElement de, dd4hep::
                                                  const std::string& sys_id, const std::string& object ) {
   ConditionIdentifier            ident;
   dd4hep::ConditionKey::KeyMaker km( 0, item );
-  ident.hash                          = km.hash;
-  ident.detector                      = de;
-  ident.object                        = object;
-  ident.sys_id                        = sys_id;
-  ident.sys_id_hash                   = dd4hep::detail::hash32( sys_id );
-  ident.object_hash                   = dd4hep::detail::hash32( object );
-  ident.repository                    = this;
-  auto*                          info = new LoadInfo<ConditionIdentifier>( ident );
-  auto                           ret  = addLocationInfo( km.hash, info );
-  dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash );
+  ident.hash        = km.hash;
+  ident.detector    = de;
+  ident.object      = object;
+  ident.sys_id      = sys_id;
+  ident.sys_id_hash = dd4hep::detail::hash32( sys_id );
+  ident.object_hash = dd4hep::detail::hash32( object );
+  ident.repository  = this;
+  auto* info        = new LoadInfo<ConditionIdentifier>( ident );
+  auto  ret         = addLocationInfo( km.hash, info );
   locations.insert( &info->info );
   return ret;
 }
diff --git a/Core/src/DetectorDataService.cpp b/Core/src/DetectorDataService.cpp
index b2f39ffd2..3b308f648 100644
--- a/Core/src/DetectorDataService.cpp
+++ b/Core/src/DetectorDataService.cpp
@@ -181,8 +181,8 @@ void LHCb::Detector::DetectorDataService::initialize( const nlohmann::json& conf
       auto cond_key =
           LHCb::Detector::ConditionKey( m_description.detector( std::string{deName} ), std::string{condKey} );
       // Check in the conditions mappings which file this corresponds to
-      if ( auto el = m_all_conditions->locations.get<2>().find( cond_key );
-           el != end( m_all_conditions->locations.get<2>() ) ) {
+      const auto& conditions_by_key = m_all_conditions->locations.get<by_condition_key>();
+      if ( auto el = conditions_by_key.find( cond_key ); el != end( conditions_by_key ) ) {
         overrides.emplace_back(
             LHCb::Detector::ConditionOverrideEntry{( *el )->sys_id, ( *el )->object, YAML::Load( o.second )} );
       } else {
@@ -322,8 +322,9 @@ void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetEle
 
 void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetElement& de, const std::string& cond,
                                                             const YAML::Node& data ) {
-  auto key = LHCb::Detector::ConditionKey( de, cond );
-  if ( auto el = m_all_conditions->locations.get<2>().find( key ); el != end( m_all_conditions->locations.get<2>() ) ) {
+  auto        key               = LHCb::Detector::ConditionKey( de, cond );
+  const auto& conditions_by_key = m_all_conditions->locations.get<by_condition_key>();
+  if ( auto el = conditions_by_key.find( key ); el != end( conditions_by_key ) ) {
     update_condition( ( *el )->sys_id, ( *el )->object, data );
   } else {
     throw std::invalid_argument{"cannot find " + cond + " in " + de.name() + "'s ConditionsRepository"};
-- 
GitLab


From b444c12a886b8f5aaa63e670b257b7863230b8b5 Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Wed, 2 Aug 2023 14:31:32 +0200
Subject: [PATCH 08/10] Print info message instead of debug when creating a
 missing classical condition.

---
 Core/src/ConditionsLoader.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp
index c651b522c..c46a13814 100644
--- a/Core/src/ConditionsLoader.cpp
+++ b/Core/src/ConditionsLoader.cpp
@@ -266,7 +266,7 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
             if ( loaded_it == loaded.end() ) {
               auto [is_alignment, cond] = empty_condition( cond_ident );
               register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id );
-              dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::DEBUG, "ConditionsLoader",
+              dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::INFO, "ConditionsLoader",
                                 "++ Created empty%s condition %s in %s", is_alignment ? " alignment" : "",
                                 cond_ident->object.c_str(), cond_ident->sys_id.c_str() );
             }
-- 
GitLab


From 3b0423e60793da5ffccab3db80078a3cc16b9f9c Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Wed, 2 Aug 2023 15:06:43 +0200
Subject: [PATCH 09/10] Use rad instead of mm for rotations

---
 Core/src/ConditionsLoader.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp
index c46a13814..93dcb85ff 100644
--- a/Core/src/ConditionsLoader.cpp
+++ b/Core/src/ConditionsLoader.cpp
@@ -51,7 +51,7 @@ namespace {
                                      [&key]( auto hash ) { return key.values.item_key == hash; } );
 
     auto const* payload = is_alignment ? "NotFound: !alignment\n  position: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n  "
-                                         "rotation: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n"
+                                         "rotation: [0.0 * rad, 0.0 * rad, 0.0 * rad]\n"
                                        : "";
     auto node = YAML::Load( payload );
     return {is_alignment, is_alignment ? node["NotFound"] : node};
-- 
GitLab


From 2f9c575412398aaecd8d2b1358e9b871815e6ec9 Mon Sep 17 00:00:00 2001
From: Roel Aaij <roel.aaij@cern.ch>
Date: Thu, 3 Aug 2023 12:51:39 +0200
Subject: [PATCH 10/10] [ConditionsLoader] loop over filenames instead of
 conditions

---
 Core/include/Core/ConditionsRepository.h |  25 ++--
 Core/src/ConditionsLoader.cpp            | 140 ++++++++---------------
 2 files changed, 67 insertions(+), 98 deletions(-)

diff --git a/Core/include/Core/ConditionsRepository.h b/Core/include/Core/ConditionsRepository.h
index 2f5c36071..87c6e28b2 100644
--- a/Core/include/Core/ConditionsRepository.h
+++ b/Core/include/Core/ConditionsRepository.h
@@ -20,6 +20,7 @@
 
 #include <boost/multi_index/hashed_index.hpp>
 #include <boost/multi_index/key_extractors.hpp>
+#include <boost/multi_index/member.hpp>
 #include <boost/multi_index/ordered_index.hpp>
 #include <boost/multi_index_container.hpp>
 
@@ -32,13 +33,19 @@ namespace LHCb::Detector {
   // Tag structs to refer to the three indices of the
   // ConditionsRepository::Locations multi-index container
   struct by_location_hash {};
-  struct by_filename_hash {};
+  struct by_filename {};
   struct by_condition_key {};
 
   /// Utility function to extract the combined location hash (filename
   /// and condition name) from a ConditionIdentifier
   dd4hep::Condition::key_type location_key( const ConditionIdentifier* entry );
 
+  /// Utility function to extract the filename hash from a const ConditionIdentifier
+  inline std::string condition_filename( const ConditionIdentifier* entry ) { return entry->sys_id; };
+
+  /// Utility function to extract the condition key from a const ConditionIdentifier
+  inline dd4hep::Condition::key_type condition_key( const ConditionIdentifier* entry ) { return entry->hash; };
+
   struct RequestSelector;
 
   /**
@@ -51,25 +58,27 @@ namespace LHCb::Detector {
 
   public:
     // A multi-index container that allows (fast) lookup in three
-    // ways. the value is always the ConditionIdentifier.
+    // ways. the value is either ConditionIdentifier* or
+    // const ConditionIdentifier*.
+    template <bool const_pointer = false>
     using Locations = boost::multi_index_container<
-        ConditionIdentifier*,
+        std::conditional_t<const_pointer, const ConditionIdentifier*, ConditionIdentifier*>,
         bmi::indexed_by<
             // Analogously to an `std::map` with key the combined hash
             // of the filename and the condition name
             bmi::ordered_unique<
                 bmi::tag<LHCb::Detector::by_location_hash>,
                 bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &location_key>>,
-            // Analogously to an `std::unordered_multimap` with key the hash of the filename
-            bmi::hashed_non_unique<bmi::tag<LHCb::Detector::by_filename_hash>,
-                                   bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>,
+            // Analogously to an `std::multimap` with key the hash of the filename
+            bmi::ordered_non_unique<bmi::tag<LHCb::Detector::by_filename>,
+                                    bmi::global_fun<const ConditionIdentifier*, std::string, &condition_filename>>,
             // Analogously to an `std::unordered_map` with key the
             // combined hash of the detector element and the condition
             // name
             bmi::hashed_unique<
                 bmi::tag<LHCb::Detector::by_condition_key>,
-                bmi::member<ConditionIdentifier, dd4hep::Condition::key_type, &ConditionIdentifier::hash>>>>;
-    Locations locations;
+                bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &condition_key>>>>;
+    Locations<> locations;
 
   private:
     /// Default assignment operator
diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp
index 93dcb85ff..de17b0d59 100644
--- a/Core/src/ConditionsLoader.cpp
+++ b/Core/src/ConditionsLoader.cpp
@@ -40,6 +40,10 @@
 #include <utility>
 #include <vector>
 
+#include <range/v3/view/all.hpp>
+#include <range/v3/view/transform.hpp>
+#include <range/v3/view/unique.hpp>
+
 namespace {
   std::tuple<bool, YAML::Node> empty_condition( const LHCb::Detector::ConditionIdentifier* cond ) {
     const std::array<dd4hep::Condition::key_type, 2> alignment_keys = {LHCb::Detector::Keys::deltaKey,
@@ -56,6 +60,9 @@ namespace {
     auto node = YAML::Load( payload );
     return {is_alignment, is_alignment ? node["NotFound"] : node};
   }
+
+  namespace rv = ranges::views;
+  namespace ra = ranges::actions;
 } // namespace
 
 /// Standard constructor, initializes variables
@@ -110,13 +117,6 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
   // via "SYSTEM" keyword (needed only for XML parsing but done here instead of inside the loop)
   dd4hep::xml::UriContextReader xml_entity_resolver( m_reader.get(), &ctxt );
 
-  // First collect all required URIs which need loading.
-  // Since one file contains many conditions, we have
-  // to create a unique set
-
-  // container to keep track of the URLs already visited
-  std::set<std::string_view> processed_urls;
-
   // FIXME using a stable buffer for storing the condition docs to be parsed might look like an optimization,
   // but in the case of GitCondDB (or the ConditionsFileReader) it does not really help as each request
   // creates a new string that then has to be moved into this buffer
@@ -126,19 +126,22 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
   // dd4hep::setPrintLevel( dd4hep::DEBUG );
 
   // I have the impression ConditionsIdentifier is not a very good name, but I stick by it for the moment
-  struct ToDoItem {
-    ToDoItem( const dd4hep::cond::ConditionsLoadInfo* load_info )
-        : cond_id{reinterpret_cast<const ConditionIdentifier*>( load_info->ptr() )} {}
-    bool                       done    = false;
-    const ConditionIdentifier* cond_id = nullptr;
-  };
+  ConditionsRepository::Locations<true> all_conditions{};
+  std::for_each( conditions_to_load.begin(), conditions_to_load.end(), [&all_conditions]( auto entry ) {
+    auto* load_info = entry.second->ptr();
+    if ( !load_info ) {
+      dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ CANNOT update condition: [%16llX] [No load info]",
+                        entry.first );
+    } else {
+      all_conditions.insert( reinterpret_cast<const ConditionIdentifier*>( load_info ) );
+    }
+  } );
+
+  const auto& conditions_by_location_hash = all_conditions.get<by_location_hash>();
+  const auto& conditions_by_filename      = all_conditions.get<by_filename>();
 
-  // we convert the list of conditions to load into a map so that we can easily
-  // flag as already loaded the conditions we get from each URL that are not
-  // what we requested initially. So, if we ask for conditions A and B, and
-  // when processing A we load B as it comes from the same URL, then we flag B
-  // as done so that we can skip the iteration.
-  std::map<dd4hep::Condition::key_type, ToDoItem> todo_list{conditions_to_load.begin(), conditions_to_load.end()};
+  // Extract the range of unique filenames from the conditions
+  auto unique_filenames = conditions_by_filename | rv::transform( &ConditionIdentifier::sys_id ) | rv::unique;
 
   auto block_register = [this]( dd4hep::IOV::Key iov_key, const std::vector<dd4hep::Condition>& entity_conditions ) {
     dd4hep::cond::ConditionsPool* pool = manager().registerIOV( *m_iovType, iov_key );
@@ -146,18 +149,16 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
   };
 
   try {
-    for ( auto [current_key, item] : todo_list ) { // O[N]
+    for ( auto filename : unique_filenames ) {
 
-      auto register_condition = [&loaded, &todo_list, &current_key = current_key,
-                                 &item = item]( std::vector<dd4hep::Condition>& entity_conditions,
-                                                const ConditionIdentifier* cond_ident, std::string cond_name,
-                                                const YAML::Node& cond_data, std::string doc ) {
+      auto register_condition = [&loaded]( std::vector<dd4hep::Condition>& entity_conditions,
+                                           const ConditionIdentifier* cond_ident, const std::string& cond_name,
+                                           const YAML::Node& cond_data, const std::string& doc ) {
         dd4hep::Condition c = LHCb::YAMLConverters::make_condition( cond_name, cond_data );
         if ( !c.isValid() ) {
           // Will eventually have to raise an exception
           dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Failed to load condition %s from %s",
                             cond_name.c_str(), doc.c_str() );
-          return false;
         } else {
           const auto cond_key = cond_ident->hash;
 
@@ -167,59 +168,28 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
 
           // record the condition for bulk registration to the manager
           entity_conditions.push_back( c );
-
-          // if this is the condition we were looking for,
-          // flag the ToDoItem as done and move to the next condition in the YAML object
-          if ( cond_key == current_key ) {
-            item.done = true;
-            return false;
-          }
-          // otherwise see if we are supposed to load it,
-          // in which case the pending work item can be marked as already dine
-          if ( auto cond_itr = todo_list.find( cond_key ); cond_itr != todo_list.end() ) {
-            cond_itr->second.done = true;
-            return false;
-          }
-          return true;
         }
       };
 
-      const auto cond_id = item.cond_id;
-      // FIXME this early skip for conditions already loaded from a URL does not convince me
-      if ( item.done ) { continue; }
-      if ( !cond_id ) {
-        dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ CANNOT update condition: [%16llX] [No load info]",
-                          current_key );
-        continue;
-      }
-
-      const auto* repo = cond_id->repository;
-
-      // set::insert returns a pair where the second tells if the insertion
-      // took place (true) or the entry was already present (false)
-      if ( !processed_urls.insert( cond_id->sys_id ).second ) {
-        // no insertion
-        continue;
-      }
-
       // ready, set, chrono started!
       TTimeStamp start;
       // Load from the git cond db here XXX
       // If the file can't be loaded, create empty conditions for all
       // of the conditions expected in this file
-      if ( m_reader->load( cond_id->sys_id, &ctxt, buffer ) ) {
-        auto ext = std::string_view{cond_id->sys_id};
+      if ( m_reader->load( filename, &ctxt, buffer ) ) {
+        auto ext = std::string_view{filename};
         ext.remove_prefix( ext.find_last_of( '.' ) );
         if ( ext == ".yml" || ext == ".yaml" ) {
-          dd4hep::ConditionKey::KeyMaker kmaker( cond_id->sys_id_hash, 0 );
+          // Use the filename hash as the upper bits of the hash
+          dd4hep::ConditionKey::KeyMaker kmaker( dd4hep::detail::hash32( filename ), 0 );
 
           // condition YAML files are expected to contain a mapping <name> -> <condition data>
-          auto data = YAML::Load( ( m_useOverlay ) ? m_overlay.get_or_default( cond_id->sys_id, buffer ) : buffer );
+          auto data = YAML::Load( ( m_useOverlay ) ? m_overlay.get_or_default( filename, buffer ) : buffer );
 
           // Checking whether we have conditions to override in that same file (i.e. o.path)
           // in that case we directly update the YAML node
           for ( auto const& o : m_conditionsOverride ) {
-            if ( o.path == cond_id->sys_id ) {
+            if ( o.path == filename ) {
               dd4hep::printout( dd4hep::DEBUG, "ConditionsLoader", "++ Overriding %s:%s", o.path.c_str(),
                                 o.name.c_str() );
               data[o.name] = o.value;
@@ -235,37 +205,29 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
           for ( const auto& cond_data : data ) {
             const std::string cond_name = cond_data.first.as<std::string>();
 
-            // see if the condition is in the list of locations(?)
+            // Check if the condition has been requested.
             // Only the upper 32 bits of kmaker.hash have been set
             // above, set the lower bits here
             kmaker.values.item_key = dd4hep::detail::hash32( cond_name );
 
-            auto cond_ident = repo->locations.find( kmaker.hash ); // O[log(N)]
-            if ( cond_ident == repo->locations.end() ) {
-              dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Got stray condition %s from %s",
-                                cond_name.c_str(), cond_id->sys_id.c_str() );
-              continue;
+            auto cond_ident = conditions_by_location_hash.find( kmaker.hash ); // O[log(N)]
+            if ( cond_ident != conditions_by_location_hash.end() ) {
+              register_condition( entity_conditions, *cond_ident, cond_name, cond_data.second, filename );
+            } else {
+              dd4hep::printout( dd4hep::WARNING, "ConditionsLoader", "++ Got stray condition %s from %s",
+                                cond_name.c_str(), filename.c_str() );
             }
-
-            bool stray =
-                register_condition( entity_conditions, *cond_ident, cond_name, cond_data.second, cond_id->sys_id );
-            if ( !stray ) continue;
-
-            // this warning means that we found in the YAML file a condition we were not requesting
-            // FIXME I do not see why this is a problem
-            dd4hep::printout( dd4hep::WARNING, "ConditionsLoader", "++ Got stray but valid condition %s from %s",
-                              cond_name.c_str(), cond_id->sys_id.c_str() );
           }
 
           // Check if there are conditions in this file that have been
           // requested, but have not yet been loaded. If so, create
           // empty conditions for those too.
-          for ( const auto* cond_ident : boost::make_iterator_range(
-                    repo->locations.get<by_filename_hash>().equal_range( cond_id->sys_id_hash ) ) ) {
+          for ( const auto* cond_ident :
+                boost::make_iterator_range( conditions_by_filename.equal_range( filename ) ) ) {
             auto loaded_it = loaded.find( cond_ident->hash );
             if ( loaded_it == loaded.end() ) {
               auto [is_alignment, cond] = empty_condition( cond_ident );
-              register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id );
+              register_condition( entity_conditions, cond_ident, "NotFound", cond, filename );
               dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::INFO, "ConditionsLoader",
                                 "++ Created empty%s condition %s in %s", is_alignment ? " alignment" : "",
                                 cond_ident->object.c_str(), cond_ident->sys_id.c_str() );
@@ -292,23 +254,21 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
         std::string valid_end = ctxt.valid_until == std::numeric_limits<std::int64_t>::max()
                                     ? std::string{"inf   "}
                                     : std::to_string( ctxt.valid_until );
-        dd4hep::printout(
-            dd4hep::INFO, "ConditionsLoader", "++ Added %4ld conditions from %-48s to pool [%6s-%6s] [%7.3f sec]",
-            loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( ctxt.valid_since ).c_str(),
-            valid_end.c_str(), stop.AsDouble() - start.AsDouble() );
+        dd4hep::printout( dd4hep::INFO, "ConditionsLoader",
+                          "++ Added %4ld conditions from %-48s to pool [%6s-%6s] [%7.3f sec]",
+                          loaded.size() - loaded_len, filename.c_str(), std::to_string( ctxt.valid_since ).c_str(),
+                          valid_end.c_str(), stop.AsDouble() - start.AsDouble() );
         loaded_len = loaded.size();
       } else {
         // File containing Conditions not found, create empty
         // conditions for all requested conditions that are in this
         // file.
         std::vector<dd4hep::Condition> entity_conditions;
-        auto const&                    conditions_by_filename_hash = repo->locations.get<by_filename_hash>();
-        entity_conditions.reserve( conditions_by_filename_hash.count( cond_id->sys_id_hash ) );
+        entity_conditions.reserve( conditions_by_filename.count( filename ) );
 
-        for ( const auto* cond_ident :
-              boost::make_iterator_range( conditions_by_filename_hash.equal_range( cond_id->sys_id_hash ) ) ) {
+        for ( const auto* cond_ident : boost::make_iterator_range( conditions_by_filename.equal_range( filename ) ) ) {
           auto [is_alignment, cond] = empty_condition( cond_ident );
-          register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id );
+          register_condition( entity_conditions, cond_ident, "NotFound", cond, filename );
         }
 
         // If the file is not in the DB, it's never going to appear,
@@ -317,7 +277,7 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov,
         block_register( iov_key, entity_conditions );
 
         dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ Added %4ld null conds for  %-48s to pool [%6s-%6s]",
-                          loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( 0 ).c_str(), "inf   " );
+                          loaded.size() - loaded_len, filename.c_str(), std::to_string( 0 ).c_str(), "inf   " );
         loaded_len = loaded.size();
       }
       if ( print_results ) {
-- 
GitLab