[PyROOT] Update cppyy to a recent version

[guitargeek]

Udate cppyy

Sister PR in roottest: root-project/roottest#1071

Summary

Synchronizes the CPyCppyy CPython extension and cppyy Python library with upstream to fix bugs, add features, and avoid duplicate maintenance efforts.

Behavior changes

No implicit conversion from fixed-sized char buffers to null-terminated string

If you have a char buffer with constant size, people might use it for different things. For example, to store null-terminated short strings in a TTree. Therefore, the current PyROOT converts such buffers to Python strings. However, that means it's impossible to get the full buffer if it contains zeros, which can be useful if the buffer doesn't contain a string but for example some status bytes.

Therefore, the used is not required to explicitly convert the buffer to a Python string with the as_string() method.

Demo:

import ROOT

ROOT.gInterpreter.Declare("""

struct Struct {
    char char_buffer[5] {};
};


void fill_char_buffer(Struct & st)
{
    std::string foo{"foo"};
    std::memcpy(st.char_buffer, foo.data(), foo.size());
}

""")
struct = ROOT.Struct()
ROOT.fill_char_buffer(struct)
char_buffer = struct.char_buffer

# With thew new cppyy, you get access to the lower level buffer instead:
print("struct.char_buffer            : ", char_buffer)

# However, you can turn the buffer into a string very easily with as_string():
print("struct.char_buffer.as_string(): ", char_buffer.as_string())

The output:

struct.char_buffer            :  <cppyy.LowLevelView object at 0x74c7a2682fb0>
struct.char_buffer.as_string():  foo

Associated GitHub issues

This will close the following GitHub issue:

• A crash when trying to initialise a vector from a >1D numpy array in PyROOT #12718
• [PyROOT] Segfault when creating proxy to derived class with multiple overloads #7686
• PyROOT: wrong overload resolution for C++ functions from python  #11924
• Wrong conversion from Numpy Array to std.vector when using the wrong type #12230
• [PyROOT] Python list of strings -> std::initializer_list< std::string > conversion is broken #11411
• cppyy does not retrieve correct size of type when pythonizing vector #11596

Upstream contributions to cppyy in the context of this synchronization

• Avoid warnings about unused variables wlav/CPyCppyy#11
• Improve integer type checking in Utility::GetBuffer() wlav/CPyCppyy#12
• Carry a copy of the string also in the std::string_view converter and consider statefulness in InitializerListConverter wlav/CPyCppyy#14
• Improve output of CPPMethod::GetPrototype wlav/CPyCppyy#16 (was in PyROOT before)
• Improve priority assignment of classes in same inheritance tree wlav/CPyCppyy#17 (was in PyROOT before)
• Improve priority assignment of classes in same inheritance tree wlav/cppyy-backend#20 (was in PyROOT before)
• Make CppyyLegacy namespace in CPyCppyy optional wlav/CPyCppyy#18
• Fix incomplete initializer lists, unused vars, and some type warnings wlav/CPyCppyy#19
• Support conversion from str to char[] wlav/CPyCppyy#21
• Correcly forward control flag to InstancePtrConverter wlav/CPyCppyy#22
• Don't attempt to print detailed exception info if filename is empty wlav/CPyCppyy#23

Performance validation

To validate the performance, I ran the Python tests in roottest and the PyROOT pythonization tests and compared runtimes with and without the cppyy upgrade. The total runtime of these tests reduced by about 4 % from 208 s to 287 s, so the performance impact of this PR is marginal. The runtime comparison for each test can be found in cppyy_upgrade_test_runtimes.txt.
The worst observed performance penalty is 24 %. However, significant speedups are observed in some of the longer tests. For convenience, the 20 tests with the longest runtime are listed here:

                               title  no_cppyy_upgrade  cppyy_upgrade     ratio
        python-regression-regression             32.61          34.30  1.051825
       pyroot-pyz-rdataframe-asnumpy             14.30          14.61  1.021678
     pyroot-pyz-rdataframe-makenumpy             13.66           9.65  0.706442
            python-function-function             12.27          11.08  0.903015
                      python-cpp-cpp             12.03          11.26  0.935993
                 python-cpp-advanced             11.55          10.83  0.937662
 pyroot-pyz-rdataframe-histo-profile             11.24           7.37  0.655694
                  python-basic-basic             10.11           9.95  0.984174
                      python-stl-stl              8.35           7.87  0.942515
                  pyroot-pyz-rtensor              6.94           2.32  0.334294
                  python-ttree-ttree              6.15           5.70  0.926829
               python-basic-datatype              5.87           5.86  0.998296
                 python-pickle-write              5.56           5.56  1.000000
               python-basic-overload              5.29           5.30  1.001890
python-pythonizations-pythonizations              5.27           5.20  0.986717
      python-pythonizations-smartptr              5.04           4.89  0.970238
               python-basic-operator              4.79           4.74  0.989562
                    python-cpp-cpp11              4.72           4.69  0.993644
                python-memory-memory              4.62           4.48  0.969697
         python-regression-root_6023              4.61           4.44  0.963124

[github-actions]

Test Results

    11 files      11 suites   2d 8h 31m 25s ⏱️
 2 602 tests  2 601 ✅ 0 💤 1 ❌
26 692 runs  26 691 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 9c4418a.

♻️ This comment has been updated with latest results.

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

[guitargeek]

Hi @smuzaffar! This is a big upgrade, and testing it with CMSSW would give us more confidence, the ROOT tests are all green already. Can you give us a hand? Thanks!

[smuzaffar]

@guitargeek , cmssw tests via cms-sw#205

[guitargeek]

Thank you!

[dpiparo]

@smuzaffar , thanks also from me - appreciated!

[smuzaffar]

FYI , cmssw tests passed cms-sw#205 (comment)

[guitargeek]

This is great news! I'll stop making code changes to the PR now, so we keep the validated state. I'll just need to update a few more times for the commit history and bookkeeping of patches.

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

[phsft-bot]

Build failed on windows10/default.
See console output.

[phsft-bot]

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

[vepadulano]

Thank you for this incredible piece of work! It's such a huge step towards the right direction! I believe this first PR is ready to be merged to move forward, but note the following comment.

I strongly believe we should have a similar approach as with our LLVM fork. We need to have some "source of truth" that is a certain cppyy/CPyCPPyy tag from upstream and then a clear way to reach the status of our fork from there, i.e. a series of patches that can be applied without conflicts. This PR goes in that direction but doesn't implement it fully as the sync script refers to a repository outside of our organisation. Ideally we would have separate repositories (one for cppyy and one for CPyCppyy) that we can refer to

[vepadulano]

Why are we even testing this? I believe it's not necessary and as your commit message rightly says, fragile. Let's just remove it

[guitargeek]

I'm not sure, I'm not familiar with the tests.

Right now things are quite stable with only this little change of the reference value, but if you agree that it can be removed I'm happy to do this as soon as the test fails somewhere again!

[guitargeek]

I strongly believe we should have a similar approach as with our LLVM fork. We need to have some "source of truth" that is a certain cppyy/CPyCPPyy tag from upstream and then a clear way to reach the status of our fork from there, i.e. a series of patches that can be applied without conflicts. This PR goes in that direction but doesn't implement it fully as the sync script refers to a repository outside of our organisation. Ideally we would have separate repositories (one for cppyy and one for CPyCppyy) that we can refer to

Thanks for raising this point. The situation will be improved in the next weeks, I'll try to get as many patches merged to upstream as possible. Then, based on how many differences are left, we can decide if we want to go with one (or multiple) separate repositories, or we stay with the patch files.