Skip to content

Add cuda_compiler to zip_keys#4407

Merged
isuruf merged 2 commits into
conda-forge:mainfrom
jakirkham-feedstocks:zip_cuda_compiler
May 2, 2023
Merged

Add cuda_compiler to zip_keys#4407
isuruf merged 2 commits into
conda-forge:mainfrom
jakirkham-feedstocks:zip_cuda_compiler

Conversation

@jakirkham

@jakirkham jakirkham commented May 2, 2023

Copy link
Copy Markdown
Member

Adds cuda_compiler to zip_keys alongside cuda_compiler_version. This is needed as part of the move to using cuda-nvcc_{{ target_platform }} in the future (as opposed to nvcc_{{ target_platform }}). Though for now this still uses nvcc_{{ target_platform }} in all cases (merely providing a place to add cuda-nvcc_{{ target_platform }} later).

xref: #4400


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.

@jakirkham jakirkham requested a review from a team as a code owner May 2, 2023 18:25
@conda-forge-webservices

Copy link
Copy Markdown
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.

@jakirkham jakirkham mentioned this pull request May 2, 2023
5 tasks
@jakirkham

Copy link
Copy Markdown
Member Author

cc @bdice @leofang @adibbley @vyasr

@jakirkham

Copy link
Copy Markdown
Member Author

cc @conda-forge/core

Comment thread recipe/conda_build_config.yaml Outdated

@isuruf isuruf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You need to fix the cuda migrators too

@jakirkham jakirkham force-pushed the zip_cuda_compiler branch from 048b189 to 19a2e87 Compare May 2, 2023 19:58
@jakirkham

Copy link
Copy Markdown
Member Author

Good point. Those should fixed now. Please take another look and let me know if anything else is needed

@jakirkham jakirkham requested a review from isuruf May 2, 2023 19:59
Comment thread recipe/conda_build_config.yaml Outdated
Comment on lines +44 to +45
cuda_compiler: # [linux64 or win]
- nvcc # [linux64 or win]

@isuruf isuruf May 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
cuda_compiler: # [linux64 or win]
- nvcc # [linux64 or win]
cuda_compiler:
- nvcc # [linux or win]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was changed in response to this suggestion above ( #4407 (comment) )

Could you please share a bit more about why we want to revert that?

Should add the CUDA arch migrator now contains this key with appropriate length

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are supposed to only change that for Line 46 and below.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The selectors used should match that of cuda_compiler_version

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed. Had another question about that below

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds like we were ok with the suggestion below. So committed that

That said, if I'm still missing something here, please let me know

Comment thread recipe/conda_build_config.yaml Outdated
@jakirkham jakirkham requested a review from isuruf May 2, 2023 20:16
@isuruf

isuruf commented May 2, 2023

Copy link
Copy Markdown
Member

Can you check that this works in an existing feedstock locally by passing -e <path/to/conda-forge-pinning-feedstock/recipe/conda_build_config.yaml to conda-smithy rerender?

@jakirkham

Copy link
Copy Markdown
Member Author

Yep, submitted PR ( conda-forge/ucx-split-feedstock#121 ) with the result of that change

Should add the CUDA arch migrator changes were needed as well

@isuruf isuruf merged commit 9479a9a into conda-forge:main May 2, 2023
@jakirkham jakirkham deleted the zip_cuda_compiler branch May 2, 2023 20:48
@jakirkham

Copy link
Copy Markdown
Member Author

Thanks Isuru! 🙏

@jakirkham

Copy link
Copy Markdown
Member Author

Tested also in PR ( conda-forge/arrow-cpp-feedstock#1038 ) to see if cuda_compiler being None for CPU caused any issues, but it seems that CI job is working. So this looks good

@jakirkham

Copy link
Copy Markdown
Member Author

If anything comes up, please feel free to ping me and I will take a look 🙂

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