Repository navigation
ci(tensilelite): prevent new legacy Tensile imports (AIHPBLAS-3989) - #13278
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #13278 +/- ##
===========================================
- Coverage 74.64% 74.63% -0.01%
===========================================
Files 2881 2881
Lines 480340 480423 +83
Branches 71286 71290 +4
===========================================
+ Hits 358536 358562 +26
- Misses 97494 97550 +56
- Partials 24310 24311 +1
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
5a6efa4 to
c5c5738
Compare
newling
left a comment
There was a problem hiding this comment.
Just to understand, what's the alternative here, that we don't add these tests and then at some point in the future we just make import Tensile stop working and patch any remaining Tensile->tensilelite fixes that are needed, in a single PR, i.e. make Tensile illegal and fix remaining cases in 1 PR. What is the problem with that, is it just that we can 2 PRs that pass CI that are mutually imcompatible?
The problem with that approach is practicality. There are still a lot of imports that need to be changed and getting all of those through CI will take time, during which new (Tensile-style) imports will be added, which either require a new PRs to clean up, or continuously patching and rebasing the final PR that removes that support for |
newling
left a comment
There was a problem hiding this comment.
LGTM, argument for adding these (temporarily) makes sense to me
c5c5738 to
8b3e04e
Compare
8b3e04e to
9123cd7
Compare
|
miopen failures on Windows gfx110x-all is a timeout |
TheRock Submodule Bump ActivityNewest first
|
…3989) (#13932) JIRA ID : AIHPBLAS-3989 ## Motivation #12162 removes the `Tensile` compatibility package, so a new import of the old name now fails at import time. The absolute-import pre-commit hook added in #13278 still carries the `Tensile` package root, a bridge exemption for `Tensile/__init__.py`, and `Tensile` as a banned name. All of that is dead after #12162. The rule that remains useful is that `tensilelite` modules use package-relative imports. ## Technical Details - `check_no_absolute_imports.py` now checks only `projects/hipblaslt/tensilelite/tensilelite`. The `Tensile` package root, the `TEMPORARY_EXEMPTIONS` bridge exception and the `Tensile` banned name are removed, and the docstring and failure message are updated. - The `tensilelite-no-absolute-imports` pre-commit entry matches only the `tensilelite` package and its `Tests/` exclusion, and its stale "move in flight" comment is removed. - The checker's unit tests use `tensilelite` as the banned example. The bridge-exemption and missing-package-root tests are removed. An added test records that `Tensile` imports are no longer reported by this hook. - Behavior is unchanged for `tensilelite`: only lines added by a change are checked, and `Tests/` is excluded. ## Test Plan Run the checker's unit tests, which exercise staged and PR-range changes, the package root, test-directory exclusion, existing imports and unparseable files. Run the repository's pre-commit checks in PR CI. GPU testing is unnecessary because this change only examines Python source. ## Test Result - Checker unit tests: 29 passed (run with `--noconftest`; at this point in the stack the package conftest still fails on stale `Tensile` imports inherited from `develop`). - Pre-commit hook run against the full tree and PR CI: not run locally. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. ## Risk level Low (1/5): only the pre-commit hook and its tests change. It does not change packaged code or GPU behavior.
JIRA ID : AIHPBLAS-3989
Motivation
#12150 moves the Python implementation from
Tensiletotensilelitewhile keepingTensileimportable through a temporary compatibility package. New imports using the old name would add references that must be removed before that package can go away.Technical Details
Tensileortensilelite, so internal modules use package-relative imports.Tests/.Tensile/__init__.pyto install the compatibility alias. This exception will be removed in #12162 when that PR removes the bridge.This PR depends on #12150.
Test Plan
Run the checker's Python unit tests, which exercise staged and PR-range changes, both package roots, the bridge exception, and existing imports. Run the repository's pre-commit checks in PR CI. GPU testing is unnecessary because this change only examines Python source.
Test Result
git diff --check: passed.Submission Checklist
Risk level
Medium (3/5): the new check can reject TensileLite Python changes in pre-commit CI. It does not change packaged code or GPU behavior.