From 34119575c217fe31dd754f74adae904d0f4fad2c Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Thu, 27 Aug 2026 12:25:55 +0200 Subject: [PATCH] [df] Avoid side-effects if external package interacts with distributed module at exit time The distributed RDataFrame Python module raises a warning to let users know it has been moved out of the `Experimental` namespace. This was implemented by creating a dummy Python class type that injects the warning at `__getattribute__` time and using that class as the class of the injected distributed module when the user accesses it via `Experimental`. That implementation may have undesired interactions in extreme edge-cases. For example, if a third-party library accesses available Python modules during the session and inspects them, the distributed Python module warning could be triggered. One notable example are the XRootD Python bindings. The `client` module of this package registers a function with `atexit`. At exit time, that function accesses all objects available to the garbage collector, which includes all Python modules (even external to XRootD itself). See the implementation at: https://github.com/xrootd/xrootd/blob/v6.1.1/python/libs/client/finalize.py While accessing all these objects, XRootD is also accessing ROOT modules if ROOT was imported in the same application. As such, the dummy class behaviour explained above kicked in. This commit proposes to change the implementation that raises the warning to use a mechanism based on function decorators. The downside is a few more lines of code: every function supposed to be accessed publicly of the distributed module must be decorated so that the warning is raised before the function runs if the user is accessing it via `Experimental`. The upside is avoiding the scenario described above and having the warning raised right at the point of calling the function rather than slightly earlier during module attribute access. Co-authored-by: Silia Taider --- .../python/DistRDF/Backends/__init__.py | 7 +- bindings/distrdf/python/DistRDF/__init__.py | 76 ++++++++++--------- .../distrdf/backends/check_explicit_api.py | 62 ++++++++++----- 3 files changed, 86 insertions(+), 59 deletions(-) diff --git a/bindings/distrdf/python/DistRDF/Backends/__init__.py b/bindings/distrdf/python/DistRDF/Backends/__init__.py index 4c5ddb10c6144..10e78c329b5ca 100644 --- a/bindings/distrdf/python/DistRDF/Backends/__init__.py +++ b/bindings/distrdf/python/DistRDF/Backends/__init__.py @@ -16,12 +16,13 @@ import types -def build_backends_submodules(parentmodule: types.ModuleType) -> types.ModuleType: +def build_backends_submodules(parentmodule: types.ModuleType, experimental: bool) -> types.ModuleType: """ Helper function to create the submodules of the backends. """ - for _, module_name, is_pkg in pkgutil.walk_packages(__path__): + from ROOT._distrdf import _raise_warning_if_experimental + for _, module_name, is_pkg in pkgutil.walk_packages(__path__): if is_pkg: # The actual python package with the backend implementation actual = importlib.import_module(__name__ + "." + module_name) @@ -37,7 +38,7 @@ def build_backends_submodules(parentmodule: types.ModuleType) -> types.ModuleTyp dummy.__package__ = parentmodule # Attached functions - dummy.RDataFrame = actual.RDataFrame + dummy.RDataFrame = _raise_warning_if_experimental(actual.RDataFrame, experimental) setattr(parentmodule, module_name, dummy) diff --git a/bindings/distrdf/python/DistRDF/__init__.py b/bindings/distrdf/python/DistRDF/__init__.py index 01bc54eb139b7..b00bce8f65727 100644 --- a/bindings/distrdf/python/DistRDF/__init__.py +++ b/bindings/distrdf/python/DistRDF/__init__.py @@ -12,12 +12,13 @@ from __future__ import annotations import concurrent.futures +import functools import importlib.util import logging import textwrap import types import warnings -from typing import TYPE_CHECKING, Iterable +from typing import TYPE_CHECKING, Callable, Iterable from .Backends import build_backends_submodules from .LiveVisualize import LiveVisualize @@ -208,29 +209,33 @@ def FromSpec(jsonfile: str, *args, **kwargs) -> RDataFrame: ) -class _DeprecatedModule(types.ModuleType): - """A simple module type to raise a warning before usage.""" - - def __getattribute__(self, name): - msg_warng = textwrap.dedent( - """ - In ROOT 6.36, the ROOT.RDF.Experimental.Distributed module has become just ROOT.RDF.Distributed. In the - future, the 'Experimental' keyword will be removed, so it is suggested to move to the stable API in user - code. You can now change lines such as: - ``` - connection = ... # your distributed Dask client or SparkContext - RDataFrame = ROOT.RDF.Experimental.Distributed.[Backend].RDataFrame - df = RDataFrame(..., [daskclient,sparkcontext] = connection) - ``` - to simply: - ``` - connection = ... # your distributed Dask client or SparkContext - df = ROOT.RDataFrame(..., executor = connection) - ``` - """ - ) - warnings.warn(msg_warng, FutureWarning) - return super().__getattribute__(name) +def _raise_warning_if_experimental(func: Callable, experimental: bool): + """A simple function decorator to raise a warning before usage.""" + + @functools.wraps(func) + def wrapper(*args, **kwargs): + if experimental: + msg_warng = textwrap.dedent( + """ + In ROOT 6.36, the ROOT.RDF.Experimental.Distributed module has become just ROOT.RDF.Distributed. In the + future, the 'Experimental' keyword will be removed, so it is suggested to move to the stable API in user + code. You can now change lines such as: + ``` + connection = ... # your distributed Dask client or SparkContext + RDataFrame = ROOT.RDF.Experimental.Distributed.[Backend].RDataFrame + df = RDataFrame(..., [daskclient,sparkcontext] = connection) + ``` + to simply: + ``` + connection = ... # your distributed Dask client or SparkContext + df = ROOT.RDataFrame(..., executor = connection) + ``` + """ + ) + warnings.warn(msg_warng, FutureWarning, stacklevel=2) + return func(*args, **kwargs) + + return wrapper def create_distributed_module(parentmodule, experimental: bool = False): @@ -249,21 +254,18 @@ def create_distributed_module(parentmodule, experimental: bool = False): # distributed.__loader__ is not defined distributed.__package__ = parentmodule - distributed = build_backends_submodules(distributed) + distributed = build_backends_submodules(distributed, experimental) # Inject top-level functions - distributed.initialize = initialize - distributed.RunGraphs = RunGraphs - distributed.VariationsFor = VariationsFor - distributed.LiveVisualize = LiveVisualize - distributed.DistributeHeaders = DistributeHeaders - distributed.DistributeFiles = DistributeFiles - distributed.DistributeSharedLibs = DistributeSharedLibs - distributed.DistributeCppCode = DistributeCppCode - distributed.FromSpec = FromSpec - - if experimental: - distributed.__class__ = _DeprecatedModule + distributed.initialize = _raise_warning_if_experimental(initialize, experimental) + distributed.RunGraphs = _raise_warning_if_experimental(RunGraphs, experimental) + distributed.VariationsFor = _raise_warning_if_experimental(VariationsFor, experimental) + distributed.LiveVisualize = _raise_warning_if_experimental(LiveVisualize, experimental) + distributed.DistributeHeaders = _raise_warning_if_experimental(DistributeHeaders, experimental) + distributed.DistributeFiles = _raise_warning_if_experimental(DistributeFiles, experimental) + distributed.DistributeSharedLibs = _raise_warning_if_experimental(DistributeSharedLibs, experimental) + distributed.DistributeCppCode = _raise_warning_if_experimental(DistributeCppCode, experimental) + distributed.FromSpec = _raise_warning_if_experimental(FromSpec, experimental) return distributed diff --git a/roottest/python/distrdf/backends/check_explicit_api.py b/roottest/python/distrdf/backends/check_explicit_api.py index cbd5908418d43..0ab50748ea855 100644 --- a/roottest/python/distrdf/backends/check_explicit_api.py +++ b/roottest/python/distrdf/backends/check_explicit_api.py @@ -91,7 +91,25 @@ def test_redefine_one_column(self, payload): class TestDeprecation: """Test the deprecation message regarding the 'Experimental' module""" - def test_warning_message(self, payload): + MODULE_WARN = textwrap.dedent( + """ + In ROOT 6.36, the ROOT.RDF.Experimental.Distributed module has become just ROOT.RDF.Distributed. In the + future, the 'Experimental' keyword will be removed, so it is suggested to move to the stable API in user + code. You can now change lines such as: + ``` + connection = ... # your distributed Dask client or SparkContext + RDataFrame = ROOT.RDF.Experimental.Distributed.[Backend].RDataFrame + df = RDataFrame(..., [daskclient,sparkcontext] = connection) + ``` + to simply: + ``` + connection = ... # your distributed Dask client or SparkContext + df = ROOT.RDataFrame(..., executor = connection) + ``` + """ + ) + + def test_experimental_module_deprecation(self, payload): connection, backend = payload with warnings.catch_warnings(record=True) as warninglist: if backend == "dask": @@ -101,26 +119,32 @@ def test_warning_message(self, payload): RDataFrame = ROOT.RDF.Experimental.Distributed.Spark.RDataFrame df = RDataFrame(10, npartitions=2, executor=connection) - msg_warng = textwrap.dedent( - """ - In ROOT 6.36, the ROOT.RDF.Experimental.Distributed module has become just ROOT.RDF.Distributed. In the - future, the 'Experimental' keyword will be removed, so it is suggested to move to the stable API in user - code. You can now change lines such as: - ``` - connection = ... # your distributed Dask client or SparkContext - RDataFrame = ROOT.RDF.Experimental.Distributed.[Backend].RDataFrame - df = RDataFrame(..., [daskclient,sparkcontext] = connection) - ``` - to simply: - ``` - connection = ... # your distributed Dask client or SparkContext - df = ROOT.RDataFrame(..., executor = connection) - ``` - """ - ) assert len(warninglist) == 1, f"{warninglist}" assert issubclass(warninglist[0].category, FutureWarning) - assert str(warninglist[0].message == msg_warng) + assert str(warninglist[0].message == self.MODULE_WARN) + + assert df.Count().GetValue() == 10 + + def test_backend_argument_deprecation(self, payload): + connection, backend = payload + with pytest.warns() as warninglist: + if backend == "dask": + RDataFrame = ROOT.RDF.Experimental.Distributed.Dask.RDataFrame + df = RDataFrame(10, npartitions=2, daskclient=connection) + elif backend == "spark": + RDataFrame = ROOT.RDF.Experimental.Distributed.Spark.RDataFrame + df = RDataFrame(10, npartitions=2, sparkcontext=connection) + + keyword = "daskclient" if backend == "dask" else "sparkcontext" + keyword_warn = ( + f"The keyword argument '{keyword}' is not necessary anymore and will be removed in a future release" + ) + + assert len(warninglist) == 2, f"{warninglist}" + assert issubclass(warninglist[0].category, FutureWarning) + assert str(warninglist[0].message == self.MODULE_WARN) + assert issubclass(warninglist[1].category, FutureWarning) + assert str(warninglist[1].message == keyword_warn) assert df.Count().GetValue() == 10