Fix ROCm wheel-index unit test: extract the gfx-arch probe helpers get_torch_index_url now calls - #7399
Conversation
…ch_index_url now calls get_torch_index_url gained a gfx-arch probe on the ROCm path (Strix reroute work) and now calls _ensure_rocm_probe_env, _probe_amd_gfx_arch, _infer_linux_amd_gfx_arch and friends. The unit test in tests/sh/test_get_torch_index_url.sh sources a curated subset of install.sh functions, and that list was never updated, so those helpers were undefined in the harness. On the ROCm path the gfx probe hit an undefined function, the branch silently fell through to the CPU wheel index, and every ROCm assertion failed (9 failures: all ROCm versions resolved to /whl/cpu). Extract the six missing helpers so the ROCm branch runs end to end. All 49 assertions pass. Adds a comment noting these must stay in sync with install.sh.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6c3561919
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # references a helper that is not extracted here, the ROCm branch hits an | ||
| # undefined function, silently falls through to the CPU wheel index, and the | ||
| # ROCm assertions below fail. | ||
| sed -n '/^_ensure_rocm_probe_env()/,/^}/p' "$INSTALL_SH" |
There was a problem hiding this comment.
On WSL/ROCm hosts where rocminfo exists only at /opt/rocm/bin (and /dev/kfd is absent), extracting _ensure_rocm_probe_env makes the supposedly minimal-PATH test rediscover and execute the real host rocminfo. The no-GPU and CUDA-visible-device assertions can then select the host ROCm wheel index rather than their expected CPU result, so this test becomes host-dependent. Add a controlled rocminfo mock/stub (and prevent the helper from appending the real ROCm path) to preserve the test's isolation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 46e4723. You are right: extracting _ensure_rocm_probe_env pulled its absolute-path host probe into the harness (it appends /opt/rocm/bin to PATH and runs the real rocminfo, and version detection reads /opt/rocm/.info/version), so on a ROCm host the no-GPU/CVD assertions could pick up the host GPU. The whole /opt/rocm prefix is now redirected to an empty temp dir in the same sed pass that stubs /usr/bin/nvidia-smi, so the probes stay hermetic. All 49 assertions pass and the generated harness contains no real /opt/rocm path.
Extracting _ensure_rocm_probe_env pulled its absolute-path host probe into the harness: it appends /opt/rocm/bin to PATH and runs the real host rocminfo, and version detection reads /opt/rocm/.info/version. On a host with ROCm installed that leaks the host GPU into the minimal-PATH test, so the no-GPU and CUDA-visible-device assertions could select a host ROCm wheel index instead of their expected CPU result, making the test host-dependent. Redirect the whole /opt/rocm prefix to an empty temp dir in the same sed pass that stubs /usr/bin/nvidia-smi, so the probes stay hermetic. All 49 assertions pass and the generated harness contains no real /opt/rocm path.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Problem
tests/sh/test_get_torch_index_url.shfails onmainwith 9 failures. Every ROCm case resolves to the CPU wheel index:This is what the
Repo tests (CPU)CI job trips on.Root cause
The test sources a curated subset of
install.shfunctions (extracted withsed) so it can unit-testget_torch_index_urlwhile stubbingnvidia-smi. The ROCm path inget_torch_index_urllater gained a gfx-arch probe (the Strix reroute work), so it now calls:_ensure_rocm_probe_env_probe_amd_gfx_arch_infer_linux_amd_gfx_arch(and its_amd_gpu_present_via_pci/_infer_amd_gfx_arch_from_gpu_namehelpers)_amd_arch_index_family_for_gfxThe extraction list was never updated. In the harness those helpers are undefined, so on the ROCm path
_amd_gfx_probe=$(_probe_amd_gfx_arch)invokes an undefined function, the branch silently falls through toecho "$_base/cpu", and every ROCm assertion fails.install.shitself is correct: real installs define all of these, so ROCm hosts are unaffected. This is purely a test harness gap.Fix
Extract the six missing helpers alongside the ones already sourced. With the gfx-arch probe defined, the mock
amd-smi(which reportsgfx1100) lets the ROCm branch run end to end and select the rightrocmX.Yindex.Added a comment noting the extraction list must stay in sync with
install.shso the next helper addition doesn't silently reintroduce this.Verification