root icon indicating copy to clipboard operation
root copied to clipboard

Python class inherited from a ROOT class fails in last release

Open PPaye opened this issue 1 year ago • 1 comments

Check duplicate issues.

  • [ ] Checked for duplicates

Description

Inherit whatever ROOT parent class in a python class generates error on the TObject::DoError method. Python raises an error on TypeError. Which is categorical wrong, it should be NotImplemented.

At the root-forum it is being suggested that RDataFrame is not a class to be inherited in PyRoot; due to "composition over inheritance" idiom and warnings on bad data type manipulation internally leading to wrong numerical results. Is this going to be for all ROOT classes? No ROOT class should be inherited in a Python class from now on ?

It only affects the last release 6.32.04 and 6.32.06 Release 6.30.02 works fine.

Similar issues: https://github.com/root-project/root/issues/12391 https://root-forum.cern.ch/t/rdataframe-has-no-virtual-destructor/53605 https://root-forum.cern.ch/t/typeerror-no-python-side-overrides-supported-failed-to-compile-the-dispatcher-code/53198/14

Reproducer

from ROOT import TObject

class C(TObject):
    def __init__(self):
        super().__init__()  # Initialize the base class

    def some_method(self):
        print("This is a method in class C.")
  In [1]: from ROOT import TObject
        ...: 
        ...: class C(TObject):
        ...:     def __init__(self):
        ...:         super().__init__()  # Call the constructor of TObject
        ...: 
Installed ROOT event loop hook.
input_line_35:10:62: error: unknown type name '__va_list_tag'
  void DoError(int arg0, const char* arg1, const char* arg2, __va_list_tag[1] arg3) const {
                                                             ^
input_line_35:10:79: error: expected ')'
  void DoError(int arg0, const char* arg1, const char* arg2, __va_list_tag[1] arg3) const {
                                                                              ^
input_line_35:10:15: note: to match this '('
  void DoError(int arg0, const char* arg1, const char* arg2, __va_list_tag[1] arg3) const {
              ^
input_line_35:11:47: error: use of undeclared identifier 'arg3'
    return TObject::DoError(arg0, arg1, arg2, arg3);
                                              ^
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[1], line 3
      1 from ROOT import TObject
----> 3 class C(TObject):
      4     def __init__(self):
      5         super().__init__()  # Call the constructor of TObject

TypeError: no python-side overrides supported (failed to compile the dispatcher code)

ROOT version

ROOT Version: 6.32.02 Built for linuxx8664gcc on Jun 18 2024, 04:46:14 From tags/v6-32-02@v6-32-02

ROOT Version: 6.32.06 Built for linuxx8664gcc on Sep 21 2024, 19:19:59 From tags/v6-32-06@v6-32-06

Installation method

pre-built binary

Operating system

Linux, Ubuntu 23

Additional context

No response

PPaye avatar Sep 25 '24 02:09 PPaye

Dear @PPaye ,

Thanks for reaching out to us! At a first glance, I would say this is a degradation and we should try to understand why that happened, generally speaking.

But please note the following:

At the root-forum it is being suggested that RDataFrame is not a class to be inherited in PyRoot;

The answer on the ROOT forum does not say that RDataFrame should not be inherited-from in a Python class. It should not be inherited-from in general. The class explicitly does not provide a virtual destructor, so virtual inheritance would not work even in C++. This is part of the class design and it is not related to this issue with Python.

vepadulano avatar Sep 26 '24 07:09 vepadulano

Hi @PPaye,

As @vepadulano mentioned, the RDataFrame class should not be inherited from. For the error that you face, the issue lies in exposing the protected DoError method that uses the __va_list_tag type. This is done by the cppyy crossinheritance dispatcher which exposes it through re-declaration and forwarding (protected methods and data need their access changed in the C++ trampoline)

The creation of this metaclass when performing:

from ROOT import TObject

class C(TObject):
    def __init__(self):
        super().__init__()  # Initialize the base class

    def some_method(self):
        print("This is a method in class C.")

triggers the input_line_35:10:62: error: unknown type name '__va_list_tag' error that comes from the JIT when compiling the proxy class. Since the DoError method in TObject is virtual, we prefer not to modify the type since other classes like TThread override the same

A straightforward workaround to inheriting TObject would be to subclass TObject via the interpreter and then perform the crossinheritance, allowing you to use methods like TObject::Print():

import ROOT

ROOT.gInterpreter.Declare("""

class MyObject : public TObject{};

""")

class C(ROOT.MyObject):
    def __init__(self):
        super().__init__()  # Initialize the base class
        self.Print()

    def some_method(self):
        print("This is a method in class C.")

c = C()

giving

OBJ: TObject TObject Basic ROOT object

as expected

aaronj0 avatar Feb 05 '25 15:02 aaronj0

Dear @PPaye ,

I'd also like to point out that while this workaround supports your usecase, you can still inherit from other ROOT classes like TNamed without it:

import ROOT

class C(ROOT.TNamed):
    def __init__(self):
        super().__init__()  # Initialize the base class
        self.Print()

    def some_method(self):
        print("This is a method in class C.")

c = C()
OBJ: TNamed

The reason for this regression, as mentioned above, stems from the access to protected methods and data. The signature with __va_list_tag cannot be modified either, which is why the workaround was suggested. I would like to close this issue as this paticular bug is a special case that cannot be enabled without changing cppyy's behaviour, and crossinheritance with other classes like TNamed is still supported.

aaronj0 avatar Feb 06 '25 13:02 aaronj0