conda / conda-lock

Lightweight lockfile for conda environments
https://conda.github.io/conda-lock/
Other
489 stars 103 forks source link

warn if cuda packages are installed implicitly #428

Closed minrk closed 1 year ago

minrk commented 1 year ago

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.

netlify[bot] commented 1 year ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit cd685578cae4c2049800fd2170cc628cbd20692b
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/6486fa6dcf817300080d7301
Deploy Preview https://deploy-preview-428--conda-lock.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 settings.

maresb commented 1 year ago

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?

minrk commented 1 year ago

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, cudatoolkit is ~always present when cuda's depended upon, but I can't say for sure. It's probably a 'good enough' indicator to start with.

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.

There seems to be a lot of ugly code associated with the default virtual package spec. Do you think it be feasible to instead replace most of this code with the parsing of a default-virtual-packages.yml file?

I'll have a look if I get a chance. Would need #429

minrk commented 1 year ago

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.

maresb commented 1 year ago

Thanks a lot for this great work! Sorry about the complexity issue. Would it work to add # noqa: C901 to the definition line of make_lock_files?

minrk commented 1 year ago

noqa comment works, thanks

maresb commented 1 year ago

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.