Skip to content

Conversation

@wdconinc
Copy link
Contributor

This PR moves the implementation of Pythonizations detail functions into a source file. This allows the headers to be included in C++20 modules (#907).

BEGINRELEASENOTES

  • Move Pythonizations detail implementations to separate source file

ENDRELEASENOTES

Copilot AI review requested due to automatic review settings December 22, 2025 17:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the Pythonizations implementation by moving function definitions from inline functions in the header file to a separate source file. This change enables the headers to be included in C++20 modules while maintaining the same functionality.

  • Function implementations moved from static inline in header to regular definitions in source file
  • Added target_link_libraries(podio PRIVATE Python3::Python) to CMakeLists.txt
  • Header file now contains only forward declarations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/Pythonizations.cc New source file containing implementations of subscript and pythonize_subscript functions
include/podio/detail/Pythonizations.h Refactored to contain only function declarations, removed inline implementations and <stdexcept> include
src/CMakeLists.txt Added Pythonizations.cc to core_sources and linked Python3 library

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tmadlener tmadlener enabled auto-merge (squash) December 22, 2025 19:19
@tmadlener tmadlener merged commit 7187c36 into AIDASoft:master Dec 22, 2025
26 checks passed
@wdconinc wdconinc deleted the pythonizations-impl-in-lib branch December 22, 2025 19:40

PODIO_ADD_LIB_AND_DICT(podio "${core_headers}" "${core_sources}" selection.xml)
target_compile_options(podio PRIVATE -pthread)
target_link_libraries(podio PRIVATE Python3::Python)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this probably should have been in the Python:: namespace since that's what is added as a dependency in the config

if(NOT "@REQUIRE_PYTHON_VERSION@" STREQUAL "")
find_dependency(Python @REQUIRE_PYTHON_VERSION@ COMPONENTS Interpreter Development)
else()
find_dependency(Python COMPONENTS Interpreter Development)
endif()

and in the PODIO_GENERATE_DATAMODEL module:
COMMAND ${Python_EXECUTABLE} ${podio_PYTHON_DIR}/podio_class_generator.py ${CLANG_FORMAT_ARG} ${SCHEMA_EVOLUTION_ARG} ${UPSTREAM_EDM_ARG} ${YAML_FILE} ${ARG_OUTPUT_FOLDER} ${datamodel} ${ARG_IO_BACKEND_HANDLERS} ${LANGUAGE_ARG} ${VERSION_ARG} ${OLD_DESCRIPTION_ARG}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then again, the package that is found is Python3...

if(NOT PODIO_RELAX_PYVER)
find_package(Python3 ${REQUIRE_PYTHON_VERSION} EXACT REQUIRED COMPONENTS Development Interpreter)
else()
find_package(Python3 ${REQUIRE_PYTHON_VERSION} REQUIRED COMPONENTS Development Interpreter)

@paulgessinger
Copy link
Contributor

paulgessinger commented Jan 19, 2026

I've investigated this a bit after the Mac build started failing in spack for me. The way I see it, this now compiles in a strict dependency on the exact Python runtime lib into the podio library, which means that you need to actually link against python in a way you usually do for embedding the interpreter.

This then causes issues in Spack, I believe because the python build from the package there does not correctly propagate it's own build configuration on macOS, and relies on intl being installed in linux "by accident".

@tmadlener
Copy link
Collaborator

tmadlener commented Jan 19, 2026

Looks like moving these into a dedicated TU and building them into libpodio causes quite a few issues. I am wondering whether we should move the implementation back into Pythonizations.h but marking the functions inline only. That should work for modules, IIUC.

edit: Just read in #907 that Python.h is the issue not the static inline.


Some additional information of what we have learned in the meantime about the behavior with and without these changes.

  • These changes make builds on macOS fail: see Link against Python3::Module #925
  • Prior to these changes it looks like any and all references to these functions were inlined and subsequently removed by the linker as there is no trace of them in the compiled binaries (e.g. readelf -sWC libedm4hep.so | grep cppyy doesn't give any hits). cppyy is still able to find them (potentially because it JIT compiles the collection headers)
    • We were also missing a target_link_libraries(podio PUBLIC Python3::Python). However, given the above that didn't raise an issue.
  • Moving the inline definition of __cppyy_pythonize_ from the collection headers to the collection implementation files in code generations makes all symbols appear again.
    • These changes would require to at least have a target_link_libraries(<edmlib> PRIVATE Python3::Python) to be present in the cmake macro that builds the core datamodel lib.

@wdconinc
Copy link
Contributor Author

Looks like moving these into a dedicated TU and building them into libpodio causes quite a few issues. I am wondering whether we should move the implementation back into Pythonizations.h but marking the functions inline only. That should work for modules, IIUC.

Certainly the use case of modules is tangential here. If this needs to be reverted, we can figure out how to make it work with modules given the additional constraint of header-only Pythonizations.h. That said, I think just inline (not static inline) should be sufficient. The issue was not in the Python.h header in this case (at least not for the python I was testing with).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants