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

Run GPU tests on python 3.8 & 3.10 #9940

Merged
merged 4 commits into from
Feb 14, 2023
Merged

Conversation

charlesbluca
Copy link
Member

In rapidsai/dask-build-environment#58, @jakirkham suggested that it might be worth running GPU testing against python 3.8 (currently the lowest Python version supported by RAPIDS):

To ensure RAPIDS + Dask work on Python 3.8, it may be worth testing on that version. Presumably Python 3.10 will still work in that case. However if Python 3.10 is tested and a Python 3.9 or 3.10 feature is used in RAPIDS-specific code path in Dask, it may work when tested against Python 3.10, but not work in Python 3.8 environment should users try that.

Interested in if any failures will crop up here, and also to solicit opinions from other RAPIDS maintainers (cc @pentschev @quasiben)

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

@pentschev
Copy link
Member

This makes sense to me, best to test with the oldest support. Thanks @charlesbluca and @jakirkham !

@jakirkham
Copy link
Member

Thanks Charles! 🙏

Is there anything else needed before marking this ready for review?

@charlesbluca charlesbluca marked this pull request as ready for review February 11, 2023 15:56
@charlesbluca
Copy link
Member Author

Chatted with @ajschmidt8 and seems like it's pretty trivial to run testing for both python 3.8 and 3.10 on each PR (a run for example):

https://gpuci.gpuopenanalytics.com/job/dask/job/dask/job/prb/job/dask-prb/3428/

Given this, how do we feel about running the GPU tests on both 3.8 and 3.10? Don't think should have much of an impact on the overall runtime of CI

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @charlesbluca!

both python 3.8 and 3.10 on each PR

I'm curious why Python 3.10 instead of Python 3.11?

@charlesbluca
Copy link
Member Author

I'm curious why Python 3.10 instead of Python 3.11?

RAPIDS deprecated python 3.9 support in 22.12, and moving forward plans to maintain support for 3.8 and 3.10.

@charlesbluca charlesbluca changed the title Run GPU tests on python 3.8 Run GPU tests on python 3.8 & 3.10 Feb 13, 2023
@jrbourbeau
Copy link
Member

RAPIDS deprecated python 3.9 support in 22.12, and moving forward plans to maintain support for 3.8 and 3.10.

Ah, I see. I hadn't realized that, thanks for clarifying.

I'll defer to folks like yourself, John, and Peter about what's best here. It looks like testing on 3.8 and 3.10 doesn't have a big impact on overall CI runtime, so I think we're good on that front 👍

@jakirkham
Copy link
Member

I'm curious why Python 3.10 instead of Python 3.11?

RAPIDS deprecated python 3.9 support in 22.12, and moving forward plans to maintain support for 3.8 and 3.10.

Just to add to Charles' response, RAPIDS only recently added Python 3.10 support and has not yet added Python 3.11 support.

@jakirkham
Copy link
Member

Given this, how do we feel about running the GPU tests on both 3.8 and 3.10? Don't think should have much of an impact on the overall runtime of CI

+1

As long as we include Python 3.8, happy with any other CI jobs.

Comment on lines -2765 to +2766
expect = pdf.groupby(pd_grouper).sum()
# cuDF's numeric behavior aligns with numeric_only=True
expect = pdf.groupby(pd_grouper).sum(numeric_only=True)
Copy link
Member

Choose a reason for hiding this comment

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

cc @rjzamora (for awareness)

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @charlesbluca

@jrbourbeau jrbourbeau merged commit 512e414 into dask:main Feb 14, 2023
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.

4 participants