From 83ce3b3bf167853ac185531703dd1fce6e961a17 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 18 Dec 2023 14:19:22 +0100 Subject: [PATCH 1/2] [RF] Make dependency of RooFit JSON Interface on `nlohmann` private In the RooFit `json_interface`, the calls to the actual JSON library are abstracted away, such that the libraries like `nlohmann_json` are not included in the headers. Therefore, `nlohmann_json` is a private dependency of the RooFit JSON interface, not a public one. --- roofit/jsoninterface/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roofit/jsoninterface/CMakeLists.txt b/roofit/jsoninterface/CMakeLists.txt index ae4b3430fe59d..28616023ac8b2 100644 --- a/roofit/jsoninterface/CMakeLists.txt +++ b/roofit/jsoninterface/CMakeLists.txt @@ -44,5 +44,5 @@ endif() if(builtin_nlohmannjson) target_include_directories(RooFitJSONInterface PRIVATE ${CMAKE_SOURCE_DIR}/builtins) else() - target_link_libraries(RooFitJSONInterface PUBLIC nlohmann_json::nlohmann_json) + target_link_libraries(RooFitJSONInterface PRIVATE nlohmann_json::nlohmann_json) endif() From a401ead155c4e0d80b90dd62fbce5e2ef86a5dc5 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 18 Dec 2023 14:02:29 +0100 Subject: [PATCH 2/2] [RF] Avoid `nlohmann_json` as a public dependency of RooFit Multiprocess Using `nlohmann_json` as a public dependency of ROOT can result in different troubles, like #14188. That's why it's better to avoid this dependency if we can, also to minimize the dependency of RooFit on the interface level. In the case of RooFit, the only reason for this dependency was the `HeatmapAnalyzer::getMetadata()` function. However, it just returned a json with a vector of string lables. We can also return a `std::vector` here. I already talked with @Zeff020 about this change, and he is completely fine with it. The class was also only used by the RooFit Multiprocess developers so far, so changing the interface is fine. With this interface change, only some refactoring was necessary to avoid including `nlohmann_json` in the RooFit headers. This commit is similar to 9d7aa4a96e. --- roofit/multiprocess/CMakeLists.txt | 6 +- .../inc/RooFit/MultiProcess/HeatmapAnalyzer.h | 27 ++-- roofit/multiprocess/src/HeatmapAnalyzer.cxx | 121 +++++++++++------- 3 files changed, 87 insertions(+), 67 deletions(-) diff --git a/roofit/multiprocess/CMakeLists.txt b/roofit/multiprocess/CMakeLists.txt index d99e60a5e8f33..46e35993fc014 100644 --- a/roofit/multiprocess/CMakeLists.txt +++ b/roofit/multiprocess/CMakeLists.txt @@ -29,11 +29,9 @@ target_include_directories(RooFitMultiProcess INTERFACE $) if(builtin_nlohmannjson) - target_include_directories(RooFitMultiProcess - PRIVATE ${CMAKE_SOURCE_DIR}/builtins - INTERFACE $/builtins) + target_include_directories(RooFitMultiProcess PRIVATE ${CMAKE_SOURCE_DIR}/builtins) else() - target_link_libraries(RooFitMultiProcess PUBLIC nlohmann_json::nlohmann_json) + target_link_libraries(RooFitMultiProcess PRIVATE nlohmann_json::nlohmann_json) endif() ROOT_ADD_TEST_SUBDIRECTORY(test) diff --git a/roofit/multiprocess/inc/RooFit/MultiProcess/HeatmapAnalyzer.h b/roofit/multiprocess/inc/RooFit/MultiProcess/HeatmapAnalyzer.h index e469c36c1b347..cccdfddca6cc4 100644 --- a/roofit/multiprocess/inc/RooFit/MultiProcess/HeatmapAnalyzer.h +++ b/roofit/multiprocess/inc/RooFit/MultiProcess/HeatmapAnalyzer.h @@ -19,15 +19,22 @@ #include #include #include -#include -using json = nlohmann::json; namespace RooFit { namespace MultiProcess { +namespace Detail { +// To avoid unnecessary dependence on nlohman json in the interface. Note that +// we should not forward declare nlohmann::json directly, since its declaration +// might change (it is currently a typedef). With this wrapper type, we are +// completely decoupled on nlohmann::json in the RMetaData interface. +struct HeatmapAnalyzerJsonData; +} // namespace Detail + class HeatmapAnalyzer { public: - HeatmapAnalyzer(std::string const& logs_dir); + HeatmapAnalyzer(std::string const &logs_dir); + ~HeatmapAnalyzer(); // main method of this class, produces heatmap std::unique_ptr analyze(int analyzed_gradient); @@ -35,19 +42,11 @@ class HeatmapAnalyzer { // getters to inspect logfiles std::vector const getPartitionNames(); std::vector const getTaskNames(); - json const getMetadata(); + std::vector const getMetadata(); private: - // internal helper functions - std::string findTaskForDuration(json durations, int start_t, int end_t); - void sortTaskNames(std::vector &task_names); - TH2I matrix_; - - json gradients_; - json metadata_; - std::vector durations_; - + std::unique_ptr jsonData_; std::vector tasks_names_; std::vector eval_partitions_names_; }; @@ -55,4 +54,4 @@ class HeatmapAnalyzer { } // namespace MultiProcess } // namespace RooFit -#endif // ROOT_ROOFIT_MultiProcess_HeatmapAnalyzer \ No newline at end of file +#endif // ROOT_ROOFIT_MultiProcess_HeatmapAnalyzer diff --git a/roofit/multiprocess/src/HeatmapAnalyzer.cxx b/roofit/multiprocess/src/HeatmapAnalyzer.cxx index 183bf3fd6be16..44f2c7d92c5bf 100644 --- a/roofit/multiprocess/src/HeatmapAnalyzer.cxx +++ b/roofit/multiprocess/src/HeatmapAnalyzer.cxx @@ -10,16 +10,64 @@ * listed in LICENSE (http://roofit.sourceforge.net/license.txt) */ -#include "RooFit/MultiProcess/HeatmapAnalyzer.h" +#include -#include "TSystemDirectory.h" -#include "TList.h" +#include +#include + +#include #include namespace RooFit { namespace MultiProcess { +namespace Detail { + +struct HeatmapAnalyzerJsonData { + nlohmann::json gradients; + nlohmann::json metadata; + std::vector durations; +}; + +} // namespace Detail + +namespace { + +void sortTaskNames(std::vector &task_names) +{ + char const *digits = "0123456789"; + std::vector digit_vec; + std::vector> pair_vec; + for (auto &&el : task_names) { + std::size_t const n = el.find_first_of(digits); + pair_vec.push_back(std::make_pair(stoi(el.substr(n)), el)); + } + + std::sort(pair_vec.begin(), pair_vec.end()); + + for (size_t i = 0; i < task_names.size(); i++) { + task_names[i] = pair_vec[i].second; + } +} + +std::string findTaskForDuration(nlohmann::json durations, int start_t, int end_t) +{ + for (auto &&el : durations.items()) { + if (el.key().find("eval_partition") != std::string::npos) + continue; + + for (size_t idx = 0; idx < durations[el.key()].size(); idx += 2) { + if (durations[el.key()][idx] <= start_t && durations[el.key()][idx + 1] >= end_t) { + return el.key(); + } + } + } + return ""; +} + +} // namespace + /** \class HeatmapAnalyzer * * \brief Reads and processes logfiles produced by RooFit::MultiProcess::ProcessTimer @@ -45,6 +93,7 @@ namespace MultiProcess { /// outputted by RooFit::MultiProcess::ProcessTimer. /// There can be other files in this directory as well. HeatmapAnalyzer::HeatmapAnalyzer(std::string const &logs_dir) + : jsonData_{std::make_unique()} { TSystemDirectory dir(logs_dir.c_str(), logs_dir.c_str()); std::unique_ptr durationFiles{dir.GetListOfFiles()}; @@ -56,13 +105,13 @@ HeatmapAnalyzer::HeatmapAnalyzer(std::string const &logs_dir) std::ifstream f(logs_dir + "/" + std::string(file->GetName())); if (std::string(file->GetName()).find("999") != std::string::npos) { - gradients_ = json::parse(f); + jsonData_->gradients = nlohmann::json::parse(f); } else { - durations_.push_back(json::parse(f)); + jsonData_->durations.push_back(nlohmann::json::parse(f)); } } - for (json &durations_json : durations_) { + for (nlohmann::json &durations_json : jsonData_->durations) { for (auto &&el : durations_json.items()) { if (el.key().find("eval_task") != std::string::npos && std::find(tasks_names_.begin(), tasks_names_.end(), el.key()) == tasks_names_.end()) { @@ -72,18 +121,20 @@ HeatmapAnalyzer::HeatmapAnalyzer(std::string const &logs_dir) eval_partitions_names_.end()) { eval_partitions_names_.push_back(el.key()); } else if (el.key().find("metadata") != std::string::npos) { - metadata_ = durations_json[el.key()]; + jsonData_->metadata = durations_json[el.key()]; } } } - for (json &durations_json : durations_) { + for (nlohmann::json &durations_json : jsonData_->durations) { durations_json.erase("metadata"); } sortTaskNames(tasks_names_); } +HeatmapAnalyzer::~HeatmapAnalyzer() = default; + //////////////////////////////////////////////////////////////////////////////// /// This method is the main functionality in this class. It does the heavy /// lifting of matching duration timestamps to tasks and partition evaluations. @@ -92,16 +143,16 @@ HeatmapAnalyzer::HeatmapAnalyzer(std::string const &logs_dir) /// in the logs. std::unique_ptr HeatmapAnalyzer::analyze(int analyzed_gradient) { - int gradient_start_t = gradients_["master:gradient"][analyzed_gradient * 2 - 2]; - int gradient_end_t = gradients_["master:gradient"][analyzed_gradient * 2 - 1]; + int gradient_start_t = jsonData_->gradients["master:gradient"][analyzed_gradient * 2 - 2]; + int gradient_end_t = jsonData_->gradients["master:gradient"][analyzed_gradient * 2 - 1]; - std::unique_ptr total_matrix = + auto total_matrix = std::make_unique("heatmap", "", eval_partitions_names_.size(), 0, 1, tasks_names_.size(), 0, 1); // loop over all logfiles stored in durations_ - for (json &durations_json : durations_) { + for (nlohmann::json &durations_json : jsonData_->durations) { // partial heatmap is the heatmap that will be filled in for the current durations logfile - std::unique_ptr partial_matrix = + auto partial_matrix = std::make_unique("partial_heatmap", "", eval_partitions_names_.size(), 0, 1, tasks_names_.size(), 0, 1); // remove unnecessary components (those that are out of range) @@ -150,11 +201,11 @@ std::unique_ptr HeatmapAnalyzer::analyze(int analyzed_gradient) TAxis *y = total_matrix->GetYaxis(); TAxis *x = total_matrix->GetXaxis(); for (std::size_t i = 0; i != tasks_names_.size(); ++i) { - y->SetBinLabel(i + 1, (metadata_[0][i].get()).c_str()); + y->SetBinLabel(i + 1, jsonData_->metadata[0][i].get().c_str()); y->ChangeLabel(i + 1, 30, 0.01, -1, -1, -1, ""); } for (std::size_t i = 0; i != eval_partitions_names_.size(); ++i) { - x->SetBinLabel(i + 1, (eval_partitions_names_[i]).c_str()); + x->SetBinLabel(i + 1, eval_partitions_names_[i].c_str()); x->ChangeLabel(i + 1, 30, -1, -1, -1, -1, ""); } x->LabelsOption("v"); @@ -165,48 +216,20 @@ std::unique_ptr HeatmapAnalyzer::analyze(int analyzed_gradient) std::vector const HeatmapAnalyzer::getTaskNames() { return tasks_names_; -}; +} std::vector const HeatmapAnalyzer::getPartitionNames() { return eval_partitions_names_; -}; - -json const HeatmapAnalyzer::getMetadata() -{ - return metadata_; -}; - -std::string HeatmapAnalyzer::findTaskForDuration(json durations, int start_t, int end_t) -{ - for (auto &&el : durations.items()) { - if (el.key().find("eval_partition") != std::string::npos) - continue; - - for (size_t idx = 0; idx < durations[el.key()].size(); idx += 2) { - if (durations[el.key()][idx] <= start_t && durations[el.key()][idx + 1] >= end_t) { - return el.key(); - } - } - } - return ""; } -void HeatmapAnalyzer::sortTaskNames(std::vector &task_names) +std::vector const HeatmapAnalyzer::getMetadata() { - char const *digits = "0123456789"; - std::vector digit_vec; - std::vector> pair_vec; - for (auto &&el : task_names) { - std::size_t const n = el.find_first_of(digits); - pair_vec.push_back(std::make_pair(stoi(el.substr(n)), el)); - } - - std::sort(pair_vec.begin(), pair_vec.end()); - - for (size_t i = 0; i < task_names.size(); i++) { - task_names[i] = pair_vec[i].second; + std::vector out; + for (auto const &item : jsonData_->metadata[0]) { + out.emplace_back(item.get()); } + return out; } } // namespace MultiProcess