-
Notifications
You must be signed in to change notification settings - Fork 51
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
install selector: Drop python 3.9 for nightlies, default to CUDA 12.5 for stable and nightly #534
Conversation
✅ Deploy Preview for docs-rapids-ai ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
conda_cuda_vers: ["11", "12"], | ||
pip_cuda_vers: ["11.4 - 11.8", "12"], | ||
docker_cuda_vers: ["11.8", "12.0", "12.2", "12.5"], | ||
docker_cuda_vers: ["11.8", "12.0", "12.5"], |
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.
12.5 images have been available since 24.08: rapidsai/docker#689
12.2 images will be dropped in 24.10: rapidsai/docker#696
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.
I'm proposing just unconditionally dropping 12.2 for simplicity and to nudge people towards newer images.
But if reviewers disagree, I'd be happy to introduce a similar structure to what I'm proposing for Python versions, that allows the CUDA versions to differ by release. I think that'd be generically useful, just didn't add it here to limit the size of this PR.
✅ Deploy Preview for docs-rapids-ai ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -22,7 +22,7 @@ docker run --rm \ | |||
--volume="$PWD:/srv/jekyll" \ | |||
--publish [::1]:4000:4000 \ | |||
jekyll/jekyll \ | |||
jekyll serve | |||
bash -c 'rm -rf ./_site && jekyll serve' |
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.
Following these testing steps as documented worked... but only the first time.
This command leaves behind a _site/
directory with file permissions showing the default user inside the container as the owner. Where that user differs from the default user on the system where you run docker run
, the second attempt to run this fails like this:
chown: _site/assets/data: Permission denied
chown: _site/assets/data: Permission denied
chown: _site/assets: Permission denied
chown: _site/assets: Permission denied
chown: _site: Permission denied
chown: _site: Permission denied
Removing that directory at startup every time avoids that tiny bit of development friction.
Usually I'd workaround this by adding a bit of logic in the |
I can add something like that. |
Ok, think I've handled that case: e6073f3 Try it out at https://deploy-preview-534--docs-rapids-ai.netlify.app/install
You should see the following:
Tested the same thing with Docker, that looks like it's working as well. I've written very little JavaScript, so I have no idea if what I'm doing is performant (and I know performance can matter for search rankings). Please do let me know if there are different data structures or patterns you think I should be using. |
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! 🙏
LGTM
Let's see if Ben has any more thoughts 🙂
The code changes lgtm, I like that you made a more permanent implementation vs one that would need to be removed like I suggested. My only q - the deploy preview isn't working for me and it doesn't look like netlify is reporting an error: https://www.netlifystatus.com/ Has anyone been able to check it? Just want to be sure there's not something causing an issue in the selector. |
Yep I used the newest preview to test the installer selector to see how it behaved after the fix. It did the right thing of bumping to Python 3.10. Going back to stable it kept Python 3.10, which seems like a good approach Am using macOS Safari and in statuses clicking FWIW how are you viewing the preview? Are you on a different OS or browser? Curious just to make sure we don't have a JS bug in one context |
Looks like it's working for me now - (I'm on mac firefox, but yeah no issue now.) LGTM - thanks @jameslamb |
Thanks James and Ben! 🙏 |
Contributes to rapidsai/build-planning#88.
Proposes the following changes to the install selector:
Notes for Reviewers
How I tested this
Followed the instructions in the README. Ran locally and saw what I expected... Python 3.9 disabled when I tried selected
Nightly
.But enabled when I selected
Stable