Skip to content

Delete runners/test_dsv41flash_b300.py - #3273

Merged
cquil11 merged 1 commit into
mainfrom
cquil11-patch-1
Sep 18, 2026
Merged

cquil11 merged 1 commit into
mainfrom
cquil11-patch-1

Conversation

@cquil11

@cquil11 cquil11 commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@cquil11
cquil11 requested a review from a team September 18, 2026 20:55
@cquil11
cquil11 merged commit 927613f into main Sep 18, 2026
4 checks passed
@cquil11
cquil11 deleted the cquil11-patch-1 branch September 18, 2026 20:55

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 runners/test_dsv41flash_b300.py — This PR deletes runners/test_dsv41flash_b300.py outright, leaving benchmarks/single_node/agentic/dsv41flash_fp4_b300_vllm_mtp.sh's capture-tier branching (TP/CONC thresholds for --max-cudagraph-capture-size, --max-num-batched-tokens, --compilation-config, --gpu-memory-utilization) with zero test coverage. No other test in runners/ or elsewhere references this script. AGENTS.md's Test quality section only allows deleting a test without replacement if it fails the four kept-test questions (structure-only, config-pinning, tautological, duplicate) — this test ran the real script via subprocess with stubbed binaries and asserted on computed vllm args, which is exactly the pattern the guide requires ('Shell scripts are tested by running them with stubbed binaries on PATH and checking what they…

    Extended reasoning...

    …produced'). Fix: restore equivalent behavior coverage for the three capture/concurrency tiers in dsv41flash_fp4_b300_vllm_mtp.sh, or justify why the branch logic no longer needs a regression test.

    The deleted test parametrized (tp, concurrency) over (4,1), (4,8), (2,128) and asserted the script's if/elif/else in dsv41flash_fp4_b300_vllm_mtp.sh (lines ~33-40) picks the right GRAPH_SIZES tier, MAX_BATCHED_TOKENS, and GPU_MEMORY_UTILIZATION. After this deletion, a future edit that inverts the CONC<=4 or CONC>=128&&TP==2 condition, or breaks CAPTURE_SIZE derivation from GRAPH_SIZES, or drops --gpu-memory-utilization forwarding, passes CI silently. Nothing else in the repo runs this script or checks its emitted vllm CLI args (grep for dsv41flash_fp4_b300_vllm_mtp and for test_dsv41flash_b300 across the repo returns no other hits). The consequence is a benchmark launcher regression reaching production runs undetected, since docs/testing.md's CI auto-discovery only catches removed tests, not silently-broken shell logic.

    Verification: nit. The deleted runners/test_dsv41flash_b300.py was a legitimate behavior test, exactly the kind AGENTS.md line 90 says to keep ("Shell scripts are tested by running them with stubbed binaries on PATH and checking what they produced, not by reading their text"). It sourced the real benchmarks/single_node/agentic/dsv41flash_fp4_b300_vllm_mtp.sh with stubbed source/hf/nvidia-smi/vllm/etc.,…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant