Skip to content

Conversation

@guitargeek
Copy link
Contributor

Upstream version of root-project/root#19222

The CPPIntance type is immutable, and new attributes can't be set:

import cppyy
>>> setattr(cppyy._backend.CPPInstance, "test", 10)
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    setattr(cppyy._backend.CPPInstance, "test", 10)
    ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: cannot set 'test' attribute of immutable type 'cppyy.CPPInstance'
>>>

In our pythonizations, we still want to monkey-patch a __reduce__ method anyway, and to make Python not complain we to a hack by setting the attribute with the C Python API using PyObject_GenericSetAttr. This function doesn't normally do any safety checks, and our monkey patching works.

However, the Python debug build is not having any of that, thanks to a new assert that was added a year ago:

https://github.com/python/cpython/blame/c419af9e277bea7dd78f4defefc752fe93b0b8ec/Objects/object.c#L1921

To make the ROOT Python interface work with debug builds of the Python interpreter, we therefore have to implement adding the reduce method properly.

This commit suggests to achieve this by defining the __reduce__ method in the immutable CPPInstance type within CPyCppyy, but in such a way that its implementation can be routed to ROOT via a private API.

@guitargeek guitargeek changed the title Avoid hack in setting __reduce__ attribute of CPPInstance Provide private API to install callback in __reduce__ attribute of CPPInstance Jul 17, 2025
This API addressed the following problem im ROOT:

The CPPIntance type is immutable, and new attributes can't be set:
```txt
import cppyy
>>> setattr(cppyy._backend.CPPInstance, "test", 10)
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    setattr(cppyy._backend.CPPInstance, "test", 10)
    ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: cannot set 'test' attribute of immutable type 'cppyy.CPPInstance'
>>>
```

In our pythonizations, we still want to monkey-patch a `__reduce__`
method anyway, and to make Python not complain we to a hack by setting
the attribute with the C Python API using `PyObject_GenericSetAttr`.
This function doesn't normally do any safety checks, and our monkey
patching works.

However, the Python debug build is not having any of that, thanks to a
new assert that was added a year ago:

https://github.com/python/cpython/blame/c419af9e277bea7dd78f4defefc752fe93b0b8ec/Objects/object.c#L1921

To make the ROOT Python interface work with debug builds of the Python
interpreter, we therefore have to implement adding the reduce method
properly.

This commit suggests to achieve this by defining the `__reduce__` method
in the immutable CPPInstance type within CPyCppyy, but in such a way
that its implementation can be routed to ROOT via a private API.
@guitargeek guitargeek changed the title Provide private API to install callback in __reduce__ attribute of CPPInstance Provide private API to set CPPInstance __reduce__ callback Jul 17, 2025
@wlav
Copy link
Owner

wlav commented Jul 17, 2025

This implementation means there can only ever be 1 reduce method, or can it now be overridden in a derived class? If the latter, isn't it sufficient to make this a no-op?

@guitargeek
Copy link
Contributor Author

Sure, this can be overridden in derived classes, but if I understand the intent of the ROOT pythonization correctly, we need to set a default base implementation for any kind of CPPInstance. I mean if the intent was to serialize only TObject-derived classes, then this pythonization would have been done for TObject and all is good. But what if you want to __reduce__ a std::vector? I'll have to think more about that and try what actually happens if you try to pickle non-TObject classes.

Thanks for looking at all of these PRs by the way! We hotfixed quite a few things so we can add a Python debug build to our CI, but now that we have it, it's very good to think about the implications of what the pydebug interpreter found more in detail.

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.

2 participants