-
Notifications
You must be signed in to change notification settings - Fork 43
Cleanup main model: dependency inversion main model impl info #1107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 18 commits
fa6808a
87057e3
9962299
8462283
1cec998
0fe5288
85280c2
b60c807
53f80f4
45bbcbb
0cfed9e
7496e27
9086696
b0156a2
3d99233
28d06aa
6fab6c1
060d3f1
9ad7b8b
d348ff7
1a20b38
ec8490b
93dff85
3bb015e
8026cea
2c7341c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
// SPDX-FileCopyrightText: Contributors to the Power Grid Model project <[email protected]> | ||
// | ||
// SPDX-License-Identifier: MPL-2.0 | ||
|
||
#pragma once | ||
|
||
#include "logging_impl.hpp" | ||
|
||
#include <mutex> | ||
|
||
namespace power_grid_model::common::logging { | ||
|
||
template <std::derived_from<Logger> LoggerType> | ||
requires requires(LoggerType& destination, LoggerType const& source) { | ||
{ merge_into(destination, source) }; | ||
figueroa1395 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
class MultiThreadedLoggerImpl : public MultiThreadedLogger { | ||
using SubThreadLogger = LoggerType; | ||
mgovers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public: | ||
class ThreadLogger : public SubThreadLogger { | ||
public: | ||
ThreadLogger(MultiThreadedLoggerImpl& parent) : parent_{&parent} {} | ||
ThreadLogger(ThreadLogger const&) = default; | ||
ThreadLogger& operator=(ThreadLogger const&) = default; | ||
ThreadLogger(ThreadLogger&&) noexcept = default; | ||
ThreadLogger& operator=(ThreadLogger&&) noexcept = default; | ||
~ThreadLogger() override { sync(); } | ||
void sync() const { parent_->sync(*this); } | ||
|
||
private: | ||
MultiThreadedLoggerImpl* parent_; | ||
}; | ||
|
||
std::unique_ptr<Logger> create_child() override { return std::make_unique<ThreadLogger>(*this); } | ||
LoggerType& get() { return log_; } | ||
LoggerType const& get() const { return log_; } | ||
|
||
void log(LogEvent tag) override { log_.log(tag); } | ||
void log(LogEvent tag, std::string_view message) override { log_.log(tag, message); } | ||
void log(LogEvent tag, double value) override { log_.log(tag, value); } | ||
void log(LogEvent tag, Idx value) override { log_.log(tag, value); } | ||
|
||
private: | ||
friend class ThreadLogger; | ||
|
||
LoggerType log_; | ||
std::mutex mutex_; | ||
|
||
void sync(ThreadLogger const& logger) { | ||
std::lock_guard const lock{mutex_}; | ||
merge_into(log_, static_cast<SubThreadLogger const&>(logger)); | ||
mgovers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}; | ||
|
||
using NoMultiThreadedLogger = MultiThreadedLoggerImpl<NoLogger>; | ||
|
||
} // namespace power_grid_model::common::logging |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,6 @@ | |
#include "job_interface.hpp" | ||
#include "main_model_fwd.hpp" | ||
|
||
#include "main_core/calculation_info.hpp" | ||
#include "main_core/update.hpp" | ||
|
||
namespace power_grid_model { | ||
|
@@ -23,15 +22,19 @@ class JobAdapter<MainModel, ComponentList<ComponentType...>> | |
public: | ||
JobAdapter(std::reference_wrapper<MainModel> model_reference, | ||
std::reference_wrapper<MainModelOptions const> options) | ||
: model_reference_{model_reference}, options_{options} {} | ||
: model_reference_{model_reference}, options_{options} { | ||
reset_logger_impl(); | ||
} | ||
JobAdapter(JobAdapter const& other) | ||
: model_copy_{std::make_unique<MainModel>(other.model_reference_.get())}, | ||
model_reference_{std::ref(*model_copy_)}, | ||
options_{std::ref(other.options_)}, | ||
components_to_update_{other.components_to_update_}, | ||
update_independence_{other.update_independence_}, | ||
independence_flags_{other.independence_flags_}, | ||
all_scenarios_sequence_{other.all_scenarios_sequence_} {} | ||
all_scenarios_sequence_{other.all_scenarios_sequence_} { | ||
reset_logger_impl(); | ||
} | ||
JobAdapter& operator=(JobAdapter const& other) { | ||
if (this != &other) { | ||
model_copy_ = std::make_unique<MainModel>(other.model_reference_.get()); | ||
|
@@ -41,6 +44,8 @@ class JobAdapter<MainModel, ComponentList<ComponentType...>> | |
update_independence_ = other.update_independence_; | ||
independence_flags_ = other.independence_flags_; | ||
all_scenarios_sequence_ = other.all_scenarios_sequence_; | ||
|
||
reset_logger_impl(); | ||
} | ||
return *this; | ||
} | ||
|
@@ -51,7 +56,9 @@ class JobAdapter<MainModel, ComponentList<ComponentType...>> | |
components_to_update_{std::move(other.components_to_update_)}, | ||
update_independence_{std::move(other.update_independence_)}, | ||
independence_flags_{std::move(other.independence_flags_)}, | ||
all_scenarios_sequence_{std::move(other.all_scenarios_sequence_)} {} | ||
all_scenarios_sequence_{std::move(other.all_scenarios_sequence_)} { | ||
reset_logger_impl(); | ||
} | ||
JobAdapter& operator=(JobAdapter&& other) noexcept { | ||
if (this != &other) { | ||
model_copy_ = std::move(other.model_copy_); | ||
|
@@ -61,10 +68,15 @@ class JobAdapter<MainModel, ComponentList<ComponentType...>> | |
update_independence_ = std::move(other.update_independence_); | ||
independence_flags_ = std::move(other.independence_flags_); | ||
all_scenarios_sequence_ = std::move(other.all_scenarios_sequence_); | ||
|
||
reset_logger_impl(); | ||
} | ||
return *this; | ||
} | ||
~JobAdapter() { model_copy_.reset(); } | ||
~JobAdapter() { | ||
reset_logger_impl(); | ||
model_copy_.reset(); | ||
} | ||
|
||
private: | ||
// Grant the CRTP base (JobInterface<JobAdapter>) access to | ||
|
@@ -83,7 +95,7 @@ class JobAdapter<MainModel, ComponentList<ComponentType...>> | |
// current_scenario_sequence_cache_ is calculated per scenario, so it is excluded from the constructors. | ||
main_core::utils::SequenceIdx<ComponentType...> current_scenario_sequence_cache_{}; | ||
|
||
std::mutex calculation_info_mutex_; | ||
Logger* log_{nullptr}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does job adapter also need to store it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i've thought about his for a bit and this relates to the registering/unregistering discussion from elsewhere in this PR (specifically the one in #1107) i've described the fact that the registering/unregistering is quite tricky in itself. an alternative could be to use thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Experiment in #1115 |
||
|
||
void calculate_impl(MutableDataset const& result_data, Idx scenario_idx) const { | ||
MainModel::calculator(options_.get(), model_reference_.get(), result_data.get_individual_scenario(scenario_idx), | ||
|
@@ -135,14 +147,6 @@ class JobAdapter<MainModel, ComponentList<ComponentType...>> | |
std::ranges::for_each(current_scenario_sequence_cache_, [](auto& comp_seq_idx) { comp_seq_idx.clear(); }); | ||
} | ||
|
||
CalculationInfo get_calculation_info_impl() const { return model_reference_.get().calculation_info(); } | ||
void reset_calculation_info_impl() { model_reference_.get().reset_calculation_info(); } | ||
|
||
void thread_safe_add_calculation_info_impl(CalculationInfo const& info) { | ||
std::lock_guard const lock{calculation_info_mutex_}; | ||
model_reference_.get().merge_calculation_info(info); | ||
} | ||
|
||
auto get_current_scenario_sequence_view_() const { | ||
return main_core::utils::run_functor_with_all_types_return_array<ComponentType...>([this]<typename CT>() { | ||
constexpr auto comp_idx = main_core::utils::index_of_component<CT, ComponentType...>; | ||
|
@@ -152,5 +156,14 @@ class JobAdapter<MainModel, ComponentList<ComponentType...>> | |
return std::span<Idx2D const>{std::get<comp_idx>(current_scenario_sequence_cache_)}; | ||
}); | ||
} | ||
|
||
void reset_logger_impl() { | ||
log_ = nullptr; | ||
model_reference_.get().reset_logger(); | ||
} | ||
void set_logger_impl(Logger& log) { | ||
log_ = &log; | ||
model_reference_.get().set_logger(*log_); | ||
} | ||
Comment on lines
+159
to
+167
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tells me now than Edit: I see that main model just holds a pointer to each logger, which the adapter does too, so we can just redirect on each thread... This seems fine, and can potentially help us clean the pattern used here in the adapter of having a reference wrapper and a copy of the model at the same time. However, even though this makes it clean having "copies" in two places, could that bite us in the butt later? Should we keep a pointer to the logger only here for instance, and call it via the adaptor in main model impl? Probably not, but maybe something worth thinking about for a couple of minutes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you like, we can either go with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if either convince me, but I don't have any alternative idea either. So we can try the view and see how it looks like (puns intended 😆) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's do it in a separate PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this comment can be answered as well by #1115, right? |
||
}; | ||
} // namespace power_grid_model |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SubThreadLogger being CalculationInfo was kind of confusing to me before review.
It is now a SubThreadLogger which is a "map".
Lets throw all notions of earlier CalculationInfo out? Or atleast rename te calculation_info in places like MathSolver.
We do that in a later PR or now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
math solver was already completely made agnostic of the calculation info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only locations that IMO should know of calculation info are the objects/functions that manage a calculation info in their own scope, that is: the benchmark and the C API (
main_model.hpp
for now)