Extract logger from Context & renamed Global_context to Data_store by jbigot · Pull Request #752 · pdidev/pdi

jbigot · 2026-09-04T14:04:56Z

Context no longer exposes a logger() accessor.
Each plugin now receives its own PDI::Logger& explicitly, as an additional parameter on the PDI::Plugin constructor and can access it afterwards through the inherited Plugin::logger() accessor.
Every function that was handed a Context& to reach its logger now takes a Logger& alongside it.
Removed Context_proxy, which existed solely to hand each plugin its own child logger, a role now served directly by Plugin.
Renamed Global_context to Data_store
- Renamed Global_context to Data_store to match the name already used in the documentation.
- Renamed associated files from global_context* to data_store*.
- Renamed Data_store variables and functions named *context to *store.

Before merging your code, please check the following:

* `Context` no longer exposes a `logger()` accessor. * Each plugin now receives its own `PDI::Logger&` explicitly, as an additional parameter on the `PDI::Plugin` constructor and can access it afterwards through the inherited `Plugin::logger()` accessor. * Every function that was handed a `Context&` to reach its logger now takes a `Logger&` alongside it. * Removed `Context_proxy`, which existed solely to hand each plugin its own child logger, a role now served directly by `Plugin`. A step towards #721.

* Renamed `Global_context` to `Data_store` to match the name already used in the documentation. * Renamed associated files from `global_context*` to `data_store*`. * Renamed Data_store variables and functions named `*context` to `*store`. Anything typed as the abstract `Context` is untouched. Fix #721.

Yushan-Wang

Wow, it was many files changed!
Except a bit of naming consistency, I am OK with this PR

Yushan-Wang · 2026-09-07T08:52:24Z

 #include <pdi/pdi_fwd.h>
 #include <pdi/data_descriptor.h>
 #include <pdi/datatype_template.h>
 #include <pdi/logger.h>


Can be removed?

logger.h is unused, while data_descriptor.h and datatype_template.h are redundant of pdi_fwd.h ?

Indeed, logger.h was there for the accessor, and the accessor is gone. Good catch.

Regarding the other two, they are not equivalent to pdi_fwd.h that only declares types; the dedicated headers are the one that define the types.
Since only pointers or references are manipulated here, we could argue that it's up to the client to include them when de-referencing... This is up for debate. But this predates this PR and would break stuff in various places (reference_expression.cxx would not compile anymore for example).
I would keep it as-is for now, but it could be nice to add a separate issue, a rule in CONTRIBUTING.md, and tooling with something like iwyu to check that everything is according to the rule in the CI.

Yushan-Wang · 2026-09-07T09:13:38Z

-			throw Spectree_error{node, "Incorrect array ordering: `{}', only C order is supported", order_str};
+		{
+			string order_str = to_string(PC_get(node, ".order"), "");
+			if (order_str == "c" && order_str == "C") {


Suggested change

if (order_str == "c" && order_str == "C") {

if (order_str == "c" || order_str == "C") {

We can close #743

Wouldn't it be better to merge #743 , with a clean CHANGELOG entry, correct attribution etc. ? Then rebase this on top of it.

Yushan-Wang · 2026-09-07T09:15:51Z

 {
 	// holder types
-	ctx.add_datatype("array", to_array_datatype_template);
+	ctx.add_datatype("array", to_array_datatype_template(logger));


Shall we add logger to other types as well?
For the sake of being able to add logger information in the future?

I propose to completely drop the parameter from all templates for uniformity and to instead rely on a lambda to pass the logger when necessary. That way, we get a uniform API and no unused logger that might make contributors wonder why they're here, similarly to your comment on Dnc_io below.

JAuriac · 2026-09-10T13:59:32Z

 target_link_libraries(pdi_set_value_plugin PUBLIC PDI::PDI_plugins spdlog::spdlog)
+# logger_operation.cxx casts to PDI::Global_context to reach the root logger; that class is
+# library-internal (pdi/src/, not the public pdi/include/pdi/ API) and only reachable this way
+# when set_value is built as part of the PDI monorepo superbuild, not as a standalone plugin
+# against an installed PDI package.
+target_include_directories(pdi_set_value_plugin PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/../../pdi/src")
 set_target_properties(pdi_set_value_plugin PROPERTIES CXX_VISIBILITY_PRESET hidden)


Are we sure we want this ? Is it technically an anti-pattern, as set_value would not be functioning as a plugin but as a core component ? If we can instead modify PDI core, should we use something similar to global_pattern(), adding a Logger::global_level(spdlog::level::level_enum) or equivalent to the public API so that our plugin can access this "on its own" ?

You're completely right. This is a quick and dirty workaround; and the reason I left it as-is is because the plan is indeed to make set_value a core component, in the next step of #378 (Cf. #751 )

That being said, if we ship in-between this is not very clean. So we can go with the approach you propose for this step and revert it in the next one.

I tried to make it go unnoticed, but you were there to watch ;p Good catch!

jmorice91 · 2026-09-17T14:37:28Z

-	Plugin(Context& ctx);
+	Plugin(Logger& logger, Context& ctx);

 	virtual ~Plugin() noexcept(false);


Why this dtor can throw?

Because a plugin destructor is allowed to fail, and the failure has to be reportable rather than
fatal. Since C++11 a destructor is implicitly noexcept, and a derived destructor takes its
exception specification from the base, so without noexcept(false) on ~Plugin anything thrown
from any plugin destructor would be std::terminate() instead of an error.

Plugins do throw from there:

~decl_hdf5_plugin calls handle_hdf5_err("Cannot finalize HDF5 library") when H5close() fails;

~set_value_plugin runs the configured on_finalize triggers, i.e. arbitrary expose / share /
set operations, each of which can throw PDI::Error;

~pycall_plugin calls pybind11::finalize_interpreter().

The exception then propagates out of Data_store::finalize() to PDI_finalize(), whose
function-try-block turns it into a PDI_status_t for the application.

It is not something this PR introduces: the line is unchanged, and noexcept(false) has been on
~Plugin since c92f64c7 (2019). The PR only adds the logger parameter to the constructor above
it.

This question is unrelated to this PR.
My concern is about a feedback review for catalyst_plugin: the dtor must be specified as noexcept

catalyst_plugin::~catalyst_plugin() noexcept

With this default destructor and your answer, It seems for me that a plugin destructor marked with noexcept is not good choice. Am I mistaken?

It's a bad idea for a virtual destructor that defines the interface: whether implementations are allowed to throw. It's a good idea for an implementation to specify that it won't throw.

Co-authored-by: yushan wang <yushan.wang@cea.fr>

jmorice91

small fix is proposed.

General comment:
It is not clear for me when we needed to use logger instead of logger() in a plugins.
It would be nice to have a rule/recommendation for that to be sure that this PR verify this.
As you have done this huge work what is your recommendation between logger and logger()?

Ps: I'm not sure that the plugins serialize doesn't the same rule.

jmorice91 · 2026-09-21T09:27:12Z


 	/** Actually load the plugins
 	 * 
 	 * \param confs the configurations specifying the list of plugin paths & plugins to load


missing description of ctx in the parameter list

Suggested change

* \param confs the configurations specifying the list of plugin paths & plugins to load

* \param ctx the context

* \param confs the configurations specifying the list of plugin paths & plugins to load

jmorice91 · 2026-09-21T09:29:05Z

+		Stored_plugin(Plugin_store& store, std::string name, PC_tree_t conf);

 		/** Loads a plugin if not done yet and its pre-dependencies if required
 		 * \param plugins the list of all plugins (for dependencies)


Missing description ctx as parameter for ensure_loaded

Suggested change

* \param plugins the list of all plugins (for dependencies)

* \param ctx the context

* \param plugins the list of all plugins (for dependencies)

jmorice91 · 2026-09-21T10:31:01Z


 	public:
 		/** Pre-loads a plugin
 		 * \param ctx the context


No ctx is used in the ctor

Suggested change

jbigot · 2026-10-03T21:58:41Z

small fix is proposed.

General comment: It is not clear for me when we needed to use logger instead of logger() in a plugins. It would be nice to have a rule/recommendation for that to be sure that this PR verify this. As you have done this huge work what is your recommendation between logger and logger()?

In the constructor, a parameter named logger ill hide the member function of the same name. So in that case, it's either logger the easy choice, or this->logger() the (maybe) cleaner choice. I would say it is mostly a question of taste and I wouldn't enforce one or the other way. Maybe the best idea would be to not call the parameter logger, and use the member function to be coherent in the whole plugin implementation.

jbigot requested a review from a team September 4, 2026 14:04

jbigot requested review from Yushan-Wang and benoitmartin88 as code owners September 4, 2026 14:04

jbigot linked an issue Sep 4, 2026 that may be closed by this pull request

Extract Logger from Global_context #721

Open

jbigot requested a review from a team September 4, 2026 14:06

jbigot added 2 commits September 4, 2026 23:07

jbigot force-pushed the extract_logger branch from e573c79 to 09c951f Compare September 4, 2026 21:07

jbigot changed the title ~~Extract logger from Context~~ Extract logger from Context & renamed Global_context to Data_store Sep 6, 2026

Yushan-Wang reviewed Sep 7, 2026

View reviewed changes

Yushan-Wang mentioned this pull request Sep 10, 2026

Fix #552, adding output redirection #747

Draft

19 tasks done

JAuriac reviewed Sep 10, 2026

View reviewed changes

This comment was marked as duplicate.

Sign in to view

jmorice91 reviewed Sep 17, 2026

View reviewed changes

jbigot and others added 2 commits September 20, 2026 14:28

Apply suggestions from code review

1c0e75e

Co-authored-by: yushan wang <yushan.wang@cea.fr>

Update plugins/timer/timer.cxx

42433af

Co-authored-by: yushan wang <yushan.wang@cea.fr>

jmorice91 reviewed Sep 21, 2026

View reviewed changes

	if (order_str == "c" && order_str == "C") {
	if (order_str == "c" \|\| order_str == "C") {

	* \param confs the configurations specifying the list of plugin paths & plugins to load
	* \param ctx the context
	* \param confs the configurations specifying the list of plugin paths & plugins to load

	* \param plugins the list of all plugins (for dependencies)
	* \param ctx the context
	* \param plugins the list of all plugins (for dependencies)

Conversation

jbigot commented Sep 4, 2026

Uh oh!

Yushan-Wang left a comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

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!

jmorice91 left a comment • 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!

jmorice91 Sep 21, 2026 • 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!

jbigot commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmorice91 left a comment •

edited

Loading

jmorice91 Sep 21, 2026 •

edited

Loading