feat(diagnose): report a code object manager that is not the runtime's own - #385
volen-silo wants to merge 1 commit into
Conversation
7d0af16 to
9452104
Compare
87cbcac to
a1968d9
Compare
9452104 to
4e5fe59
Compare
a1968d9 to
22e9b4a
Compare
4e5fe59 to
6f617c8
Compare
22e9b4a to
5c39eee
Compare
6f617c8 to
09476f8
Compare
5c39eee to
f1a1b9b
Compare
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
This is a solid feature (surfacing comgr/HIP runtime mismatches), but I found a few issues that affect correctness of the diagnosis output and the fix advice text, plus a couple of secondary concerns. Left inline comments on the ones anchored to this diff; one test-coverage gap below since it points at a file this PR doesn't touch.
Untested field, tests/e2e-cucumber/tests/e2e/examine_steps.rs (not modified by this PR): the e2e assertion for comgr_paths entries still only requires ["path", "real_path", "version", "source"] and wasn't extended to require the new install_root field this PR adds to the wire format, and no e2e step checks hip_paths/hip_selected either. Right now only in-process unit tests exercise install_root — the actual CLI --json output for the field this design turns on is unverified end-to-end.
Requesting changes mainly for the fix-18 advice/data mismatch (correctness bug in user-facing guidance) and the comgr_matches_runtime vs diagnose disagreement — both are logic errors, not style nits.
| "# Then pick ONE of the following. They are alternatives, not steps.", | ||
| "# (a) Keep the system installation: remove or uninstall the wheel that", | ||
| "# supplies the second copy.", | ||
| "# (b) Keep the wheel: order the search path so the runtime's own copy", |
There was a problem hiding this comment.
Option (b) here says "Keep the wheel: order the search path so the runtime's own copy is found first", followed by an LD_LIBRARY_PATH command — but per this PR's own conflict definition (diagnose.rs's matching), "the runtime's own copy" is the copy belonging to the currently-active HIP runtime, which in the canonical wheel/system conflict is the system install, not the wheel. A user who wants to keep the wheel and follows this advice literally will export LD_LIBRARY_PATH to the system comgr directory instead — the opposite of the stated intent — and there's no obvious symptom afterward besides now picking up the system's libraries. Worth double-checking which path this recipe actually resolves to for option (b) and fixing the wording/command so it points at the wheel's own comgr, not the runtime's.
| // `None` rather than `false` when either side is missing: "they disagree" and | ||
| // "there was nothing to compare" are different answers, and a caller that | ||
| // cannot tell them apart reports a conflict on a machine with no ROCm at all. | ||
| e.comgr_matches_runtime = match (comgr.first(), hip.first()) { |
There was a problem hiding this comment.
The documented --json field comgr_matches_runtime is computed with a weaker rule (comgr.install_root != hip.install_root, both non-empty) than check_18_comgr_conflict, which additionally requires the active runtime to ship a matching comgr copy of its own before firing. When the runtime ships no comgr copy of its own — the exact case this PR's own test a_runtime_with_no_copy_of_its_own_is_not_a_conflict covers — rocm examine --json will report comgr_matches_runtime: false while rocm diagnose reports nothing at all. Anyone consuming this field programmatically (or a user comparing the two commands) sees a contradiction. Suggest computing this field with the same logic check_18_comgr_conflict uses.
| crate::collect_sdk_library_paths(&root, &mut paths); | ||
| for path in paths { | ||
| add(path, "managed-runtime"); | ||
| crate::collect_sdk_library_paths(root, &mut paths); |
There was a problem hiding this comment.
install_library_dirs now calls collect_sdk_library_paths (which also scans root/bin and root/lib/rocm_sysdeps/lib) unconditionally for every root, whereas the code this replaces (comgr_search_dirs) only added root/lib/root/lib64 for rocm-install-sourced roots. This silently widens what's probed for plain bare-metal /opt/rocm installs — e.g. a vendored dependency under lib/rocm_sysdeps/lib or bin that happens to match the comgr/hip library name pattern would now be picked up as an extra copy that never showed up before this PR, changing comgr_selected/comgr_paths ordering and copy counts on existing installs. This behavior change to an established field isn't called out in the PR description — was the widened scan for bare-metal roots intentional?
| let mut copies: Vec<LibraryCopy> = Vec::new(); | ||
| let mut seen: std::collections::BTreeSet<String> = std::collections::BTreeSet::new(); | ||
|
|
||
| // Order is the whole point: this is the order the loader consults, so the |
There was a problem hiding this comment.
find_library_copies recomputes install_library_dirs(roots) and re-invokes paths_in_loader_cache (which spawns ldconfig -p) once per prefix, so both now run twice per rocm diagnose/rocm examine call — once for comgr, once for hip — even though neither depends on which prefix is being searched. That's an extra ldconfig -p subprocess plus a second full walk of every managed runtime's lib/lib64 → pythonX/site-packages → _rocm_sdk_* directories on every invocation of a command that's typically run repeatedly while debugging. Might be worth hoisting the roots/loader-cache computation out so it's shared between the two calls.
| .into_iter() | ||
| .map(|root| (root, "managed-runtime")), | ||
| ); | ||
| roots.dedup_by(|a, b| a.0 == b.0); |
There was a problem hiding this comment.
known_install_roots dedups with Vec::dedup_by, which only removes adjacent duplicates. e.rocm_path is pushed first and the sorted /opt/* siblings are appended after, so if e.rocm_path equals one of those sorted entries but isn't adjacent to it in the final vector (e.g. e.rocm_path is /opt/rocm-6.4 and /opt also contains rocm and rocm-6.4, sorting to [rocm, rocm-6.4]), the roots vector ends up [rocm-6.4, rocm, rocm-6.4, ...] and the two rocm-6.4 entries survive the dedup. install_library_dirs/find_library_copies then scan that install's directories twice per probe — masked from the final output only because record_library_copy separately dedups by resolved real path downstream. Sorting the full vector (or using a HashSet) before dedup would close this.
f1a1b9b to
c591f8a
Compare
d9b0a75 to
f2bd0c9
Compare
c591f8a to
2a77f96
Compare
6ea3a21 to
ee48870
Compare
2a77f96 to
c6cfd42
Compare
ee48870 to
a8749a3
Compare
991f5d8 to
166d9ed
Compare
r0x0r
left a comment
r0x0r
left a comment
There was a problem hiding this comment.
What the change does. Teaches examine/diagnose to notice that the libamd_comgr copy the loader would pick belongs to a different installation than the libamdhip64 runtime, reports the HIP copies alongside the comgr ones, and adds fix-18-comgr-conflict as an advisory-only entry that states both remedies without ranking them.
Coverage. Full read of crates/rocm-core/src/{examine,diagnose,fix,lib}.rs and the four e2e-cucumber files at head, reviewed against the PR's own base (feat/report-comgr-copies), so #384's changes are excluded. Checked against AGENTS.md §3. Both findings from the earlier, now-dismissed review were re-tested specifically. No tests run.
Assessment: looks sound — no blocking findings survived verification.
The earlier review's two findings are addressed
-
fix-18 advice/data mismatch — fixed. The machine-specific
Fixindiagnose.rsderives itsLD_LIBRARY_PATHdirectory frommatching.real_path, wherematchingis the comgr copy whoseinstall_rootequals the HIP runtime's, so "order the search path so the runtime's own copy is found first" now describes what the command actually does. The<directory of the wheel's own copy>placeholder that remains infix.rsis the generic catalog recipe, which has no live paths to fill in — correct for its role. -
comgr_matches_runtimevsdiagnosedisagreeing — fixed.probe_comgrandcheck_18_comgr_conflictnow call the sameexamine::comgr_matches_runtime, andcomgr_matches_runtime_never_disagrees_with_the_diagnosispins that invariant across the catalog cases, including the one that used to diverge (a runtime shipping no comgr copy of its own). Making the disagreement structurally impossible, rather than fixing the two sites to agree, is the right shape of fix. -
The e2e gap is closed too.
assert_comgr_copies_reportednow requiresinstall_root:for field in ["path", "real_path", "version", "source", "install_root"] {
and two new steps assert
hip_pathsandhip_selected, wired into examine-16.
AGENTS.md §3 is satisfied: diagnose-21 (@id:diagnose-fix-comgr-conflict-is-advisory-only) covers the user-observable half, and its comment explains honestly why the conflict itself cannot be provoked in the suite — the detection rule is left to unit tests that build machine state directly. That is the explanation §3 asks for rather than a silent omission.
Non-blocking
managed_runtime_roots()runs twice perfind_library_copies.known_install_roots(examine.rs:2170) calls it to buildroots, theninstall_library_dirs(:2200) calls it again to buildmanaged_library_paths, andprobe_comgrruns the pair twice (comgr, then HIP) — four registry walks per examination. It is a deterministic filesystem read, so this is cost rather than correctness, but threading the first result through would remove both the cost and any question about the two views agreeing.comgr_version_from_file_nameis now called for HIP paths too (record_library_copy). The soname parsing is correct for both; only the name still says comgr.assert_hip_selection_is_statedpasses trivially on the mock lane — withhip_paths == []it only assertship_selectedis null, so the list/selection agreement is never exercised off a GPU lane. It inherits this from the comgr sibling rather than introducing it.assert_fix_is_advisoryusescontains("print-only") || contains("will NOT run it"), so dropping either string still passes.assert_both_options_unrankedcovers the dangerous case, so this is slack rather than a hole.- Untested: the ordering contract in
find_library_copies(LD_LIBRARY_PATH beats loader cache beats install dirs) is the whole point of the function and has no unit test; neither does the partial-version evidence branch indiagnose.rswhere exactly one side has a version string.
Tradeoffs
comgr_matches_runtimereturnsNoneboth for "no known installation claims this copy" and for "the runtime ships no comgr of its own". Serialized, a consumer cannot tell those apart from "not computed". The comments document the distinction; the wire format does not carry it.- The symmetric rule ("do both come from the same installation?") stays quiet on a machine holding only the mismatched comgr, because there would be no alternative to point at. Deliberate and documented, but it does mean a genuinely broken single-stack install reports nothing.
Positive signals
- Driving the JSON field and the diagnostic from one function, then testing that they agree, removes a whole class of future drift.
- Longest-prefix attribution in
owning_install_roothandles managed runtimes spread across_rocm_sdk_*subdirectories without special cases, anddedup_roots_keeping_firsthas a test for the non-adjacent duplicate that a naive consecutive dedup would miss. auto_applicable: false, with both remedies stated and neither recommended, is the right posture for a fix where either choice can break a working Python environment.#[serde(default)]on every new field keeps older consumers reading the new output.
One thing outside the diff
This PR's base is #384, which currently has changes requested and 6 failing checks, so #385 cannot land ahead of it regardless of its own state. Its own checks are green at this head apart from one still running.
The merge decision is yours; this review is posted as a comment and files no approval or change request.
🤖 by agent-hub on AMD AgentHub
r0x0r
added
agent-hub-reviewed
166d9ed to
1837ea8
Compare
a8749a3 to
51a20ff
Compare
…s own Builds on the copy search: with every copy of libamd_comgr known, the catalog can say when the one that loads belongs to a different installation than the HIP runtime that loads, and device code compilation therefore fails with an error naming neither. The rule is symmetric, and that is what makes it safe. The design note for this entry proposed a special case -- "when the active runtime is the managed runtime, the matching copy is the wheel copy" -- without which it "fires on every healthy CLI installation". That is a patch over a rule stated asymmetrically. Asking one question of both libraries instead, does the libamd_comgr that would load come from the same installation as the libamdhip64 that would load, makes every case fall out of the rule: a healthy managed install takes both from the managed runtime, so nothing differs and nothing is reported. A control that has to be written as an exception is a rule that has not been stated correctly yet. That turns on attributing a copy to its installation correctly. A managed runtime spreads its libraries across separate _rocm_sdk_* packages, so ownership is decided by matching against known installation roots, longest first -- never by walking up from the file, which would call each package its own install and fire on the most common install we ship. That is the property the mutation check targets. Two further guards on firing: an unattributed copy is a gap in what the search knows rather than a finding about the machine, and the runtime must actually ship a copy of its own, or there is nothing to point the user at. The entry is print-only and ranks neither remedy. Removing a stack and reordering the search path can each break a working Python environment, and which is right depends on which stack the user means to keep. The finding says out loud that it describes the environment outside the managed runtimes. `rocm serve` puts a managed runtime's libraries first on purpose and gets a different, correct answer, and a report that did not name the environment it examined would read as a claim about one it never looked at. The rule above is now stated exactly once, as `comgr_matches_runtime` in examine.rs, and both this entry and `rocm examine --json`'s field of the same name call it rather than each restating the conditions -- so the two surfaces cannot disagree about one machine, and a regression test pins that. Fix option (b)'s advice matched an earlier, asymmetric statement of the rule; it now says to prefer the wheel's own copy of both libraries, which is what the symmetric rule actually asks the user to do. The machine-readable inspection now carries the same `install_root` attribution for the HIP runtime side that it already carried for the code object manager side, since the conflict question is about both libraries and a reader could not previously check either the attribution or the selection for one of them. Known roots are deduplicated across the whole list, not only where repeats happen to land next to each other. `dedup_by` drops consecutive duplicates, and these roots arrive from three independent sources -- the active install, a sorted /opt scan, and the managed runtimes -- so the active install colliding with a sibling was caught or missed depending on where the path sorted. A root recorded twice can make a machine holding one stack read as a machine holding two, which is the false conflict this entry exists not to report. hip_paths and hip_selected are `serde(default)` for the same reason the comgr fields are: an examination read back over the remote path may have been produced by an older CLI that never wrote them. Once a managed runtime is found, its root resolves to its `_rocm_sdk_devel` package directory, and the search used to guess a venv layout (`root/lib/<python>/site-packages`) to find its sibling packages; that guess does not hold for a real install, so the search now reads back the library directories the SDK probe already recorded (`library_paths`) instead of re-deriving them. This generalises the walker introduced in #384 (`find_library_copies`/`install_library_dirs`) so it serves the HIP side too, which is why `managed_runtime_roots` now returns `library_paths` plural rather than a single `site_packages` guess. The AppPaths::discover data-dir fix and the @requires-os:linux platform tags this commit originally carried for examine-17/18 belong with the scenario they correct, which #384 introduced -- moved there so the parent doesn't depend on the child to pass its own CI, and this commit now only rebases on top of that fix rather than restating it. Sweeping the same gap once it was visible: examine-17 (the host-independent code-object-manager scenario, renumbered by #384's rebase) asserts `hip_paths`/`hip_selected` the same way it asserts `comgr_paths`/`comgr_selected`, and the empty branch had the identical problem #384's review flagged for the comgr side -- `hip_paths: []` / `hip_selected: null` also hold by nothing more than `Examination`'s own defaults, so the assertion could not tell "probed, found none" from "never probed". `probe_comgr` now pushes a "no libamdhip64 found" note the same way it already did for libamd_comgr, and the HIP assertion now requires it in the empty branch. Verified by temporarily suppressing the note and watching the scenario go red, then restoring it. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
1837ea8 to
4236280
Compare
3 participants
What
#384 made every copy of
libamd_comgrvisible. This adds the diagnosis: the catalog now reports when the copy that would load belongs to a different installation than the HIP runtime that would load — the state in which device code compilation fails with an error naming neither the library nor the second copy.Why the rule is symmetric
The design note for this entry proposed a special case: "when the active HIP runtime is the managed runtime, the matching copy is the wheel copy. Without this rule the entry fires on every healthy CLI installation."
That is a patch over a rule stated asymmetrically. Asking one question of both libraries instead — does the
libamd_comgrthat would load come from the same installation as thelibamdhip64that would load? — makes every case fall out of the rule rather than being legislated:/opt/rocm/opt/rocmA control that has to be written as an exception is a rule that has not been stated correctly yet.
The detail it all turns on
A managed runtime spreads its libraries across separate
_rocm_sdk_*packages. Ownership is therefore decided by matching against known installation roots, longest first — never by walking up from the file, which would call each package its own installation and fire on the most common install we ship.That is not a theoretical concern, and it is what the mutation check targets: replacing the attribution with path-derivation turns
a_library_deep_inside_a_managed_runtime_belongs_to_the_runtimered.Two further guards on firing
It advises and ranks nothing
Print-only. Removing a stack and reordering the search path can each break a working Python environment, and which is right depends on which stack the user means to keep — a question only they can answer. The finding also states that it describes the environment outside the managed runtimes:
rocm serveputs a managed runtime's libraries first on purpose and gets a different, correct answer, and a report that did not name the environment it examined would read as a claim about one it never looked at.Verification
cargo test --workspace --all-targets --exclude e2e-cucumber— clean;rocm-core352 passedcargo clippy --locked --workspace --all-targets,cargo fmt --all --check— cleancargo xtask e2e -- -n diagnose— 17 scenarios, 14 passed. The 3 failures are the@requires-bare-metalscenarios that cannot pass on a WSL2 dev host, unchanged from before this PR.Every new assertion was verified to fail, not just to pass.
remove, which also appears in the surrounding prose, so deleting one of the two options would still have passed. It now keys on the option markers, and deleting option (a) turns it red.Two guards fired during the work and were updated deliberately: the
Examinationtop-level wire contract, and the catalog entry count.CI
examine-17(examine-finds-the-managed-runtimes-own-compilation-library, introduced in #384, before this PR) was red on the Strix Halo Windows lane, the only GPU lane still failing on this branch. Two separate bugs, both pre-existing, neither caused by this diff:crate::runtime::default_data_dirinstead ofAppPaths::discover, so it disagreed with the rest of the CLI and found nothing under e2e's isolated data dir on every GPU family. Fixed in this commit (see commit message) — this alone cleared MI300X, MI350P, rad3 R9700, and Strix Halo Ubuntu/WSL2.probe_comgr's whole search --LD_LIBRARY_PATH, the loader cache, and the install-rootlib/scan -- is built around ELF shared-object names (libamd_comgr,libamdhip64). Native Windows has none of that and ships the equivalent libraries under different names, so the scenario's assertion ("a managed runtime is installed, so the inspection cannot report zero") never held there. This is the same boundaryfix-18-comgr-conflictis already gated on (&["linux", "wsl"]in diagnose.rs) and that this commit already tags onto diagnose-21 for the identical reason -- examine-17 just didn't get the same tag when it was added in feat(examine): report every code object manager library, not just the first #384. It now carries@requires-os:linux, which still runs the scenario on WSL2 (os_familyreports "linux" there) and skips it only on native Windows.Coverage boundary
The suite cannot install a second ROCm stack, so e2e cannot provoke a real conflict. It pins the half that matters if the entry ever stops being advisory — asking for the fix changes nothing and recommends neither option. The detection rule is proven by unit tests that construct the machine state directly, including the two cases that must stay silent.
Risk
Medium, concentrated in one place: a false report on a healthy managed installation. That is the risk the ticket names, and the symmetric rule plus root-based attribution are the two things standing against it, each with a test that fails when broken.
Follow-up
Overlaps #382, which replaces
auto_applicablewith a per-platform class. This branch is off #384, so the new recipe uses the current bool; whichever lands second rebases. If #382 lands first this entry becomesPRINT-ONLYin the new class directly.