revert(hipdnn): remove Python bindings from TheRock#6425
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
| therock_subproject_fetch(therock-nanobind-sources | ||
| CMAKE_PROJECT | ||
| # Originally mirrored from: https://github.com/wjakob/nanobind/archive/refs/tags/v2.12.0.tar.gz | ||
| URL https://rocm-third-party-deps.s3.us-east-2.amazonaws.com/nanobind-2.12.0.tar.gz |
There was a problem hiding this comment.
I'll ask to remove these artifacts after this PR is in
There was a problem hiding this comment.
Did we double check no one else is using these since they were added?
There was a problem hiding this comment.
Double checked in TheRock and doesn't seem like anyone is using this
There was a problem hiding this comment.
Right now it is unused but even if we move the hipDNN bindings to a separate repo (and eventually we have more than one?), do you intend to build nanobind there or should TheRock provide these deps?
There was a problem hiding this comment.
I think it makes sense to keep the artifacts uploaded, and use those until it's clear where the new final home is.
There was a problem hiding this comment.
I believe TheRock shouldn't provide the dependencies; since in the future it will be rocm-libraries ->TheRock->rocm-bindings. If it did, it would cause a cycle dependencies.
So mostly likely rocm-bindings will build nanobind.
There was a problem hiding this comment.
I will keep the artifacts around in s3 then
BrianHarrisonAMD
left a comment
There was a problem hiding this comment.
Looks good, but we need to double check no one else started depending on the 3rd party deps, or any of the common scripts we added.
If they are confirmed fully dead code, then we should be good to remove.
geomin12
left a comment
There was a problem hiding this comment.
i see build failures. don't seem unrelated, so worth a retry
i also agree Brian, we should make sure this builds all properly and double check that no components use this. a search should solve that though
| "-DHIP_DNN_SKIP_TESTS=$<NOT:$<BOOL:${THEROCK_BUILD_TESTING}>>" | ||
| -DHIP_DNN_BUILD_PLUGINS=OFF | ||
| -DHIP_DNN_GENERATE_SDK_HEADERS=OFF | ||
| -DHIPDNN_BUILD_PYTHON_BINDINGS=ON |
There was a problem hiding this comment.
Should this be rather set explicitly to OFF?
There was a problem hiding this comment.
I'm actually removing this option in rocm-libraries. It is defaulted off so it's fine to remove here.
| therock_subproject_fetch(therock-nanobind-sources | ||
| CMAKE_PROJECT | ||
| # Originally mirrored from: https://github.com/wjakob/nanobind/archive/refs/tags/v2.12.0.tar.gz | ||
| URL https://rocm-third-party-deps.s3.us-east-2.amazonaws.com/nanobind-2.12.0.tar.gz |
There was a problem hiding this comment.
Right now it is unused but even if we move the hipDNN bindings to a separate repo (and eventually we have more than one?), do you intend to build nanobind there or should TheRock provide these deps?
Summary
We thought hipDNN bindings were going to land in TheRock artifacts, but the new artitecture is that we are going to have a different repo for all bindings.
So, this PR reverts TheRock-side hipDNN Python binding build, packaging, and wheel-validation CI wiring added by PR #5429, so binding-specific CI can move out of TheRock per ALMIOPEN-2285. JIRA ID : ALMIOPEN-2285
Risk Assessment
Medium risk. The user risk hint was low, but this changes hipDNN build, package, and test infrastructure by removing TheRock Python binding paths; local test-matrix unit coverage and formatting checks passed, with PR CI still needed for build and artifact validation.
ASIC Coverage
Standard PR CI is sufficient. This is ASIC-independent build, CI, and artifact packaging cleanup that removes hipDNN Python binding wiring from TheRock; it does not change kernels, provider support surface, dispatch behavior, or GPU defaults, so no dedicated multi-arch sweep is required.
Testing Summary
nanobind,robin-map,therock-nanobind, ortherock-robin-mapreferences in touched TheRock build/test areas.Testing Checklist
python -m unittest build_tools.github_actions.tests.fetch_test_configurations_test- Status: Passedpython -m black --check build_tools/github_actions/tests/fetch_test_configurations_test.py- Status: Passednanobind,robin-map,therock-nanobind, andtherock-robin-mapreferences - Status: PassedTechnical Changes
share/hipdnn/python/**artifact include/exclude rules and the shared loader-environment helper used only by the deleted Python binding test.third-party/nanobindandthird-party/robin-mapCMake subprojects from the third-party build list.