Refactor DeferredLogger as a mutable pointer into GroupStateHelper by hakonhagland · Pull Request #6705 · OPM/opm-simulators

hakonhagland · 2025-12-29T03:11:29Z

Refactor DeferredLogger usage in the GroupStateHelper class by integrating it as a mutable member of GroupStateHelper with RAII-based scoped setup. This simplifies method signatures throughout the codebase by removing the need to pass DeferredLogger& to every method that needs logging.

hakonhagland · 2025-12-29T03:11:43Z

jenkins build this please

hakonhagland · 2025-12-31T07:26:48Z

jenkins build this please

hakonhagland · 2026-01-01T06:00:53Z

jenkins build this please

akva2 · 2026-01-02T13:53:28Z

i'd be careful about doing this change. the passing of loggers is, while annoying, a feature. there are some spots where we need multiple loggers in flight to handle recursive stuff for wells et al, and there is no guarantee all these cases are exercised in the test suite.

hakonhagland · 2026-01-02T15:54:41Z

there are some spots where we need multiple loggers in flight to handle recursive stuff for wells

@akva2 Can you show an example?

akva2 · 2026-01-05T07:14:31Z

I'm sorry, I can't really pinpoint it, and it's a bit hard to look for. The fact that you have already changed this stuff in previous PRs makes it even harder to track. I just remember there has been PRs and there has been code where a def_logger and a local_def_logger was used to fix such issues. If you are confident that it's okay, it's probably okay, it was just a heads up that you really need to make sure you don't end up reintroducing these issues.

atgeirr · 2026-01-05T08:04:05Z

    const Parallel::Communication& comm() const { return this->comm_; }

+    /// @brief Get the deferred logger
+    /// @throws assertion failure if no logger has been set via setupScopedDeferredLogger()


Not strictly true, I suggest making it true: instead of assert() make it a regular if and throw.

Correct, I have updated the PR with a commit to address this. I also added two commits to address the recursive creation of temporary loggers that @akva2 made me aware of and he has approved those. Does this look ok to you?

hakonhagland · 2026-01-05T09:31:01Z

jenkins build this please

hakonhagland · 2026-01-06T05:32:47Z

i'd be careful about doing this change. the passing of loggers is, while annoying, a feature. there are some spots where we need multiple loggers in flight to handle recursive stuff for wells et al, and there is no guarantee all these cases are exercised in the test suite.

@akva2 Indeed, I found a place at https://github.com/OPM/opm-simulators/blob/master/opm/simulators/wells/BlackoilWellModel_impl.hpp#L1832. I will have to think about this, so I will put the PR in draft state to avoid merging for now.

hakonhagland · 2026-01-06T12:00:13Z

jenkins build this please

hakonhagland · 2026-01-06T12:27:22Z

jenkins build this please

hakonhagland · 2026-01-06T13:47:32Z

there are some spots where we need multiple loggers in flight to handle recursive stuff for wells

@akva2 See the last two commits: This should work now. You can now push as many deferred loggers as you want. We could consider cleaning up method calls where both groupStateHelper and deferredLogger are passed such that we only pass groupStateHelper (no need to pass both) in a follow-up PR, see e.g. https://github.com/OPM/opm-simulators/blob/master/opm/simulators/wells/BlackoilWellModel_impl.hpp#L1832

akva2

This is kinda hard to review in terms of regressions and/or change behavior, but the principles seems fine to me.

hakonhagland · 2026-01-07T08:26:12Z

jenkins build this please

Introduce a RAII-based mechanism for binding a DeferredLogger pointer to GroupStateHelper, enabling cleaner method signatures by avoiding repeated parameter passing. Changes: - Add ScopedDeferredLoggerGuard nested class with move semantics - Add setupScopedDeferredLogger() factory method and deferredLogger() accessor - Add mutable DeferredLogger* member (logical constness for logging) - GuideRateHandler now delegates to GroupStateHelper for logger access - Remove setLogger() calls from BlackoilWellModel_impl.hpp This is a first step toward simplifying the DeferredLogger parameter threading throughout the blackoil well model module.

Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

…rod() Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

…et() Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

…ctor() Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

…ucer() Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

…ells() Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

hakonhagland · 2026-01-10T06:37:42Z

jenkins build this please

hakonhagland · 2026-01-10T09:26:59Z

jenkins build this please

hakonhagland · 2026-01-10T10:12:01Z

@akva2 The jenkins build reports an error that seems unrelated to this PR: CUDA ERROR (code = 35, CUDA driver version is insufficient for CUDA runtime version). Any ideas?

akva2 · 2026-01-10T10:15:54Z

Irrelevant, just that we build against cuda but there is no gpu. Sendt fra Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Håkon Hægland ***@***.***> Sent: Saturday, January 10, 2026 11:12:23 AM To: OPM/opm-simulators ***@***.***> Cc: Arne Morten Kvarving ***@***.***>; Mention ***@***.***> Subject: Re: [OPM/opm-simulators] Refactor DeferredLogger as a mutable pointer into GroupStateHelper (PR #6705) [https://avatars.githubusercontent.com/u/6708379?s=20&v=4]hakonhagland left a comment (OPM/opm-simulators#6705)<#6705 (comment)> @akva2<https://github.com/akva2> The jenkins build reports an error that seems unrelated to this PR: CUDA ERROR (code = 35, CUDA driver version is insufficient for CUDA runtime version)<https://ci.opm-project.org/job/opm-simulators-PR-builder/9303/testReport/junit/(root)/default/runSimulator_spe1case1_carfin_gr/>. Any ideas? — Reply to this email directly, view it on GitHub<#6705 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAQLQR6GSSOD7A2U2E5E2UT4GDGAPAVCNFSM6AAAAACQGNTT22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOMZSGMZTAMBSHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

hakonhagland · 2026-01-12T05:37:55Z

jenkins build this please

We need the move constructor in ScopedLoggerGuard since pushLogger() returns it by value.

Clarify why ScopedLoggerGuard needs to store a pointer to GroupStateHelper (and cannot store a reference)

Reduce scope of logger guard to log messages before the calls to OpmLog::debug() on rank 0 at the end of getWellConvergence().

hakonhagland · 2026-01-14T05:28:18Z

jenkins build this please

hakonhagland · 2026-01-14T05:50:46Z

jenkins build this please

…eter Methods that receive GroupStateHelper can obtain the DeferredLogger via groupStateHelper.deferredLogger(), making the separate parameter redundant.

hakonhagland · 2026-01-14T06:23:44Z

jenkins build this please

hakonhagland · 2026-01-14T07:31:20Z

jenkins build this please

When looping over well container some ranks may never enter the loop, so logger guard needs to be created outside the loop

hakonhagland · 2026-01-14T08:13:09Z

jenkins build this please

Don't create extra deferred loggers that do not match the logger in groupStateHelper

hakonhagland · 2026-01-14T08:51:25Z

jenkins build this please

hakonhagland · 2026-01-14T09:20:30Z

@akva2 and @atgeirr I have updated the PR with commits after private discussion with @atgeirr. Please have a look again and let me know if anything can be improved.

hakonhagland · 2026-01-15T07:43:33Z

jenkins build this please

Use groupStateHelper logger instead of creating an explicit logger.

hakonhagland · 2026-01-15T08:11:54Z

jenkins build this please

atgeirr · 2026-01-16T15:04:26Z

I think this is good now, merging!

hakonhagland added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Dec 29, 2025

hakonhagland force-pushed the deflog branch from 4367c55 to f38dd5e Compare December 31, 2025 07:25

atgeirr reviewed Jan 5, 2026

View reviewed changes

hakonhagland force-pushed the deflog branch from 24df6e6 to e8b53d8 Compare January 5, 2026 09:30

hakonhagland marked this pull request as draft January 6, 2026 05:33

hakonhagland force-pushed the deflog branch from 90cf6a6 to 5f8a77c Compare January 6, 2026 12:26

hakonhagland marked this pull request as ready for review January 6, 2026 13:47

hakonhagland requested review from akva2 and atgeirr January 6, 2026 13:47

akva2 approved these changes Jan 7, 2026

View reviewed changes

Comment thread opm/simulators/wells/GroupStateHelper.hpp Outdated

Comment thread opm/simulators/wells/GroupStateHelper.hpp

hakonhagland added 8 commits January 10, 2026 07:34

Refactor: Remove DeferredLogger parameter from checkGroupConstraintsInj

0bb1fb9

Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

Refactor: Remove DeferredLogger parameter from checkGroupConstraintsP…

aacf753

…rod() Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

Refactor: Remove DeferredLogger parameter from getInjectionGroupTarget()

3c7abc9

Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

Refactor: Remove DeferredLogger parameter from getProductionGroupTarg…

cb51773

…et() Use GroupStateHelper's stored deferred logger instead of passing it as a parameter. This simplifies the method signature and call sites. Part of the DeferredLogger refactoring series for GroupStateHelper.

Adapt test case to not pass deferred logger

3edabe4

hakonhagland added 3 commits January 13, 2026 16:46

Clarify why we need the move constructor

8fa67b3

We need the move constructor in ScopedLoggerGuard since pushLogger() returns it by value.

Clarify why we need to store a pointer

090971f

Clarify why ScopedLoggerGuard needs to store a pointer to GroupStateHelper (and cannot store a reference)

Reduce scope of logger guard

6aa2bb9

Reduce scope of logger guard to log messages before the calls to OpmLog::debug() on rank 0 at the end of getWellConvergence().

hakonhagland force-pushed the deflog branch from c75f0e4 to ecfd06d Compare January 14, 2026 05:50

Simplify method signatures by removing redundant DeferredLogger param…

3999a59

…eter Methods that receive GroupStateHelper can obtain the DeferredLogger via groupStateHelper.deferredLogger(), making the separate parameter redundant.

hakonhagland force-pushed the deflog branch from ecfd06d to 3999a59 Compare January 14, 2026 06:23

Add do_mpi_gather parameter to pushLogger() for NLDD support

cf3a8b5

Don't create guard in loop which ranks will never enter

9a93f9c

When looping over well container some ranks may never enter the loop, so logger guard needs to be created outside the loop

Always extract the logger from groupStateHelper

e5e589f

Don't create extra deferred loggers that do not match the logger in groupStateHelper

Remove unused loggers

4b0d7de

hakonhagland force-pushed the deflog branch from cae83ec to 4b0d7de Compare January 15, 2026 08:06

Use group state helper logger

30fcf3b

Use groupStateHelper logger instead of creating an explicit logger.

atgeirr merged commit cdc61ce into OPM:master Jan 16, 2026
2 checks passed

hakonhagland deleted the deflog branch March 16, 2026 09:03

Conversation

hakonhagland commented Dec 29, 2025

Uh oh!

hakonhagland commented Dec 29, 2025

Uh oh!

hakonhagland commented Dec 31, 2025

Uh oh!

hakonhagland commented Jan 1, 2026

Uh oh!

akva2 commented Jan 2, 2026

Uh oh!

hakonhagland commented Jan 2, 2026

Uh oh!

akva2 commented Jan 5, 2026

Uh oh!

atgeirr Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

hakonhagland Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

hakonhagland commented Jan 5, 2026

Uh oh!

hakonhagland commented Jan 6, 2026

Uh oh!

hakonhagland commented Jan 6, 2026

Uh oh!

hakonhagland commented Jan 6, 2026

Uh oh!

hakonhagland commented Jan 6, 2026

Uh oh!

akva2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hakonhagland commented Jan 7, 2026

Uh oh!

hakonhagland commented Jan 10, 2026

Uh oh!

hakonhagland commented Jan 10, 2026

Uh oh!

hakonhagland commented Jan 10, 2026

Uh oh!

akva2 commented Jan 10, 2026 via email

Uh oh!

hakonhagland commented Jan 12, 2026

Uh oh!

hakonhagland commented Jan 14, 2026

Uh oh!

hakonhagland commented Jan 14, 2026

Uh oh!

hakonhagland commented Jan 14, 2026

Uh oh!

hakonhagland commented Jan 14, 2026

Uh oh!

hakonhagland commented Jan 14, 2026

Uh oh!

hakonhagland commented Jan 14, 2026

Uh oh!

hakonhagland commented Jan 14, 2026

Uh oh!

hakonhagland commented Jan 15, 2026

Uh oh!

hakonhagland commented Jan 15, 2026

Uh oh!

atgeirr commented Jan 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants