Repository navigation
refactor(rocm): make the model registry adapter note helper private again - #930
Closed
jussielo-amd wants to merge 2 commits into
Closed
jussielo-amd wants to merge 2 commits into
jussielo-amd wants to merge 2 commits into
Conversation
…gain model_registry_adapter_availability_note was widened to pub(crate) when the rocm model catalog renderer moved out of examine_report_text.rs, only because one test that stayed behind called it. That test, linux_only_engine_runtime_status_is_explicit_on_native_windows, checked two things: the runtime: label from engine_runtime_status_label and the note the verbose model listing prints. Split it along that line. The label assertion keeps its name and stays in examine_report_text.rs. The note assertion moves, unchanged and with the same cfg!(windows) branches, to model_catalog_text.rs as vllm_adapter_availability_note_is_explicit_on_native_windows, next to the renderer that prints the note. The helper goes back to private, and the module docs and docs/architecture.md describe the new test location. Tests and visibility only; no behaviour change. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the module-boundary and test-organization refactor.
No open issues 🎉
What changed in this PR
Tightens the model catalog module boundary by making model_registry_adapter_availability_note private again. This is a test-organization and visibility refactor with no user-visible behavior changes.
Changes:
- Moves adapter-note assertions into the catalog module’s tests, preserving the existing platform-specific checks.
- Keeps runtime-label assertions in the examine module and removes its cross-module test import.
- Updates module and architecture documentation to reflect the new boundary.
| File | Description |
|---|---|
docs/architecture.md |
Documents the private helper and revised test ownership. |
apps/rocm/src/model_catalog_text.rs |
Makes the helper private and adds the relocated adapter-note test. |
apps/rocm/src/examine_report_text.rs |
Removes adapter-note assertions and import while retaining runtime-label coverage. |
Review effort: Balanced
…ry-note-helper-private Signed-off-by: Jussi Elo <jussi.elo@amd.com> # Conflicts: # docs/architecture.md
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Delivers ROCMAI-885. When the
rocm modelcatalog renderer moved out ofexamine_report_text.rsintomodel_catalog_text.rs, the helpermodel_registry_adapter_availability_notewas widened from private topub(crate)for a single reason: one test that stayed behind inexamine_report_text.rs,linux_only_engine_runtime_status_is_explicit_on_native_windows, also called it. A crate-wide visibility that exists only for a test is a small leak in the module boundary, and it was recorded as such in the module docs and indocs/architecture.md. That test was really two checks in one body: theruntime:label thatengine_runtime_status_labelprints (which belongs toexamine_report_text.rs) and the adapter-availability note the verboserocm modellisting prints (which belongs to the catalog renderer). This PR splits it along that line and puts the helper back to private.What moved
model_registry_adapter_availability_noteassertions oflinux_only_engine_runtime_status_is_explicit_on_native_windowsexamine_report_text.rstestsmodel_catalog_text.rstests, asvllm_adapter_availability_note_is_explicit_on_native_windowsmodel_registry_adapter_availability_notevisibilitypub(crate)The
engine_runtime_status_labelassertions keep the original test name and stay inexamine_report_text.rs. The moved assertions are byte-for-byte the same, inside the sameif cfg!(windows) { .. } else { .. }split, so the Windows branch asserts the note containsunsupported_native_windowsand every other host asserts there is no note. Thecfg!is a runtime branch rather than an attribute, so the test compiles on every host and each host runs the branch that applies to it, exactly as before. Module docs in both files and the twodocs/architecture.mdentries (examine_report_text.rsandmodel_catalog_text.rs) now describe the new test location and the private helper; the sentence sayingexamine_report_text.rscalls into the catalog module "outside its own tests" is now simply "does not call into it".The ticket also asked to check the
docs/architecture.mdclaim that the catalog module uses exactly two items frommodel_readiness. That claim is accurate:model_catalog_text.rsreferencesrocm_core::model_readiness::only forcurated_alternatives(called directly) andrecipe_display_ref(through a private one-line wrapper of the same name), andmodel_readinessis not re-exported at therocm_coreroot, so nothing else arrives implicitly. That entry is unchanged.Verification
Ran:
cargo fmt --all --check;cargo clippy -p rocm --all-targets -- -D warnings;cargo test -p rocm --bin rocm --no-fail-fast(1510 passed, 8 ignored, 0 failed);cargo xtask manifest --check;cargo xtask check-crate-edges;cargo xtask lychee; the repository commit hooks (including hawkeye and lychee); a conflict-marker grep; and the leak scan over the diff againstorigin/main(the only hits are the word "private" in the doc sentence that says an item is private). The test listing (--list) before and after differs by exactly one line, the new namemodel_catalog_text::tests::vllm_adapter_availability_note_is_explicit_on_native_windows.The moved test was shown to fail when its target is broken. On Linux, changing the
PlatformRuleOut::Allowedarm of the helper to return a note turns the non-Windows branch red (assertion failed: model_registry_adapter_availability_note("vllm", &|| true).is_none()). The Windows branch cannot run on a Linux host, so it was exercised by temporarily forcing the condition to true: with the helper returning a note withoutunsupported_native_windowsthecontainsassertion fails, and with a note that contains it the test passes. Both temporary edits were reverted by reverse edits and the final diff contains neither. On a real Windows host the helper takes theNativeWindowsVllmarm, whose text is untouched here; that branch runs on the Windows CI job.Not run: the cross-target
cargo clippy --target x86_64-pc-windows-msvccheck failed on this machine while building the C dependencyaws-lc-sys(no usable C cross-compiler in the sandbox), before reachingrocm. The change contains nocfg(windows)attribute, only the runtimecfg!(windows)branch that already existed, so it compiles identically on every target. The full workspace gate,cargo xtask e2eandscripts/smoke_local.pywere not run; CI carries them.User-visible behaviour
None, so no Gherkin scenario applies. This is a test move and a visibility change on a private helper; no command output, exit code, file or engine selection changes, and the strings the helper returns are untouched.
Notes for reviewers
One commit. Reading order: the new test at the end of
model_catalog_text.rs, the trimmed test inexamine_report_text.rs(theuseof the helper is gone), then the visibility change and the doc wording. Coverage should not drop: no assertion was deleted, one test became two.