Skip to content

refactor(rocm): make the model registry adapter note helper private again - #930

Closed
jussielo-amd wants to merge 2 commits into
mainfrom
refactor-model-registry-note-helper-private
Closed

jussielo-amd wants to merge 2 commits into
mainfrom
refactor-model-registry-note-helper-private

Conversation

@jussielo-amd

Copy link
Copy Markdown
Collaborator

Summary

Delivers ROCMAI-885. When the rocm model catalog renderer moved out of examine_report_text.rs into model_catalog_text.rs, the helper model_registry_adapter_availability_note was widened from private to pub(crate) for a single reason: one test that stayed behind in examine_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 in docs/architecture.md. That test was really two checks in one body: the runtime: label that engine_runtime_status_label prints (which belongs to examine_report_text.rs) and the adapter-availability note the verbose rocm model listing prints (which belongs to the catalog renderer). This PR splits it along that line and puts the helper back to private.

What moved

Item From To
The model_registry_adapter_availability_note assertions of linux_only_engine_runtime_status_is_explicit_on_native_windows examine_report_text.rs tests model_catalog_text.rs tests, as vllm_adapter_availability_note_is_explicit_on_native_windows
model_registry_adapter_availability_note visibility pub(crate) private

The engine_runtime_status_label assertions keep the original test name and stay in examine_report_text.rs. The moved assertions are byte-for-byte the same, inside the same if cfg!(windows) { .. } else { .. } split, so the Windows branch asserts the note contains unsupported_native_windows and every other host asserts there is no note. The cfg! 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 two docs/architecture.md entries (examine_report_text.rs and model_catalog_text.rs) now describe the new test location and the private helper; the sentence saying examine_report_text.rs calls 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.md claim that the catalog module uses exactly two items from model_readiness. That claim is accurate: model_catalog_text.rs references rocm_core::model_readiness:: only for curated_alternatives (called directly) and recipe_display_ref (through a private one-line wrapper of the same name), and model_readiness is not re-exported at the rocm_core root, 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 against origin/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 name model_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::Allowed arm 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 without unsupported_native_windows the contains assertion 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 the NativeWindowsVllm arm, 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-msvc check failed on this machine while building the C dependency aws-lc-sys (no usable C cross-compiler in the sandbox), before reaching rocm. The change contains no cfg(windows) attribute, only the runtime cfg!(windows) branch that already existed, so it compiles identically on every target. The full workspace gate, cargo xtask e2e and scripts/smoke_local.py were 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 in examine_report_text.rs (the use of the helper is gone), then the visibility change and the doc wording. Coverage should not drop: no assertion was deleted, one test became two.

…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>
@jussielo-amd
jussielo-amd marked this pull request as ready for review November 5, 2026 20:10
@jussielo-amd
jussielo-amd requested a review from a team as a code owner November 5, 2026 20:10
@jussielo-amd
jussielo-amd requested review from r0x0r and a balanced review from Copilot November 5, 2026 20:10

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

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
@jussielo-amd

Copy link
Copy Markdown
Collaborator Author

Superseded by #936: this pull request's commits are included in that branch (head a920d07), and #936's description covers this change. Closing so the review happens in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants