Skip to content

Nothing to see here #1189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Oseltamivir wants to merge 3 commits into from
Closed

Nothing to see here #1189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff53521
Lower MIN_EVAL_CONC to 8 and run evals for dsv4-fp4-b300-sglang-mtp
Oseltamivir Apr 26, 2026
e18036f
Merge branch 'main' into chore/dsv4-b300-mtp-eval-min-conc
Oseltamivir Apr 27, 2026
ec065b3
Update perf-changelog.yaml
Oseltamivir Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions perf-changelog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1886,3 +1886,10 @@
- "Image pinned to lmsysorg/sglang:deepseek-v4-blackwell@sha256:df18bfc4aa9ecf59451002b49ba00cae58042de9e2a96378bbd21b404dd62c7b"
- "Adds SGLANG_OPT_* env knobs (SWA_SPLIT_LEAF_ON_INSERT, USE_JIT_NORM, USE_JIT_INDEXER_METADATA, USE_TOPK_V2, USE_CUSTOM_ALL_REDUCE_V2)"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1187

- config-keys:
- dsv4-fp4-b300-sglang-mtp
description:
- "Run evals for DSv4 FP4 B300 SGLang MTP"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1189
evals-only: true
Comment thread
Oseltamivir marked this conversation as resolved.
2 changes: 1 addition & 1 deletion utils/matrix_logic/generate_sweep_configs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"8k1k": (8192, 1024)
}

MIN_EVAL_CONC = 16
MIN_EVAL_CONC = 8

# Reverse mapping for exp-name generation
seq_len_itos = {v: k for k, v in seq_len_stoi.items()}
Expand Down
2 changes: 1 addition & 1 deletion utils/matrix_logic/test_generate_sweep_configs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ def test_multi_node_eval_conc_uses_only_conc_values_at_or_above_min_conc(self):
result = mark_eval_entries(matrix_values)

assert result[0]["run-eval"] is True
assert result[0]["eval-conc"] == 32
assert result[0]["eval-conc"] == 16
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 26, 2026

Choose a reason for hiding this comment

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

🟡 Nit: After lowering MIN_EVAL_CONC to 8, test_single_node_skips_eval_entries_below_min_conc (lines 251-308) no longer exercises the threshold-filter code path it claims to test — its conc values [8, MIN_EVAL_CONC=8, 32, 64] all pass _eligible_eval_concs (c >= MIN_EVAL_CONC), and the conc=8 entries end up run-eval=False only because the highest/median selector picks {32, 64}. The test still passes but has lost coverage of the threshold guard. Consider lowering one entry to conc=4 to restore meaningful coverage. The PR updated the sibling multinode assertion (line 336) but missed updating this single-node test.

Extended reasoning...

What the bug is

The sibling test test_single_node_skips_eval_entries_below_min_conc in utils/matrix_logic/test_generate_sweep_configs.py (lines 251-308) advertises (via name and docstring) that it covers the "skip entries below MIN_EVAL_CONC" threshold path in _eligible_eval_concs at generate_sweep_configs.py:58:

return sorted(c for c in conc_values if c >= MIN_EVAL_CONC)

Before this PR, MIN_EVAL_CONC=16 and the test's four entries had conc=[8, MIN_EVAL_CONC=16, 32, 64]. The two conc=8 entries failed the >= MIN_EVAL_CONC filter and were dropped via the if not _eligible_eval_concs(entry): continue guard at the top of the single-node loop — exactly the path the test was designed to cover.

Why it no longer exercises the threshold path

After this PR lowers MIN_EVAL_CONC to 8, the entries become [8, MIN_EVAL_CONC=8, 32, 64]. Trace:

  1. All four entries pass c >= MIN_EVAL_CONC (since 8 >= 8), so all four reach the grouping step.
  2. Grouped by (model, runner, framework, precision, isl, osl, spec-decoding, dp-attn) — all entries share the same key.
  3. sorted(set(conc_values)) = [8, 32, 64]; median = conc_values[len // 2] = conc_values[1] = 32; highest = 64; target_concs = {32, 64}.
  4. The two conc=8 entries get run-eval=False purely because they aren't in target_concs — not because the threshold filter dropped them.

Counterfactual proof

If you completely removed the c >= MIN_EVAL_CONC guard from _eligible_eval_concs, the unique conc set, median, and highest would be unchanged, and this test would still pass with the same assertions. So the threshold filter the test name advertises is no longer guarded by this test.

Impact

  • The test still passes — no functional bug in production code.
  • However, the single-node threshold-filter code path has lost regression coverage. A future bug that, e.g., dropped or weakened the c >= MIN_EVAL_CONC filter for single-node entries would not be caught by this test.
  • The multinode sibling test_multi_node_skips_groups_with_only_conc_below_min_conc (with conc=[1]) still exercises the threshold path, so the multi-node side retains coverage.

How to fix

Lower one entry's conc below the new threshold (e.g., conc=4) so the filter path is still tested. Example: change one of the two conc=8 entries to conc=4. With entries [4, 8, 32, 64], the conc=4 entry is dropped by _eligible_eval_concs (restoring threshold coverage), the remaining unique set is {8, 32, 64}, median=32, highest=64, so the existing assertions for the other entries still hold.

Severity rationale

Nit. The test passes, no production behavior is wrong, and the multi-node sibling test still covers the threshold guard. The only regression is in test-coverage documentation/guarantee for the single-node path.

assert result[1]["run-eval"] is False

def test_marks_highest_and_median_conc(self):
Expand Down
Loading