-
-
Notifications
You must be signed in to change notification settings - Fork 64
Add support for multi-arch and multi-platform cuda toolchains #422
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?
Add support for multi-arch and multi-platform cuda toolchains #422
Conversation
…-arch' Enable multi-version builds See merge request csaint/rules_cuda!1
steple
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.
Thanks for adding this, I think this will be very useful.
|
pushed a new commit with the versions stuff moved around and all in one place at least and I changed the name of the nvcc_platform and runtime_platform to exec and target |
| "linux_x86_64_repo": attr.string( | ||
| mandatory = True, | ||
| doc = "Name of the repository to use for x86_64 platform", | ||
| ), | ||
| "linux_aarch64_repo": attr.string( | ||
| mandatory = True, | ||
| doc = "Name of the repository to use for ARM64/Jetpack platform", | ||
| ), | ||
| "linux_sbsa_repo": attr.string( | ||
| mandatory = True, | ||
| doc = "Name of the repository to use for SBSA platform", | ||
| ), |
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.
would a platform_mapping with attr.string_dict be better here? Validating the key in the rule impl seem to be future proof.
| for _, toolkit in registrations.items(): | ||
| if components_mapping != None: | ||
| cuda_toolkit(name = toolkit.name, components_mapping = components_mapping, version = redist_version) | ||
| # Always use the maximum version so the toolkit includes all components. |
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 is not quite true if CTK delete some component in the future. I think a union across all CTK versions will be a little bit more robust.
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 could take some work, right now the version in cuda_toolkit isn't going to necessarily be correct since it's pointing to @cuda which can point to any number of versioned cuda repos, but I don't know if that gets used anywhere in the rules so I'll try removing it and see what falls out...
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 logic is pretty deeply embedded in the repository rules where I can't use the value of a flag. I might need to go back and add the ability to register multiple toolkits to get everything to work as expected...
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.
Lets leave it for future improvement, just point it out :)
This MR adds support for multiple architectures and multiple versions of a toolchain. Using the redis_json you can create multiple versions of a toolchain:
then using a set of flags in .bazelrc you can control which version of the toolchain to use in a given config: