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.

Sign up for GitHub

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

Jump to bottom

Add aarch64 #70

Merged
merged 5 commits into from
Sep 29, 2021
Merged

Add aarch64 #70

merged 5 commits into from
Sep 29, 2021

Conversation

leofang
Copy link
Member

@leofang leofang commented Sep 27, 2021 •
edited
Loading

Follow-up of conda-forge/cudatoolkit-feedstock#59 and conda-forge/docker-images#189.

Also add myself as a maintainer in this PR 🙂

Note: conda-smithy seems to have a bug in generating README contents for compilers. For example, the README for this feedstock is already incorrect --- neither ppc64le nor win64 is listed. With this PR it gets worse: osx-arm64 is listed (which we don't build for).

I don't think it hurts, though. We can fix conda-smithy separately and then rerender this feedstock.

cc: @jakirkham

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 Author

leofang commented Sep 27, 2021 •
edited
Loading

@conda-forge/core The linux-anvil-aarch64-cuda docker images seem to have permission issues. They were uploaded after conda-forge/docker-images#189 was merged, but they are not accessible by either the CI or human users.

@leofang
Copy link
Member Author

leofang commented Sep 27, 2021

截圖 2021-09-27 00 16 59

kkraus14
kkraus14 approved these changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@kkraus14 kkraus14 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @leofang

@leofang
Copy link
Member Author

leofang commented Sep 27, 2021

@conda-forge-admin, please restart ci

@leofang
Copy link
Member Author

leofang commented Sep 27, 2021

The linux-anvil-aarch64-cuda docker images seem to have permission issues. They were uploaded after conda-forge/docker-images#189 was merged, but they are not accessible by either the CI or human users.

@kkraus14, did you or someone fix the permission issue? Looks like it's working now --- all tests passed!

@isuruf
Copy link
Member

isuruf commented Sep 27, 2021

Yes, I have to fix it manually every time there's a new image name.

@leofang
Copy link
Member Author

leofang commented Sep 27, 2021

Ah, many thanks @isuruf! 👍

@leofang leofang mentioned this pull request Sep 27, 2021
Merged
4 tasks
jakirkham
jakirkham reviewed Sep 27, 2021
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Leo! 😄

Had one suggestion below

recipe/meta.yaml Outdated
{% if cuda_major == 0 or (target_platform == "linux-ppc64le" and cuda_major_minor != (10, 2)) %}
{% if cuda_major == 0
or (target_platform == "linux-ppc64le" and cuda_major_minor != (10, 2))
or (target_platform == "linux-aarch64" and (cuda_major != 11 or arm_variant_type != "sbsa")) %}
Copy link
Member

Choose a reason for hiding this comment

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

Could we please write this in terms of (10, 2) as well? Want to make sure this still works smoothly as future versions come out :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't thinking before changing... @jakirkham What do you expect? cuda_major_minor < (11, 0)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that seems reasonable. If we make that change, can we please do the same thing for ppc64le?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jakirkham I guess I misunderstood what you meant. For ppc64le let's keep it as is until we add CUDA 11 support for it, is that ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah no worries. That's reasonable. This is admittedly a bit unrelated. Just was thinking on how we might tidy things while we were here

@leofang
Copy link
Member Author

leofang commented Sep 27, 2021

@conda-forge-admin, please rerender

conda-forge-linter and others added 2 commits September 27, 2021 18:38
@leofang
Copy link
Member Author

leofang commented Sep 27, 2021

@conda-forge-admin, please rerender

jakirkham
jakirkham approved these changes Sep 29, 2021
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Leo! 😄

@jakirkham jakirkham merged commit 408c931 into conda-forge:master Sep 29, 2021
@jakirkham jakirkham mentioned this pull request Sep 29, 2021
Closed
@leofang leofang deleted the aarch64 branch September 29, 2021 12:21
@leofang
Copy link
Member Author

leofang commented Sep 29, 2021

Thanks @kkraus14 @isuruf @Ethyling @jakirkham!

@jakirkham jakirkham mentioned this pull request Sep 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjacobelli jjacobelli jjacobelli approved these changes

@kkraus14 kkraus14 kkraus14 approved these changes

@jakirkham jakirkham jakirkham approved these changes

@dillon-cullinan dillon-cullinan Awaiting requested review from dillon-cullinan

@isuruf isuruf Awaiting requested review from isuruf

@jaimergp jaimergp Awaiting requested review from jaimergp

@mike-wendt mike-wendt Awaiting requested review from mike-wendt

@raydouglass raydouglass Awaiting requested review from raydouglass

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@leofang @conda-forge-linter @isuruf @jakirkham @kkraus14 @jjacobelli