-
-
Notifications
You must be signed in to change notification settings - Fork 22
Support RNTuple as possible output format #1026
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
Conversation
Good to go here I think for RNTuple output (see #999). I do not recommend rntuple yet (uproot does not yet have compression enabled for the output in 5.6.0) but the code paths technically work and produce correct output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (1)
- code_generator_raw_uproot/transformer_capabilities.json: Language not supported
Comments suppressed due to low confidence (2)
code_generator_raw_uproot/servicex/templates/transform_single_file.py:29
- In the root-file branch, the previous conditional that checked if v[0].fields existed was removed. Consider reinstating or handling the case where v[0].fields might be empty to avoid potential runtime errors.
if output_format in ('root-file', 'root-rntuple'):
code_generator_raw_uproot/servicex/raw_uproot_code_generator/request_translator.py:150
- The change from constructing a dict of field types to directly returning arr.layout may affect consumers expecting a specific structure. Ensure that downstream code is compatible with this new format.
rv_arrays_trees[outtreename] = (None, arr.layout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh - copilot is now reviewing our PRs!
Ok - this looks fine - My only comments are style comments (nested if statements, etc.). I also cleaned up the PR text because it is often used to generate change logs. I'll mark approve, but you might made modifications regardless.
I should note that the next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent
uproot-raw
andfunc_adl_uproot
transformersuproot
5.6 doesn't yet implement compression when writing RNTuple, so this option is not yet recommended for production.