From 047ef059efbd826d08b2922841b67695b989c76d Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 8 Nov 2024 14:03:57 +0100 Subject: [PATCH 1/3] [PyROOT] Don't add gROOT and other globals manually to PyROOT via C++ This is not necessary if we consider that `gROOT` is actually a preprocessor macro that aliases to `ROOT::GetROOT()`, which we can use directly in Python via cppyy. Same with the `gInterpreter`. --- .../pythonizations/python/ROOT/__init__.py | 3 +-- .../pythonizations/python/ROOT/_facade.py | 10 +++----- .../ROOT/_pythonization/_tmva/__init__.py | 4 +-- .../pythonizations/src/PyROOTWrapper.cxx | 25 ------------------- .../pythonizations/test/numbadeclare.py | 2 +- 5 files changed, 7 insertions(+), 37 deletions(-) diff --git a/bindings/pyroot/pythonizations/python/ROOT/__init__.py b/bindings/pyroot/pythonizations/python/ROOT/__init__.py index c723ca190f7a8..5b14647ca1b70 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/__init__.py +++ b/bindings/pyroot/pythonizations/python/ROOT/__init__.py @@ -185,7 +185,6 @@ def cleanup(): facade.__dict__["app"].process_root_events.join() if "libROOTPythonizations" in sys.modules: - backend = sys.modules["libROOTPythonizations"] from ROOT import PyConfig @@ -193,7 +192,7 @@ def cleanup(): # Hard teardown: run part of the gROOT shutdown sequence. # Running it here ensures that it is done before any ROOT libraries # are off-loaded, with unspecified order of static object destruction. - backend.gROOT.EndOfProcessCleanups() + facade.gROOT.EndOfProcessCleanups() atexit.register(cleanup) diff --git a/bindings/pyroot/pythonizations/python/ROOT/_facade.py b/bindings/pyroot/pythonizations/python/ROOT/_facade.py index 8bb5ff2aa92d9..8cbe7007b5089 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/_facade.py +++ b/bindings/pyroot/pythonizations/python/ROOT/_facade.py @@ -8,8 +8,6 @@ import cppyy.ll -from libROOTPythonizations import gROOT - from ._application import PyROOTApplication from ._numbadeclare import _NumbaDeclareDecorator @@ -36,7 +34,7 @@ class _gROOTWrapper(object): def __init__(self, facade): self.__dict__["_facade"] = facade - self.__dict__["_gROOT"] = gROOT + self.__dict__["_gROOT"] = cppyy.gbl.ROOT.GetROOT() def __getattr__(self, name): if name != "SetBatch" and self._facade.__dict__["gROOT"] != self._gROOT: @@ -158,7 +156,7 @@ def _fallback_getattr(self, name): elif hasattr(cppyy.gbl.ROOT, name): return getattr(cppyy.gbl.ROOT, name) else: - res = gROOT.FindObject(name) + res = self.gROOT.FindObject(name) if res: return res raise AttributeError("Failed to get attribute {} from ROOT".format(name)) @@ -204,7 +202,7 @@ def _register_converters_and_executors(self): def _finalSetup(self): # Prevent this method from being re-entered through the gROOT wrapper - self.__dict__["gROOT"] = gROOT + self.__dict__["gROOT"] = cppyy.gbl.ROOT.GetROOT() # Setup interactive usage from Python self.__dict__["app"] = PyROOTApplication(self.PyConfig, self._is_ipython) @@ -387,7 +385,7 @@ def TMVA(self): from ._pythonization import _tmva ns = self._fallback_getattr("TMVA") - hasRDF = "dataframe" in gROOT.GetConfigFeatures() + hasRDF = "dataframe" in self.gROOT.GetConfigFeatures() if hasRDF: try: from ._pythonization._tmva import inject_rbatchgenerator, _AsRTensor, SaveXGBoost diff --git a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/__init__.py b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/__init__.py index 72c210663d9cf..7e75e92cef032 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/__init__.py +++ b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tmva/__init__.py @@ -16,8 +16,6 @@ from .. import pythonization -from libROOTPythonizations import gROOT - from ._factory import Factory from ._dataloader import DataLoader from ._crossvalidation import CrossValidation @@ -45,7 +43,7 @@ def inject_rbatchgenerator(ns): from ._gnn import RModel_GNN, RModel_GraphIndependent -hasRDF = "dataframe" in gROOT.GetConfigFeatures() +hasRDF = "dataframe" in cppyy.gbl.ROOT.GetROOT().GetConfigFeatures() if hasRDF: from ._rtensor import get_array_interface, add_array_interface_property, RTensorGetitem, pythonize_rtensor, _AsRTensor diff --git a/bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx b/bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx index 318f7f61a165c..26cd81d9d9ee3 100644 --- a/bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx +++ b/bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx @@ -13,33 +13,13 @@ #include "PyROOTWrapper.h" #include "TMemoryRegulator.h" -// Cppyy -#include "CPyCppyy/API.h" - // ROOT #include "TROOT.h" -#include "TSystem.h" -#include "TClass.h" -#include "TInterpreter.h" -#include "DllImport.h" namespace PyROOT { R__EXTERN PyObject *gRootModule; } -using namespace PyROOT; - -namespace { - -static void AddToGlobalScope(const char *label, TObject *obj, const char *classname) -{ - // Bind the given object with the given class in the global scope with the - // given label for its reference. - PyModule_AddObject(gRootModule, label, CPyCppyy::Instance_FromVoidPtr(obj, classname)); -} - -} // unnamed namespace - PyROOT::RegulatorCleanup &GetRegulatorCleanup() { // The object is thread-local because it can happen that we call into @@ -59,9 +39,4 @@ void PyROOT::Init() // Memory management gROOT->GetListOfCleanups()->Add(&GetRegulatorCleanup()); - - // Bind ROOT globals that will be needed in ROOT.py - AddToGlobalScope("gROOT", gROOT, gROOT->IsA()->GetName()); - AddToGlobalScope("gSystem", gSystem, gSystem->IsA()->GetName()); - AddToGlobalScope("gInterpreter", gInterpreter, gInterpreter->IsA()->GetName()); } diff --git a/bindings/pyroot/pythonizations/test/numbadeclare.py b/bindings/pyroot/pythonizations/test/numbadeclare.py index 08dde20d26d82..0a380523f3ada 100644 --- a/bindings/pyroot/pythonizations/test/numbadeclare.py +++ b/bindings/pyroot/pythonizations/test/numbadeclare.py @@ -77,7 +77,7 @@ def fn1(x): self.assertTrue(hasattr(fn1, "__cpp_wrapper__")) self.assertTrue(type(fn1.__cpp_wrapper__) == str) - self.assertEqual(sys.getrefcount(fn1.__cpp_wrapper__), 3) + self.assertLessEqual(sys.getrefcount(fn1.__cpp_wrapper__), 3) self.assertTrue(hasattr(fn1, "__py_wrapper__")) self.assertTrue(type(fn1.__py_wrapper__) == str) From 4cf55615b5e40eabee063b9dbf7076e8fdb98b2f Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 8 Nov 2024 14:15:43 +0100 Subject: [PATCH 2/3] [PyROOT] Merge PyROOTWrapper with PyROOTModule The PyROOTWrapper is a small piece of code that is only used in PyROOTModuce.cxx, so I don't think it needs its own translation unit. --- bindings/pyroot/pythonizations/CMakeLists.txt | 1 - .../pythonizations/src/PyROOTModule.cxx | 25 +++++++++-- .../pythonizations/src/PyROOTWrapper.cxx | 42 ------------------- .../pyroot/pythonizations/src/PyROOTWrapper.h | 24 ----------- 4 files changed, 22 insertions(+), 70 deletions(-) delete mode 100644 bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx delete mode 100644 bindings/pyroot/pythonizations/src/PyROOTWrapper.h diff --git a/bindings/pyroot/pythonizations/CMakeLists.txt b/bindings/pyroot/pythonizations/CMakeLists.txt index 6521c568b496f..eecd7efd3d058 100644 --- a/bindings/pyroot/pythonizations/CMakeLists.txt +++ b/bindings/pyroot/pythonizations/CMakeLists.txt @@ -114,7 +114,6 @@ set(py_sources set(cpp_sources src/PyROOTModule.cxx - src/PyROOTWrapper.cxx src/RPyROOTApplication.cxx src/GenericPyz.cxx src/TClassPyz.cxx diff --git a/bindings/pyroot/pythonizations/src/PyROOTModule.cxx b/bindings/pyroot/pythonizations/src/PyROOTModule.cxx index 7ccd1d6d86d69..c8d04202af239 100644 --- a/bindings/pyroot/pythonizations/src/PyROOTModule.cxx +++ b/bindings/pyroot/pythonizations/src/PyROOTModule.cxx @@ -11,8 +11,8 @@ // Bindings #include "PyROOTPythonize.h" -#include "PyROOTWrapper.h" #include "RPyROOTApplication.h" +#include "TMemoryRegulator.h" // Cppyy #include "CPyCppyy/API.h" @@ -62,6 +62,20 @@ PyObject *RegisterExecutorAlias(PyObject * /*self*/, PyObject *args) } // namespace PyROOT +namespace { + +PyROOT::RegulatorCleanup &GetRegulatorCleanup() +{ + // The object is thread-local because it can happen that we call into + // C++ code (from the PyROOT CPython extension, from CPyCppyy or from cling) + // from different Python threads. A notable example is within a distributed + // RDataFrame application running on Dask. + thread_local PyROOT::RegulatorCleanup m; + return m; +} + +} // namespace + // Methods offered by the interface static PyMethodDef gPyROOTMethods[] = { {(char *)"AddCPPInstancePickling", (PyCFunction)PyROOT::AddCPPInstancePickling, METH_VARARGS, @@ -144,8 +158,13 @@ extern "C" PyObject *PyInit_libROOTPythonizations() // keep gRootModule, but do not increase its reference count even as it is borrowed, // or a self-referencing cycle would be created - // setup PyROOT - PyROOT::Init(); + // Initialize and acquire the GIL to allow for threading in ROOT +#if PY_VERSION_HEX < 0x03090000 + PyEval_InitThreads(); +#endif + + // Memory management + gROOT->GetListOfCleanups()->Add(&GetRegulatorCleanup()); // signal policy: don't abort interpreter in interactive mode CallContext::SetGlobalSignalPolicy(!gROOT->IsBatch()); diff --git a/bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx b/bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx deleted file mode 100644 index 26cd81d9d9ee3..0000000000000 --- a/bindings/pyroot/pythonizations/src/PyROOTWrapper.cxx +++ /dev/null @@ -1,42 +0,0 @@ -// Author: Enric Tejedor CERN 06/2018 -// Original PyROOT code by Wim Lavrijsen, LBL - -/************************************************************************* - * Copyright (C) 1995-2018, Rene Brun and Fons Rademakers. * - * All rights reserved. * - * * - * For the licensing terms see $ROOTSYS/LICENSE. * - * For the list of contributors see $ROOTSYS/README/CREDITS. * - *************************************************************************/ - -// Bindings -#include "PyROOTWrapper.h" -#include "TMemoryRegulator.h" - -// ROOT -#include "TROOT.h" - -namespace PyROOT { -R__EXTERN PyObject *gRootModule; -} - -PyROOT::RegulatorCleanup &GetRegulatorCleanup() -{ - // The object is thread-local because it can happen that we call into - // C++ code (from the PyROOT CPython extension, from CPyCppyy or from cling) - // from different Python threads. A notable example is within a distributed - // RDataFrame application running on Dask. - thread_local PyROOT::RegulatorCleanup m; - return m; -} - -void PyROOT::Init() -{ - // Initialize and acquire the GIL to allow for threading in ROOT -#if PY_VERSION_HEX < 0x03090000 - PyEval_InitThreads(); -#endif - - // Memory management - gROOT->GetListOfCleanups()->Add(&GetRegulatorCleanup()); -} diff --git a/bindings/pyroot/pythonizations/src/PyROOTWrapper.h b/bindings/pyroot/pythonizations/src/PyROOTWrapper.h deleted file mode 100644 index 2847bf6f404ad..0000000000000 --- a/bindings/pyroot/pythonizations/src/PyROOTWrapper.h +++ /dev/null @@ -1,24 +0,0 @@ -// Author: Enric Tejedor CERN 06/2018 -// Original PyROOT code by Wim Lavrijsen, LBL - -/************************************************************************* - * Copyright (C) 1995-2018, Rene Brun and Fons Rademakers. * - * All rights reserved. * - * * - * For the licensing terms see $ROOTSYS/LICENSE. * - * For the list of contributors see $ROOTSYS/README/CREDITS. * - *************************************************************************/ - -#ifndef PYROOT_ROOTWRAPPER_H -#define PYROOT_ROOTWRAPPER_H - -#include "Python.h" - -namespace PyROOT { - -// initialize ROOT -void Init(); - -} // namespace PyROOT - -#endif // !PYROOT_ROOTWRAPPER_H From 53a42450cf7242f437a2d04cb1f4f6336346f6a1 Mon Sep 17 00:00:00 2001 From: Vincenzo Eduardo Padulano Date: Tue, 12 Nov 2024 17:51:43 +0000 Subject: [PATCH 3/3] [PyROOT] Ensure gInterpreter is initialized once gROOT has been initialized --- bindings/pyroot/pythonizations/python/ROOT/_facade.py | 3 +++ bindings/pyroot/pythonizations/src/PyROOTModule.cxx | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/bindings/pyroot/pythonizations/python/ROOT/_facade.py b/bindings/pyroot/pythonizations/python/ROOT/_facade.py index 8cbe7007b5089..69a936e271808 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/_facade.py +++ b/bindings/pyroot/pythonizations/python/ROOT/_facade.py @@ -204,6 +204,9 @@ def _finalSetup(self): # Prevent this method from being re-entered through the gROOT wrapper self.__dict__["gROOT"] = cppyy.gbl.ROOT.GetROOT() + # Make sure the interpreter is initialized once gROOT has been initialized + cppyy.gbl.TInterpreter.Instance() + # Setup interactive usage from Python self.__dict__["app"] = PyROOTApplication(self.PyConfig, self._is_ipython) if not self.gROOT.IsBatch() and self.PyConfig.StartGUIThread: diff --git a/bindings/pyroot/pythonizations/src/PyROOTModule.cxx b/bindings/pyroot/pythonizations/src/PyROOTModule.cxx index c8d04202af239..664a95e5aa0c1 100644 --- a/bindings/pyroot/pythonizations/src/PyROOTModule.cxx +++ b/bindings/pyroot/pythonizations/src/PyROOTModule.cxx @@ -20,6 +20,7 @@ #include "../../cppyy/CPyCppyy/src/ProxyWrappers.h" // ROOT +#include "TInterpreter.h" #include "TROOT.h" #include "TSystem.h" #include "RConfigure.h" @@ -166,6 +167,9 @@ extern "C" PyObject *PyInit_libROOTPythonizations() // Memory management gROOT->GetListOfCleanups()->Add(&GetRegulatorCleanup()); + // Make sure the interpreter is initialized once gROOT has been initialized + TInterpreter::Instance(); + // signal policy: don't abort interpreter in interactive mode CallContext::SetGlobalSignalPolicy(!gROOT->IsBatch());