Skip to content
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

In no CUDA case, set cuda* variables undefined #4534

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakirkham
Copy link
Member

Instead of setting cuda* variables to None in the no CUDA (CPU case), set them to undefined. This works better with Jinja filters (|), which makes it easier to work with.


Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Instead of setting `cuda*` variables to `None` in the no CUDA (CPU
case), set them to `undefined`. This works better with Jinja filters
(`|`), which makes it easier to work with.
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@mikemhenry
Copy link
Contributor

I think that would make things like .startswith work cleaner, but people will be grumpy with this breaking old version selectors, but I am in favor of that since going forward it makes things easier.

@jakirkham
Copy link
Member Author

Well said!

IIUC think it would be using |startswith (Jinja handles undefined as "" in that case unlike .startswith)


Just to contextualize this, a line like this...

- libcublas-dev      # [(cuda_compiler_version or "").startswith("12")]

...could become...

- libcublas-dev      # [cuda_compiler_version|startswith("12")]

@mikemhenry
Copy link
Contributor

I forgot, one other foot gun is that if anyone is treating None as false, changing it to a string will make it truthy, so it could silently break things.

I am all for this change, just trying to enumerate some of the edge cases that will make people grumpy when their feedstocks get busted.

@jakirkham
Copy link
Member Author

Yeah think the main issue is when recipes compare to None explicitly. Seems there are a few cases of this

@mikemhenry
Copy link
Contributor

For me as a feedstock maintainer, if the migration bot for this included an explanation of what changed, and a few examples on how to migrate, I think it would be helpful.

But since this isn't a migration, I think people will re-render and not get any explanation on what changed and will just be confused.

@jakirkham
Copy link
Member Author

That's true we could put this in a migrator instead. Also could include a custom commit message with a detail about what changed

@mikemhenry
Copy link
Contributor

I think that would be better so there is less of a rug pull

@jakirkham
Copy link
Member Author

It's worth noting the migrator PR title is taken from the first line of the commit message so we can make this change pretty visible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants