-
Notifications
You must be signed in to change notification settings - Fork 11
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
Drop python 3.9 builds #97
Conversation
@@ -1,11 +1,11 @@ | |||
ARG CUDA_VER=11.8.0 | |||
ARG LINUX_VER=ubuntu20.04 | |||
ARG PYTHON_VER=3.9 | |||
ARG PYTHON_VER=3.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARG PYTHON_VER=3.10 | |
ARG PYTHON_VER=unset |
I just noticed this because I came here to do the same thing haha.
I strongly recommend changing all these build args (not just PYTHON_VER, but CUDA_VER, LINUX_VER, etc.) to a nonsense value. Benefits of that:
- makes it clearer that all this configuration is intended to provided via
--build-arg
- reduces the risk of accidentally falling back to a default value if build args aren't passed correctly (which could just silently build the wrong image)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks James! 🙏
This sounds like a good suggestion. That said, it looks like there are a bunch of places where this would need to be updated
Would it be alright if we raised an issue and worked on this separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's fine with me. I do want to see this PR get merged so we can finish dropping Python 3.9 across RAPIDS. Do you have write access here? (I don't)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah @jakirkham it looks like you do have write access here: https://github.com/orgs/rapidsai/teams/dask-build-environment-write
If you're comfortable with the current state of this PR, could you merge it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies I haven't stepped in for a while - think we need an approval from @rapidsai/dask-build-environment-write on this before we can merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks James! 🙏
Didn't want to just merge without discussing the reasonable point that you raised 😉
Approved. Will defer to Charles on when it makes sense to merge this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks John! Think this should be good to merge now, can follow up once builds have been rerun to confirm that the matrix no longer includes 3.9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you both! I'll put up an issue (and probably a PR) shortly with the other changes I described at the top of this thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you are able, please do James 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm that with the merging of this automated builds no longer include 3.9 🎉 thanks all! |
Thanks Charles and James! 🙏 |
With dask/community#373 closed out and rapidsai/build-planning#88 in progress, seems timely to drop 3.9 builds.