From b637058220b7a028553611e9d1003d4cf023879f Mon Sep 17 00:00:00 2001 From: Marco Clemencic <marco.clemencic@cern.ch> Date: Tue, 16 Jul 2024 15:36:28 +0200 Subject: [PATCH] Add the possibility of ignoring the overlay for specific conditions If a file is part of the overlay, we can declare some of its conditions exceptions for the overlay, so that their value is always loaded from the database regardless the content of the overlay. Updates to the value in memory are still recorded in the overlay dump, but ignored when the dump is loaded. --- Core/include/Core/ConditionsLoader.h | 10 ++ Core/src/ConditionsLoader.cpp | 18 ++- Core/src/DetectorDataService.cpp | 24 +++- Core/tests/src/test_DDS_with_Overlay.cpp | 136 ++++++++++++++++++++++- 4 files changed, 179 insertions(+), 9 deletions(-) diff --git a/Core/include/Core/ConditionsLoader.h b/Core/include/Core/ConditionsLoader.h index 561b336c9..15063786d 100644 --- a/Core/include/Core/ConditionsLoader.h +++ b/Core/include/Core/ConditionsLoader.h @@ -37,6 +37,8 @@ namespace LHCb::Detector { }; using ConditionsOverrides = std::vector<ConditionOverrideEntry>; + using ConditionsOverlayExceptions = std::map<std::string, std::vector<std::string>>; + class ConditionsLoader : public dd4hep::cond::ConditionsDataLoader { using Key = std::pair<std::string, std::string>; using KeyMap = std::map<dd4hep::Condition::key_type, Key>; @@ -59,6 +61,10 @@ namespace LHCb::Detector { ConditionsOverlay m_overlay; ConditionsOverrides m_conditionsOverride; + // Map of filenames -> list of conditions names for which we have to ignore the overlay + // (and always go to the DB) + ConditionsOverlayExceptions m_overlayExceptions; + public: /// Default constructor ConditionsLoader( dd4hep::Detector& description, dd4hep::cond::ConditionsManager mgr, const std::string& nam ); @@ -81,5 +87,9 @@ namespace LHCb::Detector { throw std::logic_error( "ConditionsLoader::set_limited_iov_paths invoked before initialize" ); } } + // Set exceptions to the conditions overlay + void set_overlay_exceptions( ConditionsOverlayExceptions exceptions ) { + m_overlayExceptions = std::move( exceptions ); + } }; } // namespace LHCb::Detector diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp index 6652c1dad..36ac739c3 100644 --- a/Core/src/ConditionsLoader.cpp +++ b/Core/src/ConditionsLoader.cpp @@ -216,6 +216,14 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, } } + std::vector<std::string> conditionsWithoutOverlay; + std::optional<YAML::Node> origData; + if ( auto it = m_overlayExceptions.find( filename ); + it != m_overlayExceptions.end() && !it->second.empty() ) { + conditionsWithoutOverlay = it->second; + origData = YAML::Load( buffer ); + } + // 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_conditions; @@ -223,7 +231,12 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // 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>(); + const std::string cond_name = cond_data.first.as<std::string>(); + std::optional<YAML::Node> noOverlayData; + if ( auto it = std::find( conditionsWithoutOverlay.begin(), conditionsWithoutOverlay.end(), cond_name ); + it != conditionsWithoutOverlay.end() ) { + noOverlayData = ( *origData )[cond_name]; + } // Check if the condition has been requested. // Only the upper 32 bits of kmaker.hash have been set @@ -232,7 +245,8 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, 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 ); + register_condition( entity_conditions, *cond_ident, cond_name, + noOverlayData ? *noOverlayData : cond_data.second, filename ); } else { dd4hep::printout( dd4hep::WARNING, "ConditionsLoader", "++ Got stray condition %s from %s", cond_name.c_str(), filename.c_str() ); diff --git a/Core/src/DetectorDataService.cpp b/Core/src/DetectorDataService.cpp index 0075bb9d1..f9054632a 100644 --- a/Core/src/DetectorDataService.cpp +++ b/Core/src/DetectorDataService.cpp @@ -130,11 +130,25 @@ void LHCb::Detector::DetectorDataService::initialize( const nlohmann::json& conf auto overlay = config["overlay"]; if ( overlay.is_boolean() ) { loader["UseOverlay"] = overlay.get<bool>(); - } else if ( overlay.is_object() && overlay.contains( "path" ) ) { - loader["UseOverlay"] = true; - loader["OverlayInitPath"] = overlay.at( "path" ).get<std::string>(); - } else { - throw std::invalid_argument( fmt::format( "invalid configuration for conditions overlay '{}'", overlay ) ); + } else if ( overlay.is_object() ) { + loader["UseOverlay"] = true; + for ( const auto& el : overlay.items() ) { + if ( el.key() != "path" && el.key() != "exceptions" ) { + throw std::invalid_argument( fmt::format( "invalid configuration for conditions overlay '{}'", overlay ) ); + } + if ( el.key() == "path" ) { + loader["OverlayInitPath"] = overlay["path"].get<std::string>(); + } else if ( el.key() == "exceptions" ) { + // I would love to use something like + // loader["OverlayExceptions"] = overlay["exceptions"].get<ConditionsOverlayExceptions>(); + // but I didn't manage to figure out how to convince DD4hep properties to understand + // map<string,vector<string>> + + // As explained later this is safe to do. + auto loader_impl = static_cast<ConditionsLoader*>( &loader ); + loader_impl->set_overlay_exceptions( overlay["exceptions"].get<ConditionsOverlayExceptions>() ); + } + } } } loader.initialize(); diff --git a/Core/tests/src/test_DDS_with_Overlay.cpp b/Core/tests/src/test_DDS_with_Overlay.cpp index 2014a45a6..1b6d99d2a 100644 --- a/Core/tests/src/test_DDS_with_Overlay.cpp +++ b/Core/tests/src/test_DDS_with_Overlay.cpp @@ -80,12 +80,12 @@ VPRight: !alignment CHECK_THAT( align.translation.Y(), Catch::Matchers::WithinRel( 5 * mm, 1.e-4 ) ); CHECK_THAT( align.translation.Z(), Catch::Matchers::WithinRel( 7 * mm, 1.e-4 ) ); - // phase 3: change the alignment paramters and update the overlay + // phase 3: change the alignment parameters and update the overlay { RotationZYX rot{RotationZ{5 * deg}}; dd4hep::Delta new_align{Position{1 * mm, 4 * mm, 2.5 * mm}, rot}; dds.update_alignment( de.detector(), new_align ); - // We cannot check anything here becase the overlay instance is private to the system + // We cannot check anything here because the overlay instance is private to the system } // phase 4: reload alignments (from overlay) @@ -150,3 +150,135 @@ VPRight: !alignment CHECK_THAT( align.rotation.Theta(), Catch::Matchers::WithinAbs( 0, 1.e-6 ) ); CHECK_THAT( align.rotation.Psi(), Catch::Matchers::WithinAbs( 0, 1.e-6 ) ); } + +TEST_CASE( "DDS with overlay and exceptions" ) { + namespace fs = std::filesystem; + + Detector::Test::Fixture f; + + auto& description = f.description(); + + description.fromXML( "compact/run3/trunk/debug/VP_debug.xml" ); + // description.fromXML( "compact/run3/trunk/LHCb.xml" ); + + REQUIRE( description.state() == dd4hep::Detector::READY ); + + auto det = description.detector( "VP" ); + // the `!!` is needed because handles have `operator!` but not `operator bool` + REQUIRE( !!det ); + + Detector::Test::TmpDir tmp; + + // phase 1: prepare the directory with the initial alignments (to fill the overlay) + auto cond_dir = tmp.path / "Conditions" / "VP" / "Alignment"; + fs::create_directories( cond_dir ); + std::ofstream( cond_dir / "Global.yml" ) << R"(--- +VPSystem: !alignment + position: [0.0 * mm, 5.0 * mm, 7.0 * mm] +VPLeft: !alignment + position: [1.0 * mm, -2.0 * mm, 0.0 * mm] +VPRight: !alignment + position: [-1.0 * mm, -2.0 * mm, 0.0 * mm] +)"; + REQUIRE( !fs::exists( cond_dir / "Ladders.yml" ) ); + + // phase 2: load conditions with the overlay filled from the injected data + LHCb::Detector::DetectorDataService dds( description, {"/world", "VP"} ); + dds.initialize( nlohmann::json( + {{"repository", "file:tests/ConditionsIOV"}, + {"overlay", {{"path", tmp.path}, {"exceptions", {{"Conditions/VP/Alignment/Global.yml", {"VPSystem"}}}}}}} ) ); + auto slice = dds.get_slice( 100 ); + REQUIRE( slice ); + + using LHCb::Detector::DeVP; + DeVP de = slice->get( det, LHCb::Detector::Keys::deKey ); + REQUIRE( !!de ); + + using dd4hep::deg; + using dd4hep::mm; + using dd4hep::Position; + using dd4hep::RotationZ; + using dd4hep::RotationZYX; + using dd4hep::Translation3D; + + // check that we have what we expect + auto align = getDelta( de ); + CHECK( align.hasTranslation() ); + CHECK( !align.hasRotation() ); + CHECK( !align.hasPivot() ); + CHECK_THAT( align.translation.X(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + CHECK_THAT( align.translation.Y(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + CHECK_THAT( align.translation.Z(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + + // phase 3: change the alignment parameters and update the overlay + { + RotationZYX rot{RotationZ{5 * deg}}; + dd4hep::Delta new_align{Position{1 * mm, 4 * mm, 2.5 * mm}, rot}; + dds.update_alignment( de.detector(), new_align ); + // We cannot check anything here because the overlay instance is private to the system + } + + // phase 4: reload alignments (from overlay) + dds.clear_slice_cache(); // this is to make sure we force a reload of everything + auto slice2 = dds.get_slice( 100 ); + + REQUIRE( slice2 ); + REQUIRE( slice != slice2 ); + de = slice2->get( det, LHCb::Detector::Keys::deKey ); + CHECK( !!de ); + + // check that the change in memory as no effect on the alignment that should not + // come from the overlay + align = getDelta( de ); + CHECK( align.hasTranslation() ); + CHECK( !align.hasRotation() ); + CHECK( !align.hasPivot() ); + CHECK_THAT( align.translation.X(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + CHECK_THAT( align.translation.Y(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + CHECK_THAT( align.translation.Z(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + + // phase 5: change again in memory and dump alignments to files + { + RotationZYX rot{RotationZ{5 * deg}}; + dd4hep::Delta new_align{Position{1 * mm, 4 * mm, 2.5 * mm}, rot}; + dds.update_alignment( de.detector(), new_align ); + } + dds.dump_conditions( tmp.path ); + { + REQUIRE( fs::exists( cond_dir / "Global.yml" ) ); + // dump_conditions (by default) should not write unchanged entries + CHECK( !fs::exists( cond_dir / "Ladders.yml" ) ); + + // check that the YAML file contains the expected changes + auto gbl = YAML::LoadFile( tmp.path / "Conditions" / "VP" / "Alignment" / "Global.yml" ); + auto sys = gbl["VPSystem"]; + REQUIRE( sys ); + CHECK( sys.Tag() == "!alignment" ); + std::stringstream ss; + ss << sys["position"]; + CHECK( ss.str() == "[1 * mm, 4 * mm, 2.5 * mm]" ); + CHECK( sys["rotation"] ); + CHECK( !sys["pivot"] ); + } + + // phase 6: reload the overlay from files + dds.load_conditions( tmp.path ); + + // phase 7: reload alignments (from overlay) + dds.clear_slice_cache(); + auto slice3 = dds.get_slice( 100 ); + + REQUIRE( slice3 ); + REQUIRE( slice2 != slice3 ); + de = slice3->get( det, LHCb::Detector::Keys::deKey ); + CHECK( !!de ); + + // we still expect the alignment to be the one we have in the DB (and not the overlay) + align = getDelta( de ); + CHECK( align.hasTranslation() ); + CHECK( !align.hasRotation() ); + CHECK( !align.hasPivot() ); + CHECK_THAT( align.translation.X(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + CHECK_THAT( align.translation.Y(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); + CHECK_THAT( align.translation.Z(), Catch::Matchers::WithinAbs( 0 * mm, 1.e-6 ) ); +} -- GitLab