-
Notifications
You must be signed in to change notification settings - Fork 65
23.08 Release Selector and Install Page Updates #414
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
Conversation
|
I think this is good to review. I was initially holding back to consider adding the docker changes to the install page, Selector changes are updated for Docker since CUDA 12 update impacts that as well. Because arm isn't yet supported in CUDA 12, I've left 11.8 as the default. Happy to discuss/change. |
|
@raydouglass would also appreciate your eyes on the selector when you find a moment to make sure I set up the Docker changes correctly - thanks! (I can't add you as a reviewer otherwise I'd just do that.) |
bdice
left a comment
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.
Comments attached.
|
Oh one other question I had for the group: with
|
We've wanted to stay in line with conda defaults as much as possible. I am not sure that updated versions of conda are widespread enough to recommend this to all users. However, I'm very eager for it to become the default and recommend that all users update their conda versions... |
Yeah this is a good point, conda installs do often feel like a "set and forget" thing. It's not beautiful, but we could do: Not strongly attached to this, but think it's worth bringing up just because mamba is so much faster and if people can use it, they should. |
|
We discussed this a while back and agreed to recommend cc @jakirkham @raydouglass for viz |
|
💯 to recommending it, the challenge is when to assume it. If we include However, the error you get from conda is very clear if you try to use (My weak pref is we do include it and add a troubleshooting step in the docs for when libmamba isn't installed.) |
|
Well yes, we also need to add / link to instructions on how to install it. I don't suppose we want yet another options in the selector. |
I like this proposal! Let’s add it “always” in our commands and then give clear troubleshooting instructions on how to fix it if their conda version is too old. Include a copy of the error message so it’s easy to find. Maybe add a note at the bottom of the conda selector like “We add |
ajschmidt8
left a comment
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.
almost there! there are a lot of changes in this release 🙂
I think this should also include It looks like the other labels have that property, but not the command label. |
jakirkham
left a comment
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 Ben! 🙏
Looks good from my perspective. Will defer to others on JS, etc.
ajschmidt8
left a comment
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.
this is awesome.
the release selector is sooo much simpler as a result of the Docker image changes.
bdice
left a comment
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.
A few remaining comments, but otherwise LGTM!
| var py_cuda_pkgs = [this.highlightPkgOrImg("python") + "=" + python_version, this.highlightPkgOrImg("cuda-version") + "=" + cuda_version].join(" "); | ||
| var conda_channels = [rapids_channel, "conda-forge", "nvidia"] | ||
| .map(ch => this.highlightFlag("-c") + " " + ch + " ") | ||
| .map(ch => "-" + this.highlightFlag("c") + " " + ch + " ") |
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.
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.
interesting. okay. I will update in a bit. I just made them match the existing highlights.
| var all_pkgs = pkgs_vers + py_cuda_pkgs; | ||
|
|
||
| var conda_create_ln = this.highlightCmd('conda') + " create " + this.highlightFlag("-n") + " rapids-" + rapids_version + " " + conda_channels; | ||
| var conda_create_ln = this.highlightCmd('conda') + " create --" + this.highlightFlag("solver") + "=libmamba " + this.highlightFlag("-n") + " rapids-" + rapids_version + " " + conda_channels; |
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.
| var conda_create_ln = this.highlightCmd('conda') + " create --" + this.highlightFlag("solver") + "=libmamba " + this.highlightFlag("-n") + " rapids-" + rapids_version + " " + conda_channels; | |
| var conda_create_ln = this.highlightCmd('conda') + " create " + this.highlightFlag("--solver") + "=libmamba " + this.highlightFlag("-n") + " rapids-" + rapids_version + " " + conda_channels; |
| var pip_install = "pip install "; | ||
| var index_url = " --extra-index-url=https://pypi.nvidia.com"; | ||
| var pip_install = `${this.highlightCmd("pip")} install`; | ||
| var index_url = `--${this.highlightFlag("extra-index-url")}=https://pypi.nvidia.com`; |
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.
| var index_url = `--${this.highlightFlag("extra-index-url")}=https://pypi.nvidia.com`; | |
| var index_url = `${this.highlightFlag("--extra-index-url")}=https://pypi.nvidia.com`; |
|
|
||
| var indent = " "; | ||
| var cmd = this.highlightCmd("docker") + " run --" + this.highlightFlag("gpus") + " all --pull always --" + this.highlightFlag("rm") + " -" + this.highlightFlag("it") + " \\\n" + | ||
| var cmd = this.highlightCmd("docker") + " run --" + this.highlightFlag("gpus") + " all --" + this.highlightFlag("pull") + " always --" + this.highlightFlag("rm") + " -" + this.highlightFlag("it") + " \\\n" + |
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.
| var cmd = this.highlightCmd("docker") + " run --" + this.highlightFlag("gpus") + " all --" + this.highlightFlag("pull") + " always --" + this.highlightFlag("rm") + " -" + this.highlightFlag("it") + " \\\n" + | |
| var cmd = this.highlightCmd("docker") + " run " + this.highlightFlag("--gpus") + " all " + this.highlightFlag("--pull") + " always " + this.highlightFlag("--rm") + " " + this.highlightFlag("-it") + " \\\n" + |
| var indent = " "; | ||
| var cmd = this.highlightCmd("docker") + " run --" + this.highlightFlag("gpus") + " all --pull always --" + this.highlightFlag("rm") + " -" + this.highlightFlag("it") + " \\\n" + | ||
| var cmd = this.highlightCmd("docker") + " run --" + this.highlightFlag("gpus") + " all --" + this.highlightFlag("pull") + " always --" + this.highlightFlag("rm") + " -" + this.highlightFlag("it") + " \\\n" + | ||
| indent + "--" + this.highlightFlag("shm-size") + "=1g --" + this.highlightFlag("ulimit") + " memlock=-1 --" + this.highlightFlag("ulimit") + " stack=67108864" + " \\\n" + |
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.
| indent + "--" + this.highlightFlag("shm-size") + "=1g --" + this.highlightFlag("ulimit") + " memlock=-1 --" + this.highlightFlag("ulimit") + " stack=67108864" + " \\\n" + | |
| indent + this.highlightFlag("--shm-size") + "=1g " + this.highlightFlag("--ulimit") + " memlock=-1 " + this.highlightFlag("--ulimit") + " stack=67108864" + " \\\n" + |
Co-authored-by: Bradley Dice <[email protected]>
|
Somewhat perplexed why the deploy preview isn't showing the cuProj updates. My local preview shows fine, but it's like they're not there at all in netlify. |
Must've been a hiccup on Netlify's end. I reran the build from our admin dashboard and it looks like it's showing now. |
|
Is there anything else needed here? |
|
@jakirkham It's all good - @ajschmidt8 just wants a moment to verify/modify highlighting per #414 (comment) but otherwise no concerns. Edit to add: it's not super high prio on his radar since the link above is the last thing before merge and lots else going on so movement might not happen until closer to release day. |
|
Maybe let's do final reviews. Think we can punt on smaller stylistic changes |
+1. Bradley, I don't want to merge your suggestions without reviewing them first, but I don't have time to check them all for consistency at the moment. We can address them in a future PR. |
|
Thanks all! 🙏 |

closes #413
This PR contains the updates to the release selector required for the conda-forge CUDA 12.0 work as well as the docker overhaul.
To do:
23.08release updatesThis PR updates:
PyCaretas a conda additional package, it's now only available on pip for the newest versionsbaseandnotebooksI've also merged in #412 to simplify merging at release.