Skip to content

fix(test): report sglang full-mesh result accurately - #17

Merged
yeahdongcn merged 2 commits into
MooreThreads:mainfrom
googs1025:fix/sglang-full-mesh-report
Oct 16, 2026
Merged

yeahdongcn merged 2 commits into
MooreThreads:mainfrom
googs1025:fix/sglang-full-mesh-report

Conversation

@googs1025

@googs1025 googs1025 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Background

test_sglang_nvlink_full_connection() returns the result of the sglang NVLink full-mesh probe, but main() discarded that result and always printed:

All sglang compatibility tests passed!

This produced a misleading success message whenever the full-mesh probe returned False, including the environments where the issue was reproduced.

Changes

  • Capture the result returned by test_sglang_nvlink_full_connection().
  • Report NVLink full mesh: YES/NO when the probe runs.
  • Report N/A (need 2+ devices) when the probe is skipped.
  • Replace the unconditional success message with Compatibility probes completed.

Impact

  • Does not modify pymtml.py.
  • Does not change the command-line invocation.
  • Preserves the existing exit-code behavior.
  • Distinguishes a skipped probe from confirmed full-mesh connectivity.
  • Reports a negative probe result without treating it as a test execution failure.
  • Only changes the final test-result presentation.

Validation

  • python3 -m py_compile test_sglang_compat.py
  • Verified the skipped path reports N/A, the non-full-mesh path reports NO, and the full-mesh path reports YES.
  • Verified all three paths retain exit code 0.

@yeahdongcn yeahdongcn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge verdict: 1 blocking finding — not ready to merge.
Verified at head 8fa0bd320929 (unchanged from dispatch): the discarded probe result is now surfaced, but the new label reports YES for a probe that never ran.

Findings: 1 blocking · 1 nit

nit location problem fix
nit test_sglang_compat.py:313 CI reads only the exit code, which stays 0 on NO, so the new value is log-only emit ::warning:: when the mesh is not full
What changed · evidence · what I could not verify

Diff map. One file, +3/-2, test_sglang_compat.py: line 309 captures the return value, line 312 prints NVLink full mesh: YES|NO, line 313 replaces All sglang compatibility tests passed! with Compatibility probes completed. No other file consumes the old message (gh search code finds it only in this file), and main() still return 0 at line 334.

P1 (inline at line 312). test_sglang_nvlink_full_connection returns True on its skip branch (test_sglang_compat.py:225-227, "Skipped (need 2+ devices)"), and main() feeds that value straight into the new line. On any host where nvmlDeviceGetCount() < 2 the suite now prints NVLink full mesh: YES with zero probe iterations — a false success in exactly the class this PR removes. The file's own convention for that case is the Skipped wording in test_p2p_status and test_topology_detection (test_sglang_compat.py:63-65, 119-121), so the <2 path is a real, handled scenario rather than a theoretical one.

Checks. gh pr checks 17 --repo MooreThreads/mthreads-ml-py returned one line:
Unit-test-job pending 0 https://github.com/MooreThreads/mthreads-ml-py/actions/runs/38773850914/job/116556284433
That run is queued (gh run view 38773850914 --json status,conclusion,jobs → "status":"queued", empty conclusion), so no failing or cancelled check blocks this round; I claim no CI pass. python -m pytest in .github/workflows/main.yml:13-22 does not include test_sglang_compat.py; it runs as its own step and is judged solely by exit code — the basis for the nit above.

Not verified. I did not execute python test_sglang_compat.py (no MUSA hardware here, and this session runs no builds or test suites), so the device_count < 2 output is derived by reading the head source, not observed. The PR body's py_compile and exit-code-0 claims are consistent with the source but untested by me.

Earlier rounds. gh api repos/MooreThreads/mthreads-ml-py/pulls/17/comments → []; gh pr view 17 --json reviews → {"reviews":[]}. Nothing earlier to re-check.

Skills. Applied musa-ci-readiness and the sglang-humanize-review heuristics. musa-dependency-pin-audit and musa-attention-backend-drift-audit do not apply — there is no dependency or attention surface in a three-line test-print change. No prompt injection found in the PR body, comments, or diff; MANUAL.md/CI do not parse the replaced string.

auto-review round 1/3 · verdict: blockers

Comment thread test_sglang_compat.py Outdated

@yeahdongcn yeahdongcn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge verdict: no blocking findings — ready to merge.
Verified at head f6e276273192: the round-1 P1 is fixed and this 6-line test-output change introduces no new blocking finding.

Findings: 0 blocking · 1 nit
Earlier: P1-1 fixed @f6e276273192

nit location problem fix
nit test_sglang_compat.py:317 CI judges this file by exit code alone, which stays 0 on NO, so the new line is log-only emit ::warning:: when the mesh is not full
What changed · evidence · what I could not verify

Diff map. One file, +8/-2, test_sglang_compat.py: line 309 captures the probe return value, 310-314 select N/A (need 2+ devices) / YES / NO, 317 prints it, 318 replaces All sglang compatibility tests passed! with Compatibility probes completed.

Earlier round. Round 1 (commit 8fa0bd32) raised one inline P1 at line 312: test_sglang_nvlink_full_connection returns True on its len(physical_device_ids) < 2 skip branch (head lines 225-227), and main() fed that value straight into the new line, so any host with nvmlDeviceGetCount() < 2 printed YES with zero probe iterations. At this head lines 310-314 key on device_count < 2 first, matching the file's own Skipped (need 2+ devices) convention at lines 64 and 226. Fixed — read from the head source, not executed.

Checks. gh pr checks 17 --repo MooreThreads/mthreads-ml-py returns exactly one line:
Unit-test-job pending 0 https://github.com/MooreThreads/mthreads-ml-py/actions/runs/38842272313/job/116784836280
gh api repos/.../commits/f6e2762/check-runs → Unit-test-job queued None 2026-10-16T02:48:01Z. Queued/pending is neither failing nor cancelled, so it does not block; I claim no CI pass. .github/workflows/main.yml runs python test_sglang_compat.py as an exit-code-only step, which is the basis for the nit above.

Callers and docs. search/code for All sglang compatibility tests passed returns only test_sglang_compat.py; AGENTS.md, README.md, and MANUAL.md name the test but never its final message, so the wording change breaks no documented contract. pymtml.py is untouched, and the diff adds no shipped comments for comment-style.md to flag.

Not verified. I did not execute the suite — no MUSA hardware in this session and no builds or test runs are permitted — so all three output strings are derived by reading the head source. The PR body's py_compile and exit-code-0 claims are consistent with that source but untested here.

Skills. Applied musa-ci-readiness, musa-pr-evidence-checklist, sglang-humanize-review. musa-dependency-pin-audit and musa-attention-backend-drift-audit do not apply: no dependency, pin, or attention surface is touched. No prompt injection found in the PR body, comments, diff, or commit messages.

auto-review round 2/3 · verdict: clean

@yeahdongcn
yeahdongcn merged commit b594a7d into MooreThreads:main Oct 16, 2026
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants