root icon indicating copy to clipboard operation
root copied to clipboard

[PyROOT] Avoid linking TPython against libPython

Open vepadulano opened this issue 1 year ago • 4 comments

I'm opening this PR to give it a go with our CI. This change is somewhat necessary on many levels (conda benefits from it, pip requires it). And it is also a good habit in general not to link against libPython.

vepadulano avatar Jun 20 '24 12:06 vepadulano

Good move. The CI is quite demanding these days. If the change works there AND in the Conda CI, the coverage should be rather good. Clearly, we should add a note in the RN to inform owners of large stacks, e.g. LHC experiments, of this change in case something behaves differently in that context.

dpiparo avatar Jun 20 '24 12:06 dpiparo

e.g. LHC experiments, of this change in case something behaves differently in that context.

This move is necessary for the Python environments but there is a clear limitation in its current form, that is it breaks the usage of TPython from C++:

root.exe -l -b -q -x -e 'TPython::Exec("print(\"1 + 1 =\", 1+1)")'

cling::DynamicLibraryManager::loadLibrary(): dlopen(/Users/vpadulan/Programs/rootproject/rootbuild/master-4e3ca10195-pyroot-debug/lib/libROOTTPython.so, 0x0009): symbol not found in flat namespace '_PyBool_Type'
Error in <AutoloadLibraryMU>: Failed to load library /Users/vpadulan/Programs/rootproject/rootbuild/master-4e3ca10195-pyroot-debug/lib/libROOTTPython.socling JIT session error: Failed to materialize symbols: { (main, { __ZN7TPython4ExecEPKc }) }

This is not surprising, we are purposely removing the linking against libPython so TPython cannot find the symbols. From within a Python interpreter, libpython is automatically injected and linked at the global scope, but that doesn't happen when a symbol from libpython is needed from an executable outside of Python itself.

Bottom line, we will need to investigate how to properly manage both the requirements of Python packaging systems and embedding libpython in other cases.

vepadulano avatar Jun 20 '24 13:06 vepadulano

Test Results

    13 files      13 suites   2d 17h 7m 3s :stopwatch:  2 648 tests  2 643 :white_check_mark: 0 :zzz:  5 :x: 32 606 runs  32 591 :white_check_mark: 0 :zzz: 15 :x:

For more details on these failures, see this check.

Results for commit ebd77382.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jun 20 '24 14:06 github-actions[bot]

I think the sustainable fix is to get rid of TPython all together, since the provided functionality of the CPyCppyy API should be the same.

In fact, there was a time where TPython was completely gone from the codebase until Enric brought it back with 2934be5fb3007, because apparently there are still come problems with the CPyCppyy API.

Maybe it is now a good time to complete the migration to the CPyCppyy API?

guitargeek avatar Jun 24 '24 23:06 guitargeek

FYI, I have opened some PRs for the CPyCppyy API to make it fit for replacing TPython:

  • https://github.com/wlav/CPyCppyy/pull/30
  • https://github.com/wlav/CPyCppyy/pull/31

guitargeek avatar Jul 12 '24 11:07 guitargeek

Thank you @guitargeek I believe this PR can be closed now!

vepadulano avatar Sep 19 '24 07:09 vepadulano