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 merger fixes #16073

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Conversation

silverweed
Copy link
Contributor

This Pull request:

Moves some code out of the parallel tasks in RNTupleMerger (particularly, ThrowOnError which is not necessarily happily parallelizable).
It also avoids spawning the parallel tasks if not needed, i.e. if we don't need to change the compression of the pages.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

The first part of the parallel tasks code doesn't need to be
parallelized, and it can in fact be wrong when run in parallel
(specifically, ThrowOnError may not play well with tbb).
Also, we now avoid spawning the parallel tasks altogether if we don't
need to change the pages compression.
@silverweed silverweed removed the request for review from linev July 19, 2024 15:40
Copy link

Test Results

    13 files      13 suites   2d 19h 43m 54s ⏱️
 2 652 tests  2 651 ✅ 0 💤 1 ❌
32 658 runs  32 657 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 96af86f.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! As a future idea, it might be good to have a test that RNTupleMerger / hadd refuses to merge pages that fail the checksum test.

@silverweed silverweed merged commit e1abcf1 into root-project:master Jul 22, 2024
16 of 18 checks passed
@silverweed silverweed deleted the ntuple_merger_fixes branch July 22, 2024 07:08
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.

None yet

2 participants