From 29533660c29858ac635f8a06cbc15c3c4c4cfa7c Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 12:06:40 +0200 Subject: [PATCH 1/9] Improve import structure of podio to feel more pythonic --- .gitignore | 4 ++-- CMakeLists.txt | 4 ++-- python/{__init__.py.in => __version__.py.in} | 0 python/podio/__init__.py | 23 ++++++++++++++++++++ python/podio/test_EventStoreRoot.py | 2 +- python/podio/test_EventStoreSio.py | 2 +- python/podio/test_ReaderRoot.py | 2 +- python/podio/test_ReaderSio.py | 2 +- python/podio/test_utils.py | 2 +- 9 files changed, 32 insertions(+), 9 deletions(-) rename python/{__init__.py.in => __version__.py.in} (100%) create mode 100644 python/podio/__init__.py diff --git a/.gitignore b/.gitignore index 22f536c88..3315983f2 100644 --- a/.gitignore +++ b/.gitignore @@ -64,8 +64,8 @@ spack* # Populated by cmake before build /include/podio/podioVersion.h -/python/podio/__init__.py +/python/podio/__version__.py # CLion artifacts .idea/ -cmake-build*/ \ No newline at end of file +cmake-build*/ diff --git a/CMakeLists.txt b/CMakeLists.txt index 4f4ad10ae..9dcbe88ae 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -169,8 +169,8 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/podioVersion.in.h ${CMAKE_CURRENT_SOURCE_DIR}/include/podio/podioVersion.h ) install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/include/podio/podioVersion.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/podio ) -configure_file(${CMAKE_CURRENT_SOURCE_DIR}/python/__init__.py.in - ${CMAKE_CURRENT_SOURCE_DIR}/python/podio/__init__.py) +configure_file(${CMAKE_CURRENT_SOURCE_DIR}/python/__version__.py.in + ${CMAKE_CURRENT_SOURCE_DIR}/python/podio/__version__.py) #--- add license files --------------------------------------------------------- install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/NOTICE diff --git a/python/__init__.py.in b/python/__version__.py.in similarity index 100% rename from python/__init__.py.in rename to python/__version__.py.in diff --git a/python/podio/__init__.py b/python/podio/__init__.py new file mode 100644 index 000000000..8d22cbb5c --- /dev/null +++ b/python/podio/__init__.py @@ -0,0 +1,23 @@ +"""Python module for the podio EDM toolkit and its utilities""" +import sys + +from .__version__ import __version__ + +import ROOT + +if ROOT.gSystem.Load("libpodioDict.so") < 0: + raise RuntimeError("Failed to load libpodioDict.so") + +from ROOT import podio + +from .podio_config_reader import * + +from .frame import Frame + +from . import root_io, sio_io, reading + +from .test_utils import * + +from . import EventStore + +sys.modules["podio"] = podio diff --git a/python/podio/test_EventStoreRoot.py b/python/podio/test_EventStoreRoot.py index b9f3830dd..6016301e0 100644 --- a/python/podio/test_EventStoreRoot.py +++ b/python/podio/test_EventStoreRoot.py @@ -7,7 +7,7 @@ from ROOT import TFile from podio.EventStore import EventStore -from podio.test_EventStore import EventStoreBaseTestCaseMixin +from test_EventStore import EventStoreBaseTestCaseMixin class EventStoreRootTestCase(EventStoreBaseTestCaseMixin, unittest.TestCase): diff --git a/python/podio/test_EventStoreSio.py b/python/podio/test_EventStoreSio.py index 511da07d8..eaa330bd9 100644 --- a/python/podio/test_EventStoreSio.py +++ b/python/podio/test_EventStoreSio.py @@ -5,7 +5,7 @@ import os from podio.EventStore import EventStore -from podio.test_EventStore import EventStoreBaseTestCaseMixin +from test_EventStore import EventStoreBaseTestCaseMixin from podio.test_utils import SKIP_SIO_TESTS diff --git a/python/podio/test_ReaderRoot.py b/python/podio/test_ReaderRoot.py index 44ee32157..2d14a67a4 100644 --- a/python/podio/test_ReaderRoot.py +++ b/python/podio/test_ReaderRoot.py @@ -4,7 +4,7 @@ import unittest from podio.root_io import Reader, LegacyReader -from podio.test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin +from test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin class RootReaderTestCase(ReaderTestCaseMixin, unittest.TestCase): diff --git a/python/podio/test_ReaderSio.py b/python/podio/test_ReaderSio.py index 83489919d..d9877e4da 100644 --- a/python/podio/test_ReaderSio.py +++ b/python/podio/test_ReaderSio.py @@ -3,7 +3,7 @@ import unittest -from podio.test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin +from test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin from podio.test_utils import SKIP_SIO_TESTS diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 44efc9cce..8a2679c83 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -6,7 +6,7 @@ ROOT.gSystem.Load("libTestDataModelDict.so") # noqa: E402 from ROOT import ExampleHitCollection, ExampleClusterCollection # noqa: E402 # pylint: disable=wrong-import-position -from podio.frame import Frame # pylint: disable=wrong-import-position +from podio import Frame # pylint: disable=wrong-import-position SKIP_SIO_TESTS = os.environ.get("SKIP_SIO_TESTS", "1") == "1" From 6a0b5f2d5dac0e26c5ec6f8334e76d02b7140e14 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 14:08:17 +0200 Subject: [PATCH 2/9] Make sure python bindings are usable without libs --- python/podio/__init__.py | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/python/podio/__init__.py b/python/podio/__init__.py index 8d22cbb5c..db02e7cee 100644 --- a/python/podio/__init__.py +++ b/python/podio/__init__.py @@ -5,19 +5,28 @@ import ROOT -if ROOT.gSystem.Load("libpodioDict.so") < 0: - raise RuntimeError("Failed to load libpodioDict.so") - -from ROOT import podio - from .podio_config_reader import * -from .frame import Frame +# Track whether we were able to dynamially load the library that is built by +# podio and enable certain features of the bindings only if they are actually +# available. +_DYNAMIC_LIBS_LOADED = False + +# Check if we can locate the dictionary wthout loading it as this allows us to +# silence any ouptput. If we can find it, we can also safely load it +if ROOT.gSystem.DynamicPathName("libpodioDict.so", True): + ROOT.gSystem.Load("libpodioDict.so") + from ROOT import podio -from . import root_io, sio_io, reading + _DYNAMIC_LIBS_LOADED = True -from .test_utils import * +if _DYNAMIC_LIBS_LOADED: + from .frame import Frame + from . import root_io, sio_io, reading + from .test_utils import * -from . import EventStore + from . import EventStore -sys.modules["podio"] = podio + # Make sure that this module is actually usable as podio even though most of + # it is dynamically populated by cppyy + sys.modules["podio"] = podio From 380c190312b34112c38e696e5672e7fdfbfe1daf Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 14:15:34 +0200 Subject: [PATCH 3/9] Make import of test_utils more robust --- python/podio/__init__.py | 20 ++++++++++---------- python/podio/test_utils.py | 5 +++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/python/podio/__init__.py b/python/podio/__init__.py index db02e7cee..4b6e3df0f 100644 --- a/python/podio/__init__.py +++ b/python/podio/__init__.py @@ -15,18 +15,18 @@ # Check if we can locate the dictionary wthout loading it as this allows us to # silence any ouptput. If we can find it, we can also safely load it if ROOT.gSystem.DynamicPathName("libpodioDict.so", True): - ROOT.gSystem.Load("libpodioDict.so") - from ROOT import podio + ROOT.gSystem.Load("libpodioDict.so") + from ROOT import podio - _DYNAMIC_LIBS_LOADED = True + _DYNAMIC_LIBS_LOADED = True if _DYNAMIC_LIBS_LOADED: - from .frame import Frame - from . import root_io, sio_io, reading - from .test_utils import * + from .frame import Frame + from . import root_io, sio_io, reading + from .test_utils import * - from . import EventStore + from . import EventStore - # Make sure that this module is actually usable as podio even though most of - # it is dynamically populated by cppyy - sys.modules["podio"] = podio + # Make sure that this module is actually usable as podio even though most of + # it is dynamically populated by cppyy + sys.modules["podio"] = podio diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index 8a2679c83..efd2d53f4 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -3,8 +3,9 @@ import os import ROOT -ROOT.gSystem.Load("libTestDataModelDict.so") # noqa: E402 -from ROOT import ExampleHitCollection, ExampleClusterCollection # noqa: E402 # pylint: disable=wrong-import-position + +if ROOT.gSystem.Load("libTestDataModelDict.so") >= 0: # noqa: E402 + from ROOT import ExampleHitCollection, ExampleClusterCollection # noqa: E402 # pylint: disable=wrong-import-position from podio import Frame # pylint: disable=wrong-import-position From e94cb78c88fad182e809f10e1a5c772937705fe1 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 14:54:33 +0200 Subject: [PATCH 4/9] Remove test_utils from imported modules --- python/podio/__init__.py | 13 ++++++++++--- python/podio/test_EventStoreSio.py | 2 +- python/podio/test_Frame.py | 3 ++- python/podio/test_ReaderSio.py | 2 +- python/podio/test_utils.py | 4 ++-- tests/root_io/write_frame_root.py | 7 ++++++- 6 files changed, 22 insertions(+), 9 deletions(-) diff --git a/python/podio/__init__.py b/python/podio/__init__.py index 4b6e3df0f..1cd0b0212 100644 --- a/python/podio/__init__.py +++ b/python/podio/__init__.py @@ -3,10 +3,10 @@ from .__version__ import __version__ -import ROOT - from .podio_config_reader import * +import ROOT + # Track whether we were able to dynamially load the library that is built by # podio and enable certain features of the bindings only if they are actually # available. @@ -23,10 +23,17 @@ if _DYNAMIC_LIBS_LOADED: from .frame import Frame from . import root_io, sio_io, reading - from .test_utils import * from . import EventStore # Make sure that this module is actually usable as podio even though most of # it is dynamically populated by cppyy sys.modules["podio"] = podio + + __all__ = [ + "__version__", + "Frame", + "root_io", + "sio_io", + "reading", + ] diff --git a/python/podio/test_EventStoreSio.py b/python/podio/test_EventStoreSio.py index eaa330bd9..e86d25147 100644 --- a/python/podio/test_EventStoreSio.py +++ b/python/podio/test_EventStoreSio.py @@ -6,7 +6,7 @@ from podio.EventStore import EventStore from test_EventStore import EventStoreBaseTestCaseMixin -from podio.test_utils import SKIP_SIO_TESTS +from test_utils import SKIP_SIO_TESTS @unittest.skipIf(SKIP_SIO_TESTS, "no SIO support") diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index 7761deaf0..1255e3a8d 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -7,7 +7,8 @@ # using root_io as that should always be present regardless of which backends are built from podio.root_io import Reader -from podio.test_utils import ExampleHitCollection +from test_utils import ExampleHitCollection # noqa: E402 # pylint: disable=wrong-import-position + # The expected collections in each frame EXPECTED_COLL_NAMES = { diff --git a/python/podio/test_ReaderSio.py b/python/podio/test_ReaderSio.py index d9877e4da..e0d95da69 100644 --- a/python/podio/test_ReaderSio.py +++ b/python/podio/test_ReaderSio.py @@ -4,7 +4,7 @@ import unittest from test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin -from podio.test_utils import SKIP_SIO_TESTS +from test_utils import SKIP_SIO_TESTS @unittest.skipIf(SKIP_SIO_TESTS, "no SIO support") diff --git a/python/podio/test_utils.py b/python/podio/test_utils.py index efd2d53f4..3fed5959a 100644 --- a/python/podio/test_utils.py +++ b/python/podio/test_utils.py @@ -4,8 +4,8 @@ import os import ROOT -if ROOT.gSystem.Load("libTestDataModelDict.so") >= 0: # noqa: E402 - from ROOT import ExampleHitCollection, ExampleClusterCollection # noqa: E402 # pylint: disable=wrong-import-position +ROOT.gSystem.Load("libTestDataModelDict.so") # noqa: E402 +from ROOT import ExampleHitCollection, ExampleClusterCollection # noqa: E402 # pylint: disable=wrong-import-position from podio import Frame # pylint: disable=wrong-import-position diff --git a/tests/root_io/write_frame_root.py b/tests/root_io/write_frame_root.py index 38bece171..e67a0ffea 100644 --- a/tests/root_io/write_frame_root.py +++ b/tests/root_io/write_frame_root.py @@ -1,7 +1,12 @@ #!/usr/bin/env python3 """Script to write a Frame in ROOT format""" -from podio import test_utils +import os +import sys + +sys.path.append(os.path.join(os.environ.get("PODIO_BASE"), "python", "podio")) + +import test_utils from podio.root_io import Writer test_utils.write_file(Writer, "example_frame_with_py.root") From 096d49473d3997dfdd6cc0a2a9e4cbc9c47961ce Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 15:21:38 +0200 Subject: [PATCH 5/9] flake8: Fix __init__.py --- python/podio/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/podio/__init__.py b/python/podio/__init__.py index 1cd0b0212..80ccce5f0 100644 --- a/python/podio/__init__.py +++ b/python/podio/__init__.py @@ -3,7 +3,7 @@ from .__version__ import __version__ -from .podio_config_reader import * +from .podio_config_reader import * # noqa: F403, F401 import ROOT @@ -36,4 +36,5 @@ "root_io", "sio_io", "reading", + "EventStore" ] From 7c7f0d8b935a385a0d44af316a9d73b9b9de4239 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 15:37:45 +0200 Subject: [PATCH 6/9] Make Frame python bindings more robust --- python/podio/frame.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/python/podio/frame.py b/python/podio/frame.py index 1a69d7a46..f3185512d 100644 --- a/python/podio/frame.py +++ b/python/podio/frame.py @@ -5,9 +5,16 @@ import cppyy import ROOT + # NOTE: It is necessary that this can be found on the ROOT_INCLUDE_PATH -ROOT.gInterpreter.LoadFile('podio/Frame.h') # noqa: E402 -from ROOT import podio # noqa: E402 # pylint: disable=wrong-import-position +# +# We check whether we can actually load the header to not break python bindings +# in environments with *ancient* podio versions +if ROOT.gInterpreter.LoadFile('podio/Frame.h') == 0: # noqa: E402 + from ROOT import podio # noqa: E402 # pylint: disable=wrong-import-position + _FRAME_HEADER_AVAILABLE = True +else: + _FRAME_HEADER_AVAILABLE = False def _determine_supported_parameter_types(): @@ -45,7 +52,8 @@ def _determine_cpp_type(idx_and_type): return tuple(zip(cpp_types, py_types)) -SUPPORTED_PARAMETER_TYPES = _determine_supported_parameter_types() +if _FRAME_HEADER_AVAILABLE: + SUPPORTED_PARAMETER_TYPES = _determine_supported_parameter_types() def _get_cpp_types(type_str): From 81ac98da5f679751fb58943109e63947b110d75b Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 15:46:23 +0200 Subject: [PATCH 7/9] pylint: Fix pylint warnings --- python/podio/__init__.py | 2 +- python/podio/test_EventStoreRoot.py | 3 ++- python/podio/test_EventStoreSio.py | 5 +++-- python/podio/test_Frame.py | 5 +++-- python/podio/test_ReaderRoot.py | 3 ++- python/podio/test_ReaderSio.py | 4 ++-- tests/root_io/write_frame_root.py | 6 ++++-- 7 files changed, 17 insertions(+), 11 deletions(-) diff --git a/python/podio/__init__.py b/python/podio/__init__.py index 80ccce5f0..477247fde 100644 --- a/python/podio/__init__.py +++ b/python/podio/__init__.py @@ -5,7 +5,7 @@ from .podio_config_reader import * # noqa: F403, F401 -import ROOT +import ROOT # pylint: disable=wrong-import-order # Track whether we were able to dynamially load the library that is built by # podio and enable certain features of the bindings only if they are actually diff --git a/python/podio/test_EventStoreRoot.py b/python/podio/test_EventStoreRoot.py index 6016301e0..522e389d7 100644 --- a/python/podio/test_EventStoreRoot.py +++ b/python/podio/test_EventStoreRoot.py @@ -6,8 +6,9 @@ from ROOT import TFile +from test_EventStore import EventStoreBaseTestCaseMixin # pylint: disable=import-error + from podio.EventStore import EventStore -from test_EventStore import EventStoreBaseTestCaseMixin class EventStoreRootTestCase(EventStoreBaseTestCaseMixin, unittest.TestCase): diff --git a/python/podio/test_EventStoreSio.py b/python/podio/test_EventStoreSio.py index e86d25147..c8d19b852 100644 --- a/python/podio/test_EventStoreSio.py +++ b/python/podio/test_EventStoreSio.py @@ -4,9 +4,10 @@ import unittest import os +from test_utils import SKIP_SIO_TESTS # pylint: disable=import-error +from test_EventStore import EventStoreBaseTestCaseMixin # pylint: disable=import-error + from podio.EventStore import EventStore -from test_EventStore import EventStoreBaseTestCaseMixin -from test_utils import SKIP_SIO_TESTS @unittest.skipIf(SKIP_SIO_TESTS, "no SIO support") diff --git a/python/podio/test_Frame.py b/python/podio/test_Frame.py index 1255e3a8d..0dc823d76 100644 --- a/python/podio/test_Frame.py +++ b/python/podio/test_Frame.py @@ -3,12 +3,13 @@ import unittest +# pylint: disable=import-error +from test_utils import ExampleHitCollection # noqa: E402 + from podio.frame import Frame # using root_io as that should always be present regardless of which backends are built from podio.root_io import Reader -from test_utils import ExampleHitCollection # noqa: E402 # pylint: disable=wrong-import-position - # The expected collections in each frame EXPECTED_COLL_NAMES = { diff --git a/python/podio/test_ReaderRoot.py b/python/podio/test_ReaderRoot.py index 2d14a67a4..a7bdf98f8 100644 --- a/python/podio/test_ReaderRoot.py +++ b/python/podio/test_ReaderRoot.py @@ -3,8 +3,9 @@ import unittest +from test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin # pylint: disable=import-error + from podio.root_io import Reader, LegacyReader -from test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin class RootReaderTestCase(ReaderTestCaseMixin, unittest.TestCase): diff --git a/python/podio/test_ReaderSio.py b/python/podio/test_ReaderSio.py index e0d95da69..ef7b86b6b 100644 --- a/python/podio/test_ReaderSio.py +++ b/python/podio/test_ReaderSio.py @@ -3,8 +3,8 @@ import unittest -from test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin -from test_utils import SKIP_SIO_TESTS +from test_Reader import ReaderTestCaseMixin, LegacyReaderTestCaseMixin # pylint: disable=import-error +from test_utils import SKIP_SIO_TESTS # pylint: disable=import-error @unittest.skipIf(SKIP_SIO_TESTS, "no SIO support") diff --git a/tests/root_io/write_frame_root.py b/tests/root_io/write_frame_root.py index e67a0ffea..2609c602a 100644 --- a/tests/root_io/write_frame_root.py +++ b/tests/root_io/write_frame_root.py @@ -6,7 +6,9 @@ sys.path.append(os.path.join(os.environ.get("PODIO_BASE"), "python", "podio")) -import test_utils -from podio.root_io import Writer +import test_utils # pylint: disable=import-error, disable=wrong-import-position + +from podio.root_io import Writer # pylint: disable=wrong-import-position + test_utils.write_file(Writer, "example_frame_with_py.root") From 3632f0a5cadc346c55d8ba63072f9efc25908119 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 15:46:37 +0200 Subject: [PATCH 8/9] pylint: Fix deprecation warning for newer pylint versions --- .github/scripts/pylint.rc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/pylint.rc b/.github/scripts/pylint.rc index 4d1492c68..11841f3a5 100644 --- a/.github/scripts/pylint.rc +++ b/.github/scripts/pylint.rc @@ -349,4 +349,4 @@ analyse-fallback-blocks=no # Exceptions that will emit a warning when being caught. Defaults to # "Exception" -overgeneral-exceptions=Exception +overgeneral-exceptions=builtins.Exception From f1b6bee7f5ba0f2a52140b1be26dc53ef890bfc9 Mon Sep 17 00:00:00 2001 From: tmadlener Date: Fri, 15 Sep 2023 20:18:54 +0200 Subject: [PATCH 9/9] Keep test_utils in podio module to make tests work --- python/podio/__init__.py | 8 ++++++++ tests/root_io/write_frame_root.py | 7 +------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/python/podio/__init__.py b/python/podio/__init__.py index 477247fde..483885729 100644 --- a/python/podio/__init__.py +++ b/python/podio/__init__.py @@ -26,6 +26,14 @@ from . import EventStore + try: + # For some reason the test_utils only work at (test) runtime if they are + # imported with the rest of podio. Otherwise they produce a weird c++ error. + # This happens even if we import the *exact* same file. + from . import test_utils # noqa: F401 + except ImportError: + pass + # Make sure that this module is actually usable as podio even though most of # it is dynamically populated by cppyy sys.modules["podio"] = podio diff --git a/tests/root_io/write_frame_root.py b/tests/root_io/write_frame_root.py index 2609c602a..e0525a268 100644 --- a/tests/root_io/write_frame_root.py +++ b/tests/root_io/write_frame_root.py @@ -1,12 +1,7 @@ #!/usr/bin/env python3 """Script to write a Frame in ROOT format""" -import os -import sys - -sys.path.append(os.path.join(os.environ.get("PODIO_BASE"), "python", "podio")) - -import test_utils # pylint: disable=import-error, disable=wrong-import-position +from podio import test_utils from podio.root_io import Writer # pylint: disable=wrong-import-position