warn if cuda packages are installed implicitly #428
Conversation
✅ Deploy Preview for conda-lock ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Wow, this is very exciting! Too bad about the Click annoyance. What if we made the CUDA version mandatory? Now that I have your attention, can I ask a few quick questions about CUDA support, most of which are out-of-scope for this PR?
|
now you can't specify that you want cuda without specifying a version Default behavior is still 11.4
|
mandatory version should work fine. I'm afraid I can't really answer the cuda questions, since I'm not a cuda user. I'm mostly here for making it easier to not use cuda. In my experience, Re: the version bump, I think making the version string required makes it easier to leave the default implicit version alone. Then you need to specify the version if you want to avoid the warning or if you want a more recent version. The implicit cuda message can reflect that.
I'll have a look if I get a chance. Would need #429 |
|
flake8 complains about complexity. I don't really know how to respond to that other than suggesting that the complexity check should be removed. I don't think I have the authority to refactor to reduce complexity. |
|
Thanks a lot for this great work! Sorry about the complexity issue. Would it work to add |
|
noqa comment works, thanks |
minrk
changed the title
There was a problem hiding this comment.
I have a few minor stylistic nits to help improve readability.
But really I think this needs a look from a CUDA expert, unfortunately not me. @mariusvniekerk, could you please suggest someone?
conda_lock/conda_lock.py
Outdated
| _implicit_cuda_message = """ | ||
| 'cudatoolkit' package added implicitly without specifying that cuda packages | ||
| should be accepted. | ||
| Add a minimum cuda version via `--with-cuda VERSION` or via virtual packages |
There was a problem hiding this comment.
If I'm not mistaken, the version is not "minimum" but represents the exact version of CUDA which is available on the (hypothetical) target system.
There was a problem hiding this comment.
Yes, it's the version used for the solve, but in practice cuda versions are treated as lower bounds. So setting a version in the hypothetical solve is effectively setting the minimum version for which the locked environment can be used. But I switched it to 'specify' to avoid trying to say too much.
There was a problem hiding this comment.
Ah, this is good to know... I would have assumed that it's semver.
I'm still slightly confused, and I want to make sure I understand what you're writing. Would a typical pin be cuda >=11.4 or cuda <12? Thanks for your patience!
Co-authored-by: Ben Mares <[email protected]>
in practice it is a minumum, but what matters is it's solved assuming an exact version is available
|
I think this is really good and unlikely to break anything. I'm just going to merge in a few days if there's no further feedback. |
closes #426
doesn't change default behavior, but adds a warning if cuda is not requested but 'cudatoolkit' is pulled in, as suggested by @maresb. The presence of 'cudatoolkit' itself in the dependency list is considered sufficient for an 'explicit cuda' signal.
I could leave out that condition because the available version is still implicit, which may trip folks up.
I initially wanted to allow specifying a version with
--with-cuda [version], but click doesn't seem to want folks to have options with 0 or 1 args. Plus, virtual package specs already solve this, so it's not worth the fight with click.