Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the mem_agent_detect_nvlink function in examples/mem_agent/_common.sh to safely handle cases where nvidia-smi topo -m returns no matches or fails, preventing unexpected script aborts under set -euo pipefail. It also introduces a comprehensive suite of regression tests in tests/test_mem_agent_nvlink_detect.py. Feedback on the tests suggests adding dirname to the mocked tool path in _tool_path to ensure the test for a missing nvidia-smi command does not fail prematurely during the sourcing of _common.sh.
| for tool in ("grep", "wc"): | ||
| real = shutil.which(tool) | ||
| assert real is not None, f"{tool} is required to run these tests" | ||
| (bin_dir / tool).symlink_to(real) |
There was a problem hiding this comment.
In test_missing_nvidia_smi_is_not_reported_as_no_nvlink, with_nvidia_smi=False is passed to _tool_path. This returns a restricted PATH containing only grep and wc to simulate the absence of nvidia-smi.
However, when _common.sh is sourced (line 68 of CHILD_SCRIPT), it executes dirname on line 5:
MEM_AGENT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd)"
Since dirname is not in the restricted PATH, the sourcing of _common.sh fails immediately with a 'command not found' error. As a result, the test passes (since proc.returncode != 0), but for the wrong reason: it fails during sourcing due to a missing dirname command, rather than failing inside mem_agent_detect_nvlink due to a missing nvidia-smi command.
To fix this and ensure the test validates the actual behavior of mem_agent_detect_nvlink when nvidia-smi is missing, we should also symlink dirname in _tool_path.
| for tool in ("grep", "wc"): | |
| real = shutil.which(tool) | |
| assert real is not None, f"{tool} is required to run these tests" | |
| (bin_dir / tool).symlink_to(real) | |
| for tool in ("grep", "wc", "dirname"): | |
| real = shutil.which(tool) | |
| assert real is not None, f"{tool} is required to run these tests" | |
| (bin_dir / tool).symlink_to(real) |
8468f5e to
e5fdf90
Compare
e5fdf90 to
0d2bf83
Compare
…y query errors mem_agent_detect_nvlink() ran the topology query and the match counting as one pipeline under 'set -euo pipefail'. On a machine without NVLink the grep matches nothing, exits 1, and pipefail therefore aborted the calling launcher instead of exporting NCCL_NVLS_ENABLE=0; the '2>/dev/null' on the query also hid genuine nvidia-smi failures, making both cases look identical from the outside. Split the query from the counting: a successful query with zero matches is a legal no-NVLink result and keeps setting NCCL_NVLS_ENABLE=0, while a failing or empty query is reported on stderr and returns non-zero. Adds tests/test_mem_agent_nvlink_detect.py, which drives the real _common.sh in isolated bash subprocesses with only nvidia-smi mocked (cleanup and launcher are never called). Covers pci-only, legend output, NVLink output, query failure, empty output, a missing nvidia-smi, and a pre-set NCCL_NVLS_ENABLE. Signed-off-by: 0z5a <0z5a@users.noreply.github.com> Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
0d2bf83 to
1c46441
Compare
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
|
Superseded by #460, now merged into |
Incremental review: Files changed.
Base:
0z5a-branchat791f101e20db377b28e15f12a00863baaea2605f.Allow a successful topology query with no NVLink matches under
set -euo pipefail. The previousgrepexit status aborted the launcher before it could exportNCCL_NVLS_ENABLE=0.Separate topology retrieval from counting. Report query failures, reject an empty matrix, and scope the empty-match handling to counting. The literal legend token
NV#cannot match the numeric NVLink pattern.bash -nThe checks source the real detector under
set -euo pipefailand mock onlynvidia-smi; launcher and cleanup functions are not invoked. This is a startup correctness fix, with no runtime speedup claim. Added tests and CI registration were removed.