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

Add python bindings for the RNTuple backend #488

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

tmadlener
Copy link
Collaborator

@tmadlener tmadlener commented Sep 18, 2023

BEGINRELEASENOTES

  • Add python bindings for the RNTuple reader and writer
  • Make podio-dump understand RNTuple based files
  • Fix missing storage of datamodel definitions for RNTuple based files

ENDRELEASENOTES

Tried to get this to work with spack locally, but currently stumbling over root-project/root#11797 so I cannot build podio inside spack with ENABLE_RNTUPLE. Curious to see in how many of our CI environments this works.

@tmadlener
Copy link
Collaborator Author

I see very similar errors locally:

    File "/home/runner/work/podio/podio/tools/podio-dump", line 172, in <module>
      main(clargs)
    File "/home/runner/work/podio/podio/tools/podio-dump", line 111, in main
      reader = get_reader(args.inputfile)
    File "/home/runner/work/podio/podio/python/podio/reading.py", line 74, in get_reader
      return root_io.RNTupleReader(filename)
    File "/home/runner/work/podio/podio/python/podio/root_io.py", line 42, in __init__
      self._reader = podio.ROOTNTupleReader()
  TypeError: cannot instantiate incomplete class 'podio::ROOTNTupleReader'

I am not sure why this doesn't work. I can instantiate a ROOTNTupleReader outside of the test environment, but not inside it. My suspicion is (once again) a weird interplay between installed dictionaries and newly built ones for the tests. I haven't been able to resolve this locally due to the spack issues mentioned at the top.

@tmadlener tmadlener changed the title Make podio-dump support the RNTuple based backend Add python bindings for the RNTuple backend Nov 24, 2023
@tmadlener
Copy link
Collaborator Author

This is now working for me in a local installation, and I think it should also work for a key4hep based stack. I am not entirely sure how things look like in other environments.

python/podio/reading.py Outdated Show resolved Hide resolved
python/podio/reading.py Outdated Show resolved Hide resolved
tools/CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines +70 to +74
root_flavor = _determine_root_format(filename)
if root_flavor == RootFileFormat.TTREE:
return root_io.Reader(filename)
return root_io.LegacyReader(filename)
if root_flavor == RootFileFormat.RNTUPLE:
return root_io.RNTupleReader(filename)
if root_flavor == RootFileFormat.LEGACY:
return root_io.LegacyReader(filename)
Copy link
Member

@jmcarcell jmcarcell Dec 1, 2023

Choose a reason for hiding this comment

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

Maybe not needed now because there aren't that many choices but all of this can be changed by a basically one liner instead of all the if - return:

return {RootFileFormat.TTREE: root_io.LegacyReader, RootFileFormat.RNTUPLE: root_io.RNTupleReader, 
        RootFileFormat.LEGACY: root_io.LegacyReader}[_determine_root_format(filename)](filename)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would keep it as it is right now, because it is easier to read, and we could think about replacing it with match /case once we bump our minimum python version to 3.10.

@jmcarcell
Copy link
Member

I tested it with an rntuple with EDM4hep and it worked fine

Copy link
Collaborator Author

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks for testing.

@tmadlener tmadlener merged commit 26d51c9 into AIDASoft:master Dec 1, 2023
17 checks passed
@tmadlener tmadlener deleted the podio-dump-rntuple branch December 1, 2023 18:55
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