fix(therock): stop python-resolver tests mutating the process PATH - #253
Conversation
Steps that assert on rocm's exit code printed only stdout, and rocm reports its failures on stderr. A failed serve therefore panicked with "rocm serve failed:" followed by nothing at all, which is how the MI300X lane ended up with a red scenario nobody can diagnose from CI (EAI-8031). run_rocm returns stderr already -- the five assertion sites just bound it to `_`. Replace them with a run_rocm_ok helper that panics through a shared cli_failure_report: invocation, exit code, and both streams, each labelled, each marked "(empty)" rather than omitted. The distinction matters -- "the CLI said nothing" and "the harness dropped the output" are different diagnoses, and telling them apart is the whole point. Two serve steps defer their rc assertion to a later Then step, so they now carry stderr in the world alongside the rc they already store. The formatter lives in the library, where `cargo test -p e2e-cucumber --lib` covers it in CI; the harness target's own tests never run. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`cargo test` runs every test as a thread in one process. Two python resolver tests overwrote `PATH` process-wide to point the interpreter search at a fixture directory, so for the duration of each resolve every other test lost its `PATH` too, and any test that happened to spawn a PATH-resolved binary in that window failed with ENOENT. It surfaced as `extracting_the_sdk_archive_removes_it` dying with "failed to launch tar" in 11 of 20 local suite runs; `tar` was installed the whole time. Pass the resolver's environment — the `ROCM_CLI_PYTHON` override and the directories to search — in as a value instead of reading it at the use site. The tests now hand it their fixture directory and never touch the process environment, so both `unsafe` `set_var` blocks are gone rather than serialized behind a lock. After the change the tar failure does not reproduce in 20 consecutive suite runs. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
r0x0r
left a comment
r0x0r
left a comment
There was a problem hiding this comment.
Nice root-cause writeup. The key insight — the existing mutex only serialized the two PATH-mutating tests against each other, never against the ~450 other tests sharing the process — is exactly right, and switching to injected search directories (PythonResolverEnv) is the correct fix rather than adding more locking. Production behavior is preserved (from_process_env() reads the same two variables, just once at entry), the unsafe { set_var } blocks are gone, and python_path_search_only_uses_the_given_directories pins the property so an implicit PATH read cannot creep back. LGTM.
Two non-blocking notes:
-
CI red checks are not from this change.
E2E tests (GPU)andE2E tests (Strix Halo, Windows)both fail on the pre-existingserve-hf-checkpoint-inference"1 unexpected failure(s)" regression — same signature seen on several other unrelated open branches. Your newcli_failure_reportis actually visible in those logs now (rocm serve ... failed (rc=1)), which is a nice validation that the helper works. All other checks (clippy, windows-build-and-test, affected tests, signatures) are green. -
Scope. The
cli_failure_report/run_rocm_ok/cli_stderre2e diagnostics (EAI-8031) are a good change but orthogonal to the python-resolver PATH fix — different subsystem, different ticket. Per the one-logical-change-per-PR convention they would be easier to land/revert independently. Happy either way; flagging for a possible follow-up split rather than blocking on it.
2 participants
tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No matching rows.)Summary
ROCM_CLI_PYTHONoverride and the directories to search — in as a value instead of reading it at the use site.PATHfor the whole process now hand it a fixture directory, so bothunsafeset_varblocks are gone rather than serialized behind a lock.cargo test -p rocm --bin rocmfailed in 11 of 20 runs on a cleanmainbecause of this.resolve_python_launcherreads exactly the same two variables, just once at entry instead of at each use site. Production is unaffected; the seam exists so tests do not need the process environment.Root cause
cargo testruns every test as a thread in one process.python_launcher_prefers_path_python_before_saved_managed_pythonandpython_launcher_prefers_path_python_over_managed_when_venv_capablesetPATHto a single fixture directory for the duration ofresolve_python_launcher, then restore it. That window is process-wide: while it is open, every other test also has no usablePATH, and any test that spawns a PATH-resolved binary fails withENOENT.The visible casualty was
therock::tests::extracting_the_sdk_archive_removes_it, failing with:tarwas installed the whole time. That test spawnstartwice and only the second spawn failed, which is the window opening between them.The mutex the tests already held only serialized them against each other, not against the ~450 other tests sharing the process, so it could not have helped. Removing the global mutation is what fixes it.
Verification
20 consecutive
cargo test -p rocm --bin rocmruns:extracting_the_sdk_archive_removes_itRoot cause confirmed before fixing, by running the same 20 iterations with only these two tests skipped (
--skip python_launcher_prefers_path): the tar failure disappeared, isolating it to them.Also run:
cargo clippy --locked --workspace --all-targets -- -D warnings,cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings,cargo test --workspace --all-targets --exclude e2e-cucumber,prek run --all-files.Notes for reviewers
cargo nextest, which is process-per-test and therefore immune. The Windows job runs plaincargo test, but these two tests are#[cfg(unix)]. The pain is local: the pre-push hook runscargo test, and contributors have been working around it withSKIP=cargo-test git push.python_path_search_only_uses_the_given_directoriespins the property that keeps this from coming back — with no search directories the resolver finds nothing, even though the realPATHcertainly has a python on it. It fails if anyone reintroduces an implicitPATHread.python_venv_probe_temp_root_uses_windows_temp_envstill mutatesTEMP/TMP/LOCALAPPDATAprocess-wide and still takes the mutex. It is the same class of hazard on the Windowscargo testlane, but it plays no part in this failure and I cannot verify a Windows fix from here. Worth a follow-up.