-
-
Notifications
You must be signed in to change notification settings - Fork 567
NEW: Add migrator for tegra arm variant #7956
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
base: main
Are you sure you want to change the base?
Conversation
|
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 ( |
fe5b64e to
c222abc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the issue of zip_keys becoming larger as in the other issue, this doesn't work when a feedstock has a cpu build.
|
The migrator is now compatible with feedstocks that include a non-CUDA build variant. The migrator now requires that both the CUDA 12.9 and CUDA 13.0 migrators have been added already. |
| arm_variant_type: # [aarch64] | ||
| - sbsa # [aarch64] | ||
| - None # [aarch64] | ||
| - sbsa # [aarch64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakirkham, I have changed the default arm_variant_type to None because non-CUDA builds don't care about arm_variant_type.
|
@conda-forge-admin, please restart ci |
| https://github.com/conda-forge/cuda-feedstock/blob/main/recipe/doc/recipe_guide.md#building-for-arm-tegra-devices | ||
| use_local: false | ||
| paused: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This migrator is paused so that it is opt-in. Community feedback from robotics people is that there are probably many CUDA 12.9 packages that people would never run on a Tegra device, so it's not worth migrating the entire channel.
|
@conda-forge/core, please review. |
h-vetinari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After making the suggested changes, could you please demonstrate a rerender on a branch (or in a test PR) of that migrator (together with the other changes to migrators and pinnings here, which will require use_local: true for cuda129/130 and -e to override the pinning file)? Say, on the libmagma feedstock.
| check_solvable: false | ||
| wait_for_migrators: | ||
| - cuda129 | ||
| - aarch64 and ppc64le addition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this works? I don't think it makes a lot of sense to wait for that migrator in practice (even though it's required conceptually) - the aarch/ppc migration is complete up to very few packages, and if someone wants to fix up aarch support for a feedstock that doesn't have it yet because they want tegra support, all the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this works?
It's unclear by what you mean. This is the correct name of the aarch64 migrator.
https://github.com/search?q=org%3Aconda-forge+%22aarch64+and+ppc64le+addition%22&type=code
https://github.com/search?q=repo%3Aregro%2Fcf-scripts+++%22aarch64+and+ppc64le+addition%22&type=code
I don't think it makes a lot of sense to wait for that migrator in practice (even though it's required conceptually) - the aarch/ppc migration is complete up to very few packages, and if someone wants to fix up aarch support for a feedstock that doesn't have it yet because they want tegra support, all the better.
In practice, I don't expect anyone to unpause this tegra migrator; all migrations will be manual. But if we do, then it is required that all the dependencies of a given package have been migrated to include aarch64 before tegra can be enabled. It doesn't prevent the use of this migrator to include this item in "wait_for_migrators", and it's technically correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear by what you mean. This is the correct name of the aarch64 migrator.
Scarred by spaces messing up "simple" string handling.
all migrations will be manual
That's a good point; it means the wait_for_migrators: is completely pointless (which is aimed at our bots, rather than at humans), but in that sense, it also doesn't hurt to have it in there, because it'll never be applied. 🤷
| c_compiler_version: # [linux and aarch64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] | ||
| - 14 # [linux and aarch64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] | ||
| - 14 # [linux and aarch64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] | ||
| - 14 # [linux and aarch64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] | ||
| - 14 # [linux and aarch64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to turn this into a operation: key_add migrator, rather than redefining existing pins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried for multiple days to create a correct migrator using operation: key_add instead of kind: version. The reoccurring problems were that a tegra variant for CUDA 13.0 would also be created and/or that the glibc version for variants would be incorrect or there would only be one CUDA 12.9 variant instead of two.
I need specific advice on how to accomplish this task or else I will not try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrators are applied in the order of their migrator_ts. In cases where the arithmetic depends on a specific sequence of application (e.g. because one migrator adds an additional_zip_keys: that all following migrators now need to populate), then you need to reflect that order in the timestamps (even if the dates don't correspond to reality). Don't change the timestamps of existing migrators though, only the one you're adding.
It's good to apply migration by migration on a feedstock in question (in the sense of testing a rerender with only the first one, then the second, then the third added to .ci_support/migrations) and see when/where things go wrong.
or there would only be one CUDA 12.9 variant instead of two.
What's the desired target state? Both CUDA 12.9 and 13.0 should get a tegra variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following variants should exist for aarch64
| cuda_compiler_version | tegra |
|---|---|
| None | None |
| 12.9 | sbsa |
| 12.9 | tegra |
| 13.0 | sbsa |
There is no tegra variant for CUDA >=13
I have updated my test branch with the latest changes. conda-forge/cusparselt-feedstock#72 |
This migrator adds the
tegravariant to the existingarm_variant_typekey in the global pinnings. Thetegravariant is only forcuda_compiler_version 12.9and has a different (higher)c_stdlib_versionthan the CUDA 12.9 migrator.Modifications to the other CUDA migrations and the global pinnings were needed. These modifications including adding
arm_variant_typeto the global zip with thecuda_compiler_versionwhen the platform is linux and the architecture is aarch64 and setting the defaultarm_variant_typetoNone.The default
arm_variant_typeshould beNoneand notsbsabecause non-CUDA builds should be installable on bothsbsaandtegradevices.Testing this migrator in this feedstock: conda-forge/cusparselt-feedstock#72