[jussielo-amd]

Summary

The vllm engine decides whether a ROCm SDK version is installed by live wheel discovery in two places, and until now each place kept its own copy of the answer. Install routing walks VLLM_ROCM_DISCOVER_BUILD_TABLE (in wheel_builds.rs), and launch-time environment setup matched a separate ROCM_DISCOVER_BUILD_VERSIONS list (in runtime.rs). The two agreed only because a test, vllm_rocm_discover_build_table_matches_the_discover_version_list, compared them. A runtime whose version was in the table but missing from the list would be installed through discovery and then launched without the discovery environment (a wrong-wheel failure far from the cause), and the test was the only guard. This change leaves one list: the table. Launch-time setup now asks the same row lookup install routing uses.

This is part of the Phase 8.4-H follow-up work. It is intended to be behaviour-neutral, but because both the install path and the launch path read the data it is reviewed as a behaviour change, so the first commit pins the behaviour before anything moves.

Which copy wins and why

The table wins. It is the richer one (wheel indexes, version prefixes, interpreter tag, torchaudio pin) and it is what install routing, the fail-closed static-pin fallback and verified_torchaudio_release (read by rocm install sdk through lib.rs) already depend on, whereas the list was only a projection of the table's version column. I compared the two on current main before changing anything: same values (10.0.0, 10.1.0), same order, and the table's rows already took their version fields from the list's two constants, so they had not diverged. The only readers of the list were runtime_has_discover_build and the comparison test. The ticket's paths are stale: the table now lives in wheel_builds.rs, not install.rs.

Deriving the table from the list is not possible (a list of strings cannot carry the rows), and deriving the list from the table with the table left in wheel_builds.rs would make runtime.rs import wheel_builds.rs, which already imports runtime.rs. So the table moves down into a new leaf module and the list is not derived at all: the one reader of it asks the table's lookup instead.

What changed

Three commits, meant to be read in order.

1. test(vllm): pin the discover-build versions and every reader's answer for them adds two tests against the unchanged code. discover_build_table_pins_the_versions_it_serves_in_order pins the table's versions and order. every_discover_row_routes_installs_and_launch_env_the_same_way is a characterisation test: for every table row, in four spellings a real probe produces (exact, a patch, a dev suffix, a .dev0+local suffix), the row lookup, the install route, the launch-environment check and the static-pin fallback all agree it is a discover build and that the static pin fails closed; for an unlisted 10.x line, 7.2.3, an unknown major, unparsable text and no recorded version they all agree it is not. It passes on current main and on this branch.
2. refactor(vllm): move the discover-build table into its own leaf module is a pure move into engines/vllm/src/discover_builds.rs: the table, VllmRocmDiscoverBuild, DiscoverVersion, vllm_rocm_discover_build, rocm_sdk_series_matches and the two version constants, with four tests that test them (leaf names unchanged). docs/architecture.md describes the module.
3. refactor(vllm): derive launch-time discover detection from the one discover-build table deletes ROCM_DISCOVER_BUILD_VERSIONS, ROCM_SDK_10_0 and ROCM_SDK_10_1 and makes runtime_has_discover_build call vllm_rocm_discover_build(version).is_some(). It also deletes vllm_rocm_discover_build_table_matches_the_discover_version_list, which compared the list with the table and is meaningless once there is one list. discover_build_table_pins_the_versions_it_serves_in_order replaces it: it pins the served versions and their order, so changing them is a deliberate edit of a literal list instead of an unnoticed one.

Module edges inside the crate after the change (the use crate:: lines): discover_builds imports nothing; runtime imports discover_builds; wheel_builds imports discover_builds (and runtime only in its tests, for VllmRuntime); install imports discover_builds, repair, runtime, state and wheel_builds as before; process imports runtime, startup_diagnostics and the crate root as before. No runtime to install edge, no new cycle, and wheel_builds no longer depends on runtime in non-test code. Items kept at unchanged paths: rocm_engine_vllm::verified_torchaudio_release (the only symbol other crates read, via lib.rs and apps/rocm/src/therock/resolve/wheel.rs) and everything in wheel_builds.rs that install.rs and lib.rs use (vllm_install_route, vllm_install_target, VllmInstallRoute, VllmInstallTarget, verified_torchaudio_release). install.rs imports the moved types from discover_builds instead of wheel_builds, which is the one path change, and it is crate-private.

Verification

Ran (Linux, target dir private to this work): cargo fmt --all --check; cargo clippy -p rocm-engine-vllm --all-targets -- -D warnings; cargo test -p rocm-engine-vllm --no-fail-fast (135 tests passed after the first commit and after the move, 134 after the consolidation); cargo xtask manifest --check; cargo xtask check-crate-edges; cargo xtask lychee; the leaf-name test list against main, which differs by exactly the intended replacement (two added: the pin and the characterisation test; one removed: the list-versus-table comparison); a leak scan of git diff origin/main...HEAD. Not run: clippy for --target x86_64-pc-windows-msvc, which stops in a dependency's build script (failed to find tool "lib.exe" from cc-rs) on this Linux host; the change touches no cfg(windows) code, only crate-private use lines and a new module with no platform gates, so I expect it to be unaffected, but CI's Windows job is what verifies that. Nothing in apps/ reads the discover-build data apart from verified_torchaudio_release, whose own tests (verified_torchaudio_release_is_only_an_exact_row_pin, and the rocm_engine_vllm::verified_torchaudio_release assertions in apps/rocm/src/therock/resolve/wheel.rs) are untouched, so I did not run the rocm package tests or the workspace gate, cargo xtask e2e or scripts/smoke_local.py; CI carries those.

Mutation proof. Against the unchanged code (first commit): dropping ROCM_SDK_10_1 from the launch list, which is exactly the divergence this removes, turns the characterisation test, the old comparison test and runtime_has_discover_build_matches_both_discover_lines red; changing a table row's key or swapping the two rows' keys turns the pin test (and the old comparison test) red. After the consolidation: making runtime_has_discover_build a second, narrower rule (version.starts_with("10.0") && ...) turns every_discover_row_routes_installs_and_launch_env_the_same_way and runtime_has_discover_build_matches_both_discover_lines red; swapping the two rows' keys turns the pin test, discover_rows_select_their_own_indexes and one install test red. Each mutation was reverted by a targeted reverse edit.

User-visible behaviour

None, so no new Gherkin scenario. The engine ids, version strings, the environment a launched vLLM process receives and the install route chosen for any input are unchanged, which the characterisation test and the existing apply_therock_env_dispatches_to_discover_env_for_a_discover_rocm_sdk_version, vllm_install_route_* and unknown_rocm10_line_still_fails_closed tests exercise. No CLI output, exit code or file the CLI creates changes. No Windows-only code is touched.

Notes for reviewers

Reading order is the commit order above; the second commit is mechanical (move with git diff -M or --color-moved), the third is the whole behaviour-relevant change and is small. The only deliberate difference in logic: runtime_has_discover_build used to test the version against a list of keys with rocm_sdk_series_matches, and now calls vllm_rocm_discover_build, which tests it against the table's keys with the same function, so the predicates are identical while the two key sets are identical, and cannot differ afterwards.

Open PRs that touch the same files: none touch runtime.rs, wheel_builds.rs or install.rs of the vllm engine. Three (#724, #695, #202) add lines to engines/vllm/src/lib.rs and may conflict trivially with the added mod discover_builds; line; #724, #695 and #202 also edit process.rs, which this change does not. docs/architecture.md is edited by many open PRs, so expect a textual conflict there; the edit is confined to the engines/vllm bullet list.

[Copilot]

🟢 Approval recommended

No active review issues block the refactor consolidating vLLM discovery-build metadata into a shared table.

No open issues 🎉

What changed in this PR

Consolidates vLLM discovery-build metadata into one shared table so install routing and launch-time environment setup cannot drift apart. The refactor preserves existing version matching, wheel selections, and public exports.

Changes:

• Moves the discovery table, types, lookup, and version matcher into a dependency-free leaf module.
• Replaces the separate launch-time version list with the shared table lookup.
• Adds characterization tests, pins supported versions and order, and updates architecture documentation.

File	Description
engines/​vllm/​src/​wheel_builds.rs	Uses shared discovery metadata and tests agreement between install and launch decisions.
engines/​vllm/​src/​runtime.rs	Derives discovery detection from the shared lookup instead of a separate version list.
engines/​vllm/​src/​lib.rs	Registers the private discover_builds module.
engines/​vllm/​src/​install.rs	Updates imports to use the extracted discovery module.
engines/​vllm/​src/​discover_builds.rs	Houses discovery recipes, matching helpers, and focused tests.
docs/​architecture.md	Documents the shared table and revised module responsibilities.

_{Review effort: Balanced}

[juhovainio]

I read through the diff and found nothing blocking. runtime_has_discover_build still matches the same two keys via rocm_sdk_series_matches, the new leaf module imports nothing (no cycle), the moved code is unchanged apart from paths and visibility, and the old constants have no remaining references. The pin test is a clear improvement over the list-vs-table comparison it replaces.

Two small notes, neither blocking:

• One inline comment on the test helper in wheel_builds.rs.
• The intra-doc link in discover_builds.rs now points at vllm_install_target (it used to say resolve_vllm_install_target); worth confirming it resolves under cargo doc.

This is based on reading the diff only; I did not build or run the tests. The two Strix Halo Ubuntu e2e lanes (nightly/current and release/current) were red when I looked, and MI300X/W7900 were still pending. Since the change is behaviour-neutral they look like lane flakes, but please confirm with a rerun before merging.

[juhovainio]

This helper builds every VllmRuntime field by hand in a module unrelated to runtime.rs, so any new field on the struct breaks it. A shared test constructor next to the struct would be sturdier. Non-blocking.

[jussielo-amd]

Added a #[cfg(test)] VllmRuntime::for_test() next to the struct in runtime.rs and switched this helper to it with struct-update syntax, so a new field is defaulted in one place. I applied the same change to the other test modules that built the struct by hand (runtime.rs, state.rs, process.rs; 14 sites). No test was added, removed or renamed and all 134 still pass; removing the rocm_sdk_version override from this helper turns the characterisation test red (cc726d3).

[jussielo-amd]

Thanks for the approval. Pushed cc726d3 (the shared test constructor from your inline note) and 388dd85. On the intra-doc link: vllm_install_target resolves under cargo doc -p rocm-engine-vllm --no-deps with warnings denied, both by default and with --document-private-items, and main is clean in both modes. The private-items run did show two other links the move had broken (VLLM_ROCM_BUILD_TABLE, which is module-private and so can no longer be linked from a sibling module, and verified_torchaudio_release, now pointing at crate::wheel_builds); 388dd85 fixes both and changes comments only. cargo test -p rocm-engine-vllm passes (134), clippy and fmt are clean. The two Strix Halo Ubuntu lanes you saw red are the known serve-40 xfail issue (#898) and not touched by this change.