Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: show untyped fields and make keys compatible with coffea #1376

Merged
merged 5 commits into from
Feb 7, 2025

Conversation

ariostas
Copy link
Collaborator

@ariostas ariostas commented Feb 6, 2025

I changed things so that untyped fields now show up in keys. Previously, I was ignoring those since I thought they would not be useful, but that's not true. Also, for now I still ignore fields whose name starts with _, but maybe I'll also change this later so that everything appears even though it might come with a lot of garbage.

I also changed the keys function so that it takes some arbitrary kwargs and simply ignores them. This fixes #1350, but it is a temporary solution, as coffea needs to switch to using the right kwargs once they fully move to using RNTuples.

The keys function is still missing most functionality, but in the interest of having coffea working, I'll leave that for a future PR.

@ariostas ariostas requested a review from ianna February 6, 2025 18:33
@ariostas ariostas marked this pull request as draft February 6, 2025 18:40
@ariostas
Copy link
Collaborator Author

ariostas commented Feb 6, 2025

There are further issues. I'll mark it ready for review once the notebook from #1350 works.

@ariostas
Copy link
Collaborator Author

ariostas commented Feb 6, 2025

@oshadura This is all I can do from the Uproot side. There are more issues because coffea is assuming that it's handling a TTree object, so it's trying to call methods that have no equivalent for RNTuple. I'll try to find some time to look into the coffea side and fix things there.

@ariostas ariostas marked this pull request as ready for review February 6, 2025 18:59
Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ariostas - Thanks! Could you, please, have a look at the following test that is failing on ubuntu with Python 3.9:

______________________________ test_support_leafG ______________________________
TypeError: len() of unsized object

The above exception was the direct cause of the following exception:

tmp_path = PosixPath('/tmp/pytest-of-runner/pytest-0/test_support_leafG0')

    def test_support_leafG(tmp_path):
        filename = os.path.join(tmp_path, "testy.root")
        f = ROOT.TFile(filename, "recreate")
        t = ROOT.TTree("mytree", "example tree")
    
        n = np.array(2, dtype=np.int32)
>       t.Branch("mynum", n, "mynum/I")

f          = <cppyy.gbl.TFile object at 0x55f7d34bc640>
filename   = '/tmp/pytest-of-runner/pytest-0/test_support_leafG0/testy.root'
n          = array(2, dtype=int32)
t          = <cppyy.gbl.TTree object at 0x55f7d7f8a980>
tmp_path   = PosixPath('/tmp/pytest-of-runner/pytest-0/test_support_leafG0')

tests/test_0840_support_tleafG.py:16: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <cppyy.gbl.TTree object at 0x55f7d7f8a980>
args = ('mynum', array(2, dtype=int32), 'mynum/I')

    def _Branch(self, *args):
        # Modify the behaviour if args is one of:
        # ( const char*, void*, const char*, Int_t = 32000 )
        # ( const char*, const char*, T**, Int_t = 32000, Int_t = 99 )
        # ( const char*, T**, Int_t = 32000, Int_t = 99 )
>       res = BranchPyz(self, *args)
E       SystemError: <built-in function BranchPyz> returned a result with an error set

args       = ('mynum', array(2, dtype=int32), 'mynum/I')
self       = <cppyy.gbl.TTree object at 0x55f7d7f8a980>

../../../miniconda3/envs/test/lib/python3.9/site-packages/ROOT/_pythonization/_ttree.py:214: SystemError

Re-running the workflow did not help.
Thanks!

@oshadura
Copy link
Collaborator

oshadura commented Feb 7, 2025

@ariostas many thanks for quick fix!

@ariostas
Copy link
Collaborator Author

ariostas commented Feb 7, 2025

@ianna it must have been that some dependency broke something. I ran the workflow on main and it also failed. I'll look into it.

https://github.com/scikit-hep/uproot5/actions/runs/13201682460/job/36854995215

@ariostas
Copy link
Collaborator Author

ariostas commented Feb 7, 2025

I was able to reproduce the error. The issue is actually coming from ROOT, which was updated on conda-forge yesterday. Reverting to ROOT 6.32.2 solves the issue. So it probably was that they dropped support for something that comes along with Python 3.9. Given that the issue is with ROOT and not Uproot, I think we should just skip that test for Python 3.9. I can open a small PR to fix it if you agree @ianna

@ianna
Copy link
Collaborator

ianna commented Feb 7, 2025

I was able to reproduce the error. The issue is actually coming from ROOT, which was updated on conda-forge yesterday. Reverting to ROOT 6.32.2 solves the issue. So it probably was that they dropped support for something that comes along with Python 3.9. Given that the issue is with ROOT and not Uproot, I think we should just skip that test for Python 3.9. I can open a small PR to fix it if you agree @ianna

@ariostas - thanks for looking into it! Yes, I think we can skip the test as Python 3.9 is due to reach its scheduled upstream end-of-life in October 2025.

@ariostas
Copy link
Collaborator Author

ariostas commented Feb 7, 2025

The fix is in #1377. Turns out it wasn't ROOT, but I fixed the test there.

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ariostas - Thanks! I'll enable auto-merge.

@ianna ianna enabled auto-merge (squash) February 7, 2025 21:44
@ianna ianna merged commit e6e4f3a into main Feb 7, 2025
26 checks passed
@ianna ianna deleted the ariostas/fix_keys branch February 7, 2025 21:59
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.

TypeError: Model_ROOT_3a3a_RNTuple.keys() got an unexpected keyword argument 'filter_branch'
3 participants