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

Remove xfail for KBinsDiscretizer quantile tests #3804

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Apr 28, 2021

Following the update to cupy 8.5.0, the bad read in the cupy.percentile kernel should no longer be an issue, allowing us to remove the xfail on this test.

Close #2933

Following the update to cupy 8.5.0, the bad read in the
`cupy.percentile` kernel should no longer be an issue, allowing us to
remove the xfail on this test.

Close rapidsai#2933
@wphicks wphicks added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Apr 28, 2021
@wphicks wphicks requested a review from a team as a code owner April 28, 2021 17:39
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Apr 28, 2021
@dantegd
Copy link
Member

dantegd commented Apr 28, 2021

@wphicks I know that this might be overkill, but do you think it'd be a good idea to run the test a large number of times locally, though a CI run that only runs that test a number of times might be good too, be a good idea?

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.20@f87ca91). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.20    #3804   +/-   ##
==============================================
  Coverage               ?   86.04%           
==============================================
  Files                  ?      225           
  Lines                  ?    17113           
  Branches               ?        0           
==============================================
  Hits                   ?    14725           
  Misses                 ?     2388           
  Partials               ?        0           
Flag Coverage Δ
dask 49.29% <0.00%> (?)
non-dask 77.95% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f87ca91...c3fdc55. Read the comment docs.

@wphicks
Copy link
Contributor Author

wphicks commented Apr 28, 2021

do you think it'd be a good idea to run the test a large number of times locally, though a CI run that only runs that test a number of times might be good too, be a good idea?

I don't mind doing that again if it would give us more confidence in this, but as I was initially investigating #3481, I ran this some huge number of times (tens of thousands maybe?) without seeing a failure. I also have a fair amount of confidence that we found the root cause on this one, though it's always possible that I'm wrong. Want me to spin up another run overnight, or do we feel good enough with that level of testing?

@dantegd
Copy link
Member

dantegd commented Apr 28, 2021

I think that level of testing is good, I had forgotten the journey of the bug a little bit, so will go ahead and merge

@dantegd
Copy link
Member

dantegd commented Apr 28, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 97fa089 into rapidsai:branch-0.20 Apr 28, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Following the update to cupy 8.5.0, the bad read in the `cupy.percentile` kernel should no longer be an issue, allowing us to remove the xfail on this test.

Close rapidsai#2933

Authors:
  - William Hicks (https://github.com/wphicks)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#3804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Sporadic KBinsDiscretizer pytests fail with quantile strategy
3 participants