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

Remove version constraint of nvcc to match with host cuda version #87

Merged
merged 7 commits into from
Jul 22, 2022

Conversation

galipremsagar
Copy link
Contributor

This PR removes the version constraint that nvcc imposes to have the matching CUDA version on the host machine installed. Upon some searching, relaxing & testing this fix it appears that there is no version-specific file being shipped along with nvcc conda package, but I could be wrong here too. Hence requesting review from maintainers since this check has been present since its inception.

Rationale behind this PR is to allow users to utilized any version of nvcc package without upgrading their local host CUDA installations.

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.

@conda-forge-linter
Copy link

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.

@leofang
Copy link
Member

leofang commented Jun 29, 2022

@leofang
Copy link
Member

leofang commented Jun 29, 2022

We still need this check for building the nvcc packages. It can be a warning in user envs. I failed to recognize this is part of the activation script (it's super confusing we don't make activation/deactivation scripts standalone, see #68). @galipremsagar will make the change and I will continue #68 after this PR is merged.

@galipremsagar
Copy link
Contributor Author

Synced offline with @leofang and agreed on erroring only during conda build time and just raising a warning for end-user during conda activation.

recipe/install_nvcc.sh Outdated Show resolved Hide resolved
recipe/install_nvcc.sh Outdated Show resolved Hide resolved
recipe/install_nvcc.sh Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
recipe/install_nvcc.sh Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
@jakirkham
Copy link
Member

Would also bump the build number here

{% set number = 19 %}

That should generate new packages with this change

@galipremsagar
Copy link
Contributor Author

Would also bump the build number here

{% set number = 19 %}

That should generate new packages with this change

Done 👍

@jakirkham
Copy link
Member

Thanks Prem! 🙏 LGTM ✅

Will give others a chance to look 🙂

@jakirkham jakirkham requested a review from leofang July 22, 2022 18:50
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @galipremsagar @jakirkham. LGTM.

@@ -65,8 +65,13 @@ fi

if [[ -z "\$(\${CUDA_HOME}/bin/nvcc --version | grep "Cuda compilation tools, release ${PKG_VERSION}")" ]]
then
echo "Version of installed CUDA didn't match package"
return 1
if [[ "\${CONDA_BUILD}" = "1" ]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: the extra backslash is needed because this is part of codegen, not the actual script.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be eliminated in #91.

@leofang leofang added the automerge Merge the PR when CI passes label Jul 22, 2022
@github-actions github-actions bot merged commit 49f387e into conda-forge:main Jul 22, 2022
@github-actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@leofang
Copy link
Member

leofang commented Jul 26, 2022

Should we port this change to the Windows script?

@jakirkham
Copy link
Member

Seems reasonable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants