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

Revert "Fix empty cluster handling in tdigest merge (#16675)" #16800

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

jihoonson
Copy link
Contributor

Description

This PR reverts #16675, which has introduced another bug. Our nightly CI pipeline is failing because of this bug (NVIDIA/spark-rapids#11463). I can reproduce the bug within a libcudf unit test. I will make another PR to fix both the original issue and the new bug.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jihoonson jihoonson requested a review from a team as a code owner September 11, 2024 21:37
Copy link

copy-pr-bot bot commented Sep 11, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 11, 2024
@jihoonson
Copy link
Contributor Author

/ok to test

@abellina abellina added bug Something isn't working non-breaking Non-breaking change labels Sep 11, 2024
@abellina
Copy link
Contributor

abellina commented Sep 11, 2024

@ttnghia @nvdbaranec fyi since you helped review the original change. Can you give it a look?

@ttnghia
Copy link
Contributor

ttnghia commented Sep 11, 2024

Is the bug easy to fix? Can we just fix it instead of revert then fix?

@jihoonson
Copy link
Contributor Author

Is the bug easy to fix? Can we just fix it instead of revert then fix?

I haven't figure it out yet. I would prefer reverting the change and not letting the CI fail over the whole long weekend..

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Approving the reversion until we can get a proper fix.

@ttnghia
Copy link
Contributor

ttnghia commented Sep 11, 2024

/ok to test

@ttnghia
Copy link
Contributor

ttnghia commented Sep 11, 2024

/merge

@vyasr
Copy link
Contributor

vyasr commented Sep 20, 2024

@jihoonson @ttnghia could we open an issue to track that there is still a bug that needs to be fixed?

@jihoonson
Copy link
Contributor Author

@vyasr thanks for the suggestion. Created #16881.

I also have a proper (I think) fix now. Will make a PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants