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

[ntuple] Binary format version 1.0.0.0 #16769

Merged
merged 5 commits into from
Nov 13, 2024
Merged

Conversation

jblomer
Copy link
Contributor

@jblomer jblomer commented Oct 28, 2024

Depends on #16645

@jblomer jblomer self-assigned this Oct 28, 2024
@jblomer jblomer marked this pull request as draft October 28, 2024 15:55
@hahnjo
Copy link
Member

hahnjo commented Oct 28, 2024

We should also adjust the tests that expect / allow "Pre-release format version" in the diagnostics

std::uint32_t fTypeVersionFrom = 0;
std::uint32_t fTypeVersionTo = 0;
/// Type version the extra type information is bound to
std::uint32_t fTypeVersion = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Can we explicit in the commit log why the From/To is no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -37,7 +37,7 @@ public:
{
}

const TClass *GetClass() const override { return TClass::GetClass<ROOT::Experimental::RNTuple>(); }
const TClass *GetClass() const override { return TClass::GetClass<ROOT::RNTuple>(); }
Copy link
Member

Choose a reason for hiding this comment

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

Not strictly needed (and maybe annoying to do at this point) but I would recommend to have the commit that move all the classes out of the Experimental namespace to be the very last commit in the PR (so that technically there is no 'normal' way to get a RNTuple file with non-experiemental names and a not 'completely' 1.0 format.

@hahnjo
Copy link
Member

hahnjo commented Oct 28, 2024

@pcanal please review the first 10-ish commits in #16645!

@pcanal
Copy link
Member

pcanal commented Oct 28, 2024

please review the first 10-ish commits in #16645!

Fair enough. However the comment #16769 (comment) is actually debating the split between this PR and #16645.

Copy link

github-actions bot commented Oct 28, 2024

Test Results

    19 files      19 suites   4d 4h 15m 56s ⏱️
 2 665 tests  2 665 ✅ 0 💤 0 ❌
48 742 runs  48 742 ✅ 0 💤 0 ❌

Results for commit 92e1aab.

♻️ This comment has been updated with latest results.

@jblomer
Copy link
Contributor Author

jblomer commented Oct 29, 2024

please review the first 10-ish commits in #16645!

Fair enough. However the comment #16769 (comment) is actually debating the split between this PR and #16645.

I don't see enough reason to reshuffle the commits or PRs at this point. We can safely detect files from all PR states (RC2, RC3, version 1.0).

@jblomer jblomer force-pushed the ntuple-v1 branch 5 times, most recently from cae1de5 to ccf5aa5 Compare October 31, 2024 13:03
@jblomer jblomer marked this pull request as ready for review October 31, 2024 13:15
@jblomer jblomer marked this pull request as draft October 31, 2024 15:19
@jblomer
Copy link
Contributor Author

jblomer commented Oct 31, 2024

As discussed, this should be merged only after a final round of tests with RC3

@jblomer jblomer added this to the 6.34.00 milestone Nov 4, 2024
@enirolf
Copy link
Contributor

enirolf commented Nov 5, 2024

The tutorials also mention that the data format is still experimental and subject to change. This can be addressed in a separate PR, but it's something we shouldn't forget about.

@jblomer jblomer merged commit 303a4e8 into root-project:master Nov 13, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants