Skip to content

fix(ci): longest-prefix-match for external-repo subtree change detection - #7999

Merged
tony-davis merged 1 commit into
mainfrom
users/tony-davis/therock-longest-prefix-match
Sep 8, 2026
Merged

tony-davis merged 1 commit into
mainfrom
users/tony-davis/therock-longest-prefix-match

Conversation

@tony-davis

@tony-davis tony-davis commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

ISSUE ID: ROCm/rocm-libraries#11784

Problem

find_matched_subtrees() in configure_external_repo_ci.py truncates every changed file path to exactly 2 segments (category/name) before matching against registered subtree prefixes. This makes a nested subtree registered in repos-config.json (e.g. projects/hipblaslt/tensilelite, nested inside projects/hipblaslt) indistinguishable from a change to its parent: both collapse to projects/hipblaslt, silently losing the more specific match.

This blocks the companion synthetic-subprojects PR (#7998): even once TensileLite has its own consumer-graph node, change detection needs to be able to tell a TensileLite-only change apart from a hipblaslt-proper change in the first place.

Fix

Switch to longest-prefix-match: check registered prefixes from most to least specific, and attribute each changed file to exactly one (the longest matching) subtree.

def find_matched_subtrees(changed_files, valid_prefixes):
    prefixes_by_specificity = sorted(valid_prefixes, key=lambda p: p.count("/"), reverse=True)
    matched = set()
    for path in changed_files:
        segments = path.split("/")
        for prefix in prefixes_by_specificity:
            prefix_segments = prefix.split("/")
            if segments[: len(prefix_segments)] == prefix_segments:
                matched.add(prefix)
                break
    return sorted(matched)

A file nested in projects/hipblaslt/tensilelite/... now resolves to projects/hipblaslt/tensilelite (when that prefix is registered); a file elsewhere under projects/hipblaslt/... still resolves to projects/hipblaslt as before.

Testing

Added test_nested_subtree_wins_over_parent, test_parent_only_change_does_not_match_nested_subtree, and test_mixed_nested_and_parent_changes_match_both to build_tools/tests/configure_external_repo_ci_test.py, alongside the existing suite.

  • python -m pytest build_tools/tests/configure_external_repo_ci_test.py -- 27 passed
  • pre-commit run --files build_tools/github_actions/configure_external_repo_ci.py build_tools/tests/configure_external_repo_ci_test.py (black)

Tracked in ROCm/rocm-libraries#11784, alongside the companion synthetic-subprojects PR (#7998) and the rocm-libraries repos-config.json PR, which applies the identical fix to its own independently-duplicated copy of this function.

find_matched_subtrees() truncated every changed file path to exactly 2
segments (category/name) before matching against registered subtree
prefixes. This makes a nested subtree registered in repos-config.json
(e.g. projects/hipblaslt/tensilelite, nested inside projects/hipblaslt)
indistinguishable from a change to its parent: both collapse to
projects/hipblaslt, silently losing the more specific match.

Switch to longest-prefix-match: check registered prefixes from most to
least specific, and attribute each changed file to exactly one (the
longest matching) subtree. A change nested in tensilelite now correctly
resolves to projects/hipblaslt/tensilelite instead of being merged into
projects/hipblaslt, while an ordinary hipblaslt-proper change still
resolves to projects/hipblaslt as before.

Adds tests for the nested-vs-parent distinction: a nested-only change, a
parent-only change, and both changed in the same PR.

Co-authored-by: Cursor <cursoragent@cursor.com>
@therock-pr-bot

therock-pr-bot Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot therock-pr-bot Bot added the Not ready to Review PR has unresolved policy failures — reviews blocked label Sep 8, 2026
@therock-pr-bot

therock-pr-bot Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The longest-prefix-match implementation is straightforward, well-scoped, and is backed by targeted tests that cover the newly supported nested-subtree scenarios.

Pull request overview

This PR fixes external-repo CI subtree change detection in configure_external_repo_ci.py by switching from a fixed 2-segment truncation (category/name) to a longest-prefix-match algorithm, allowing nested registered subtrees (e.g. projects/hipblaslt/tensilelite) to be distinguished from parent subtree changes.

Changes:

  • Implement longest-prefix match in find_matched_subtrees() so each changed file maps to the most-specific registered subtree prefix.
  • Add unit tests covering nested-subtree vs parent-subtree matching behavior (nested wins; parent-only changes still match parent; mixed changes match both).
File summaries
File Description
build_tools/github_actions/configure_external_repo_ci.py Replaces fixed 2-segment subtree inference with longest-prefix matching over registered prefixes.
build_tools/tests/configure_external_repo_ci_test.py Adds coverage for nested subtree matching and mixed parent/nested change scenarios.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@newling newling left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@geomin12 geomin12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm thanks!

@tony-davis
tony-davis merged commit df3d451 into main Sep 8, 2026
216 of 226 checks passed
@tony-davis
tony-davis deleted the users/tony-davis/therock-longest-prefix-match branch September 8, 2026 20:33
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Sep 8, 2026
tony-davis added a commit to ROCm/rocm-libraries that referenced this pull request Sep 10, 2026
…-prefix-match (#11785)

## Problem

TensileLite-only changes in this repo don't correctly trigger hipblaslt
retesting in Multi-Arch CI: `projects/hipblaslt/tensilelite` collapses
into `projects/hipblaslt` in change detection (2-segment path
truncation), and TheRock's consumer graph has no node for TensileLite
anyway (it has no CMake target). This PR is the rocm-libraries half of
the fix; see #11784 for the full tracking issue and the two companion
TheRock PRs (ROCm/TheRock#7998, ROCm/TheRock#7999).

## Changes

- **`repos-config.json`**: register `projects/hipblaslt/tensilelite` as
a nested subtree entry, with `auto_subtree_pull`/`auto_subtree_push`
both `false` -- it isn't independently synced (TensileLite lives inside
hipBLASLt's own tree; hipblaslt's existing entry already covers real
subtree sync). This entry exists purely so change detection has a target
to match against.
- **`pr_detect_changed_subtrees.py`**: same longest-prefix-match fix as
the companion TheRock PR, so a file under
`projects/hipblaslt/tensilelite/...` resolves to that nested subtree
instead of collapsing into `projects/hipblaslt`.
- **`therock_matrix.py`**: the new, more specific
`projects/hipblaslt/tensilelite` selector would otherwise silently fall
out of three hardcoded lookups in the legacy single-arch CI path
(`subtree_to_project_map`, `ROCJITSU_RACE_CHECK_SUBTREES`,
`SUBTREE_EXTRA_MATRIX_PROJECTS`), all keyed on the literal
`projects/hipblaslt` string. Added matching entries for
`projects/hipblaslt/tensilelite` to each, so legacy behavior is exactly
preserved (same "blas" bucket, same rocjitsu race-check selection, same
sparselt activation as a hipblaslt-proper change) -- I caught this by
running the full `.github/scripts/tests` suite, not just the
directly-touched test file; two legacy tests broke without this.

## Testing

- `python -m pytest .github/scripts/tests` -- 153 passed, 8 xfailed
(pre-existing, unrelated)
- `pre-commit run` (black) on all touched files
- Validated the new `repos-config.json` entry loads cleanly through the
existing Pydantic `RepoConfig` model

## Notes

`auto_subtree_pull`/`auto_subtree_push` are both `false` for the new
entry, so it's excluded from every actual subtree pull/push automation
(`pr-merge-sync-patches.yml` passes `--require-auto-push`, which filters
it out before matching). Its only purpose is more precise
change-detection.

Tracked in #11784, alongside the two companion TheRock PRs
(ROCm/TheRock#7998, ROCm/TheRock#7999).

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
tony-davis pushed a commit that referenced this pull request Sep 15, 2026
## Summary

Test selection returns **no tests** for a change confined to
TensileLite, because the nested subtree path
`projects/hipblaslt/tensilelite` has no entry in
`_EXTERNAL_SUBTREE_ALIASES`. This adds the alias.

## Motivation

ROCm/rocm-libraries#11785 taught rocm-libraries' change detection to
report `projects/hipblaslt/tensilelite` (longest-prefix match) instead
of collapsing TensileLite files into `projects/hipblaslt`.
`determine_rocm_test_dependencies.py` has no alias for that string, so
it falls through to the `projects/` strip, produces the non-existent
graph key `hipblaslt/tensilelite`, warns, and selects nothing — while
exiting `0`.

Verified against `df3d451` (the commit rocm-libraries pins today):

```console
$ python test_tools/determine_rocm_test_dependencies.py \
    --changed-projects projects/hipblaslt/tensilelite
Warning: unrecognized project(s) ['hipblaslt/tensilelite']; no tests will be selected for them
["hipblaslt/tensilelite"]
$ echo $?
0
```

The consequence is visible in rocm-libraries. TensileLite-only PRs ran a
sanity check and nothing else:

| PR | files | multi-arch test components |
|---|---|---|
| ROCm/rocm-libraries#11518 | `tensilelite/src` + 2 new
`tensilelite/tests/*.cpp` | no `Test tensilelite` |
| ROCm/rocm-libraries#11896 | 7, all under
`projects/hipblaslt/tensilelite/**` | sanity only |
| ROCm/rocm-libraries#11877 | 6, all under
`projects/hipblaslt/tensilelite/**` | sanity only |

ROCm/rocm-libraries#11518 merged that way and was reverted by
ROCm/rocm-libraries#12022 after failing `Test tensilelite` on gfx94X and
gfx950 in this repo's CI — after merge, blocking the submodule bump in
#8183.

## Technical Details

One alias entry. `tensilelite` is already a synthetic node at level 3
(`test_policies.toml`), so selection also reaches `hipblaslt`, `rocblas`
and `hipblas` transitively — the same set `shared/stinkytofu` already
gets.

After the change, on the real committed graph:

```console
$ python test_tools/determine_rocm_test_dependencies.py \
    --changed-projects projects/hipblaslt/tensilelite
["hipblas", "hipblaslt", "hipblasltprovider", "hipdnn-samples", "hipsolver",
 "hipsparse", "hipsparselt", "miopen", "miopenprovider", "rocalution",
 "rocblas", "rocsolver", "rocsparse", "rocwmma", "tensilelite"]
```

## Risk Assessment

Risk 2. Test-selection only; no product or device behaviour changes. It
strictly widens selection for one input that currently selects nothing,
so the failure mode is longer CI on TensileLite PRs, not reduced
coverage.

## Related

- Work tracking: ROCm/rocm-libraries#11784
- Introduced by: ROCm/rocm-libraries#11785 (rocm-libraries half of the
nested-subtree fix)
- Companion PRs: #7998 (TensileLite as a synthetic node), #7999
(longest-prefix match)
- Fallout: ROCm/rocm-libraries#11518, reverted by
ROCm/rocm-libraries#12022, blocking #8183

## Testing Summary

- Unit: new regression test in
`determine_rocm_test_dependencies_test.py`, plus the existing suite.
- Manual: CLI run against the real committed graph, before and after.

## Testing Checklist

- [x] New regression test — `python -m unittest
test_tools.tests.determine_rocm_test_dependencies_test -k
nested_tensilelite` — Status: Passed
- [x] Verified it fails without the fix (stashed the alias, test fails)
— Status: Passed
- [x] Full selector suite — `python -m unittest
test_tools.tests.determine_rocm_test_dependencies_test` — 70 tests —
Status: Passed
- [ ] PR CI — Status: Pending

The new test declares `[synthetic.tensilelite]` in its fixture,
mirroring the real `test_policies.toml`, so it exercises the walk past
the alias rather than stubbing it. Clean `stderr` is asserted
deliberately: the regression was a warning plus an empty selection, not
a non-zero exit.

## Device / Architecture Coverage

None. Change-detection logic only; no compiled code and no device
behaviour. PR CI is sufficient.

## Adjacent Tests Considered

`shared/stinkytofu`, `shared/origami`, `shared/mxdatagenerator` and
`shared/tensile` all resolve through the same alias table and are
covered by `test_shared_blas_prefixes_mapped`; all still pass.

## Follow-ups not in this PR

1. **`projects/hipblaslt` still does not select `tensilelite`.** A
non-TensileLite hipBLASLt change selects `[hipblas, hipblaslt,
hipblasltprovider, hipsparselt, miopen, rocblas]`. rocm-libraries'
`therock_matrix.py` maps `projects/hipblaslt` → `blas` → `[hipblaslt,
rocblas, hipblas, tensilelite]`. Closing that gap means adding
`tensilelite` as a consumer of `hipblaslt` in the consumer graph, which
is a larger change than an alias and wants its own review.
2. **Unmapped `projects/` paths fail open.** `_EXTERNAL_ONLY_NAMESPACES`
makes an unmapped `shared/` path raise, but an unmapped `projects/` path
warns and exits `0`, yielding a green run with no tests. Extending the
fail-closed behaviour would have surfaced this on the day
ROCm/rocm-libraries#11785 merged rather than via a revert a week later.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants