Cleanup main model: dependency inversion main model impl info #1107

mgovers · 2025-09-01T14:06:49Z

Make calculation info a proper logger
Give main model impl and job adapter the logger
Multithreaded logging is a specialized class and no longer is scattered throughout the job dispatch and adapter
NOTE: the destructors are responsible for syncing and unregistering the child classes

relates to #1093

Signed-off-by: Martijn Govers <[email protected]>

…version-main-model-impl-info Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>

Copilot

Pull Request Overview

This PR refactors the calculation info system to use dependency inversion by replacing direct CalculationInfo usage with a proper Logger interface. The key changes include replacing CalculationInfo with Logger in the main model implementation and introducing multi-threaded logging capabilities.

Replaces CalculationInfo member with Logger* in main model implementation
Introduces MultiThreadedLogger interface and implementation for thread-safe logging
Updates job adapter and dispatch logic to use logger pattern instead of direct calculation info management

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/native_api_tests/test_api_model_update.cpp	Removes unused iostream include
tests/cpp_unit_tests/test_job_dispatch.cpp	Updates mock job adapter to use logger pattern and adds test logger implementations
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp	Replaces calculation_info_ with log_ pointer and updates logger usage
power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp	Adds MultiThreadedCalculationInfo member and updates batch calculation
power_grid_model_c/power_grid_model/include/power_grid_model/job_interface.hpp	Replaces calculation info methods with logger methods
power_grid_model_c/power_grid_model/include/power_grid_model/job_dispatch.hpp	Updates job dispatch to use multi-threaded logger pattern
power_grid_model_c/power_grid_model/include/power_grid_model/job_adapter.hpp	Implements logger interface methods and removes calculation info logic
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp	Removes unused iostream include
power_grid_model_c/power_grid_model/include/power_grid_model/common/multi_threaded_logging.hpp	New file implementing multi-threaded logger template
power_grid_model_c/power_grid_model/include/power_grid_model/common/logging_impl.hpp	Adds merge_into function for NoLogger
power_grid_model_c/power_grid_model/include/power_grid_model/common/logging.hpp	Adds MultiThreadedLogger interface
power_grid_model_c/power_grid_model/include/power_grid_model/common/common.hpp	Adds concepts include
power_grid_model_c/power_grid_model/include/power_grid_model/common/calculation_info.hpp	Adds MultiThreadedCalculationInfo class

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tests/cpp_unit_tests/test_job_dispatch.cpp

power_grid_model_c/power_grid_model/include/power_grid_model/common/multi_threaded_logging.hpp

power_grid_model_c/power_grid_model/include/power_grid_model/job_dispatch.hpp

Signed-off-by: Martijn Govers <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

…ommon/multi_threaded_logging.hpp Signed-off-by: Martijn Govers <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

…ob_dispatch.hpp Signed-off-by: Martijn Govers <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>

mgovers · 2025-09-02T06:51:38Z

power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp

+    Logger& logger() const {
+        static common::logging::NoLogger no_log{};
+        return log_ != nullptr ? *log_ : no_log;
+    }


one future improvement could be to pass the logger not by reference but by pass-by-value view that does this same thing.

What do you mean with pass-by-value view in this context?

tests/cpp_unit_tests/test_job_dispatch.cpp

Signed-off-by: Martijn Govers <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>

mgovers · 2025-09-02T07:57:45Z

do-not-merge label removed because i think the PR is in a good state for a first iteration

Signed-off-by: Martijn Govers <[email protected]>

…-info Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>

nitbharambe · 2025-09-02T09:35:50Z

power_grid_model_c/power_grid_model/include/power_grid_model/job_adapter.hpp

@@ -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};


Why does job adapter also need to store it?

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 model.calculate(..., log_). Now we can also do a hybrid situation here: the adapter could be the boundary between registering/unregistering (to the job adapter). The model itself could be model.calculate(..., log_). Then, the adapter still contains a logger, but the model no longer does.

thoughts?

Experiment in #1115

nitbharambe · 2025-09-02T09:46:19Z

power_grid_model_c/power_grid_model/include/power_grid_model/common/calculation_info.hpp

+    return destination;
+}
+
+class MultiThreadedCalculationInfo : public MultiThreadedLoggerImpl<CalculationInfo> {


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?

math solver was already completely made agnostic of the calculation info

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)

power_grid_model_c/power_grid_model/include/power_grid_model/common/multi_threaded_logging.hpp

power_grid_model_c/power_grid_model/include/power_grid_model/common/logging_impl.hpp

power_grid_model_c/power_grid_model/include/power_grid_model/common/multi_threaded_logging.hpp

figueroa1395 · 2025-09-02T08:58:26Z

power_grid_model_c/power_grid_model/include/power_grid_model/job_dispatch.hpp

+    template <typename Adapter, typename ResultDataset, typename UpdateDataset>
+        requires std::is_base_of_v<JobInterface<Adapter>, Adapter>
+    static BatchParameter batch_calculation(Adapter& adapter, ResultDataset const& result_data,
+                                            UpdateDataset const& update_data, Idx threading) {
+        common::logging::NoMultiThreadedLogger no_log{};
+        return batch_calculation(adapter, result_data, update_data, threading,
+                                 static_cast<common::logging::MultiThreadedLogger&>(no_log));
+    }
+    template <typename Adapter, typename ResultDataset, typename UpdateDataset>
+        requires std::is_base_of_v<JobInterface<Adapter>, Adapter>
+    static BatchParameter batch_calculation(Adapter& adapter, ResultDataset const& result_data,
+                                            UpdateDataset const& update_data) {
+        return batch_calculation(adapter, result_data, update_data, sequential);
+    }


IMO we should move to one single line for these. i can clean up in this PR if you prefer, or wait a little bit.

I also think, e.g., that MainModelImpl should only have one constructor

We can do these changes in a follow up PR. Let's keep this one self contained with the dependency injection via the new logger stuff.

Leaving this one open as a reminder for a follow up. Feel free to resolve.

power_grid_model_c/power_grid_model/include/power_grid_model/job_dispatch.hpp

power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp

power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp

power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp

Signed-off-by: Martijn Govers <[email protected]>

…-info Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>

…-info

figueroa1395 · 2025-09-05T07:49:40Z

power_grid_model_c/power_grid_model/include/power_grid_model/job_dispatch.hpp

+                                            UpdateDataset const& update_data, Idx threading,
+                                            common::logging::MultiThreadedLogger& log) {


Side note, but it may be a good idea to update our internal UML around job_dispatch with this logger dependency (and include the logger stuff into it). I mean a simple version, just to show progress there.

figueroa1395 · 2025-09-05T08:00:58Z

tests/cpp_unit_tests/test_job_dispatch.cpp

 };

 class SomeTestException : public std::runtime_error {
  public:
    using std::runtime_error::runtime_error;
 };
+
+class TestLogger : public common::logging::Logger {


Maybe this is a next step, but I don't think TestLogger belongs here. I'd say we put it at the same level as NoLogger (or something similar), and then use it here but latter on also to unit test the logger stuff... As in our own loggers should be directly tested, but the multithreaded ones and the interface (as we expect users to make their own loggers - theoretically) should be tested through something like this. Just food for thought.

figueroa1395

Gave it another review and LGTM. Remaining comments can either be addressed in #1115 or are minor questions. Feel free to resolve them.

mgovers added 5 commits August 27, 2025 13:28

main model logs by reference

fa6808a

Signed-off-by: Martijn Govers <[email protected]>

implement threaded logger

87057e3

Signed-off-by: Martijn Govers <[email protected]>

logger seems to work

9962299

Signed-off-by: Martijn Govers <[email protected]>

Merge remote-tracking branch 'origin/main' into feature/dependency-in…

8462283

…version-main-model-impl-info Signed-off-by: Martijn Govers <[email protected]>

minor

1cec998

Signed-off-by: Martijn Govers <[email protected]>

mgovers requested review from Jerry-Jinfeng-Guo, figueroa1395, nitbharambe and Copilot September 1, 2025 14:06

mgovers self-assigned this Sep 1, 2025

mgovers added the do-not-merge This should not be merged label Sep 1, 2025

Copilot AI reviewed Sep 1, 2025

View reviewed changes

mgovers and others added 4 commits September 2, 2025 08:09

Update tests/cpp_unit_tests/test_job_dispatch.cpp

0fe5288

Signed-off-by: Martijn Govers <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

Update power_grid_model_c/power_grid_model/include/power_grid_model/c…

85280c2

…ommon/multi_threaded_logging.hpp Signed-off-by: Martijn Govers <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

Update power_grid_model_c/power_grid_model/include/power_grid_model/j…

b60c807

…ob_dispatch.hpp Signed-off-by: Martijn Govers <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

cleanup

53f80f4

Signed-off-by: Martijn Govers <[email protected]>

mgovers commented Sep 2, 2025

View reviewed changes

tests/cpp_unit_tests/test_job_dispatch.cpp Outdated Show resolved Hide resolved

mgovers and others added 3 commits September 2, 2025 08:53

Apply suggestion from @mgovers

45bbcbb

Signed-off-by: Martijn Govers <[email protected]> Signed-off-by: Martijn Govers <[email protected]>

remove unused parameter

0cfed9e

Signed-off-by: Martijn Govers <[email protected]>

more unused captures

7496e27

Signed-off-by: Martijn Govers <[email protected]>

mgovers marked this pull request as ready for review September 2, 2025 07:56

mgovers added feature New feature or request and removed do-not-merge This should not be merged labels Sep 2, 2025

mgovers and others added 5 commits September 2, 2025 10:29

clang-tidy

9086696

Signed-off-by: Martijn Govers <[email protected]>

Merge branch 'main' into feature/dependency-inversion-main-model-impl…

b0156a2

…-info Signed-off-by: Martijn Govers <[email protected]>

fix msvc

3d99233

Signed-off-by: Martijn Govers <[email protected]>

fix msvc

28d06aa

Signed-off-by: Martijn Govers <[email protected]>

fix clang-cl... it never ends

6fab6c1

Signed-off-by: Martijn Govers <[email protected]>

fix gcc + clang-cl tests

060d3f1

Signed-off-by: Martijn Govers <[email protected]>

nitbharambe reviewed Sep 2, 2025

View reviewed changes

figueroa1395 reviewed Sep 2, 2025

View reviewed changes

mgovers and others added 7 commits September 2, 2025 14:17

fix dangling reference

9ad7b8b

Signed-off-by: Martijn Govers <[email protected]>

resolve comments

d348ff7

Signed-off-by: Martijn Govers <[email protected]>

more resolve comments

1a20b38

Signed-off-by: Martijn Govers <[email protected]>

more resolve comments

ec8490b

Signed-off-by: Martijn Govers <[email protected]>

attempt to fix macos

93dff85

Signed-off-by: Martijn Govers <[email protected]>

Merge branch 'main' into feature/dependency-inversion-main-model-impl…

3bb015e

…-info Signed-off-by: Martijn Govers <[email protected]>

merge_into as member function

8026cea

Signed-off-by: Martijn Govers <[email protected]>

mgovers mentioned this pull request Sep 4, 2025

Clean-up main model: pass logger by reference to main model #1115

Draft

Merge branch 'main' into feature/dependency-inversion-main-model-impl…

2c7341c

…-info

figueroa1395 reviewed Sep 5, 2025

View reviewed changes

		UpdateDataset const& update_data, Idx threading,
		common::logging::MultiThreadedLogger& log) {

Cleanup main model: dependency inversion main model impl info #1107

Are you sure you want to change the base?

Cleanup main model: dependency inversion main model impl info #1107

Uh oh!

Conversation

mgovers commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgovers commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitbharambe Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

figueroa1395 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgovers commented Sep 1, 2025 •

edited

Loading

nitbharambe Sep 2, 2025 •

edited

Loading