rapidsai / docs

RAPIDS Documentation Site
https://docs.rapids.ai
36 stars 50 forks source link

Use CUDA version ranges in release selector. #524

Closed bdice closed 3 months ago

bdice commented 3 months ago

This PR uses CUDA version ranges in the release selector. This follows from discussions in https://github.com/rapidsai/docs/pull/522.

netlify[bot] commented 3 months ago

Deploy Preview for docs-rapids-ai ready!

Built without sensitive environment variables

Name Link
Latest commit ab1bbce41968893d9f0d03aff60fa06fac5725f7
Latest deploy log https://app.netlify.com/sites/docs-rapids-ai/deploys/669f26653dfcc00008065d58
Deploy Preview https://deploy-preview-524--docs-rapids-ai.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

bdice commented 3 months ago

@jarmak-nv @jakirkham Can you review and play with the site preview to see if this works like you expect?

I tried to get one button for conda to have CUDA 12.0-12.2 that would change to 12.0-12.5 when enabling nightlies but I didn't want to mess with any more of this selector logic at the moment.

jarmak-nv commented 3 months ago

Logic seems good, no issues after lots of clicks/testing.

I think having both 12.0-12.2 and 12.0-12.5 buttons is a bit awkward though. While I like the specificity of ranges, I think I'm still lightly in favor of just cuda 11 cuda 12 and cuda 12 nightly or something.

jakirkham commented 3 months ago

If we don't show the version ranges on the buttons, where do we specify these ranges?

jakirkham commented 3 months ago

Thanks Bradley! 🙏

This is looking pretty good


Noticed one thing with pip that I wanted to highlight (though maybe you already saw this)

Screenshot 2024-07-22 at 7 57 18 PM

All the others look ok. So probably something particular to the pip case

bdice commented 3 months ago

@jakirkham Glad this is going in the right direction -- I'm still working on some improvements, and I'm using CI as my test for website rendering. I may push some commits that don't work. I will let you know when this is ready for a final look.

bdice commented 3 months ago

@jakirkham @jarmak-nv I think this is ready to review.

jarmak-nv commented 3 months ago

If we don't show the version ranges on the buttons, where do we specify these ranges?

Yeah that's the challenge, my thought is that the selector should not be the main way of communicating what ranges of CUDA 12 the user can install alongside RAPIDS, but it does serve that purpose. Ranges work for this and the current implementation looks very nice/clean so I'm happy here.


I like how this works and the back-end changes! Thanks @bdice

Only concern is we no longer highlight that the nightly has a higher CUDA version available than the stable. Not sure if there's a straightforward path to resolve this or if it's a big deal.


Also on testing I noticed something I think might be considered undesirable, when selecting pip as the installation method, it moves you to CUDA 12 regardless of your previous selection. I think recently we've been settling on "don't change something the user has set unless the selection cannot work" and in that design theme we'd want to remove this behavior.

I think its controlled by the code here: https://github.com/rapidsai/docs/blob/ab1bbce41968893d9f0d03aff60fa06fac5725f7/_includes/selector.html#L771C1-L773C18

bdice commented 3 months ago

Only concern is we no longer highlight that the nightly has a higher CUDA version available than the stable. Not sure if there's a straightforward path to resolve this or if it's a big deal.

I am leaning towards "this is not a big deal." I think most users are happy as long as some CUDA minor version for the selected major version works with their environment.

bdice commented 3 months ago

Also on testing I noticed something I think might be considered undesirable, when selecting pip as the installation method, it moves you to CUDA 12 regardless of your previous selection.

I will revisit this in a follow-up PR. I have a solution in mind but I think this PR is in a fairly complete state and would be better to merge as is.

jakirkham commented 3 months ago

Only concern is we no longer highlight that the nightly has a higher CUDA version available than the stable. Not sure if there's a straightforward path to resolve this or if it's a big deal.

I am leaning towards "this is not a big deal." I think most users are happy as long as some CUDA minor version for the selected major version works with their environment.

Agreed. The window where these differ is usually short. So don't think it is worth the effort

jakirkham commented 3 months ago

FWIW am ok merging and iterating in follow ups. This feels like a substantial improvement

jakirkham commented 3 months ago

Thank you both! 🙏