Skip to content

test: pin apply_multi_node_invalid_variants CONC filter (PRPUNDIT-10) - #1325

Open
jiagaoxiang wants to merge 4 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-10
Open

test: pin apply_multi_node_invalid_variants CONC filter (PRPUNDIT-10)#1325
jiagaoxiang wants to merge 4 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-10

Conversation

@jiagaoxiang

Copy link
Copy Markdown
Collaborator

Summary

  • Locks the single-node no-op, multi-node --cuda-graph-max-bs N < CONC drop/keep split, CONC=0 guard, and unparseable CONC fallback to 64.

Closes test gap PRPUNDIT-10.

Test plan

  • PYTHONPATH=src pytest src/hyperloom/inference_optimizer/tests/test_grid_variant_filter.py

Lock the single-node no-op, multi-node cuda-graph-max-bs drop/keep split, CONC=0 guard, and unparseable CONC fallback to 64.
@jiagaoxiang
jiagaoxiang requested a review from a team as a code owner August 28, 2026 07:13

@zoroyihan7 zoroyihan7 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.

Thanks — good coverage of the observable behaviour: both flag spellings, the at-threshold boundary, order preservation, and kept is grid on the no-op path.

Mutation testing found three producer-side holes, all in the same area.

1. The regex is applied with .search() but every test puts the flag first. All four tests build extra_server_args as a single flag that starts the string ("--cuda-graph-max-bs 32", "--cuda_graph_max_bs=8", "--chunked-prefill-size 8192"). Mutating .search(.match( at _grid_variant_filter.py:201 leaves all 4 green. Real variants are multi-flag strings (see _grid_server_args.py:369-372 and the explore.py:1036 call site), so a regex-anchoring regression would ship silently. One variant with the flag in non-leading position — e.g. "--tp 8 --cuda-graph-max-bs 32" — closes it.

2. The 64 default has three producers and only one is pinned. Line 197 is int(os.environ.get("CONC", "64") or 64):

producer pinned?
ValueError → 64 ✅ by test_unparseable_conc_falls_back_to_64
get default "64" (CONC unset) ❌ mutating "64""1" stays green
or 64 (CONC empty string) ❌ mutating or 64or 1 stays green

Every test does monkeypatch.setenv("CONC", ...), so CONC-unset and CONC-empty are never exercised. Two delenv/setenv("CONC","") cases would cover both. The or "" None guard on line 201 is likewise unpinned.

3. test_conc_zero_does_not_drop can't fail. Deleting conc > 0 and from line 202 leaves all 4 green — and it can't be otherwise with the code as written, since the regex captures (\d+) so the parsed value is always >= 0, and n < conc is already False for every conc <= 0. The guard is genuinely unreachable as a differentiator. Either drop the redundant guard from production and keep this test as a behaviour doc, or add a comment saying the test pins the observable behaviour rather than the guard line. (Not asking you to change production here — just flagging that this one doesn't buy what the ticket assumed it would.)

Nits:

  • kept is grid (identity) is stricter than the docstring and stricter than the sibling filters — apply_aiter_moe_pin_filter:269 and apply_user_skip_list:577 both return list(grid) on their no-op path. A future harmless defensive copy would turn this red. If the identity is deliberate, a one-line comment would help; otherwise == grid plus an order assertion is safer.
  • The dropped reason is asserted only via the "CONC=64" substring, which is shared by the explicit-CONC and fallback tests, so it doesn't distinguish them on its own. Asserting the cuda_graph_max_bs=32 half too would make the row self-describing.

@zoroyihan7 zoroyihan7 added the skip-e2e-test It's a PR that doesn't need to be e2e tested label Aug 31, 2026
jiagaoxiang and others added 2 commits August 31, 2026 19:48
…e conc>0 guard

Production (_grid_variant_filter.py:202): drop 'conc > 0 and'. The regex (\d+)
guarantees conc >= 0 always, so the guard is dead code; 'n < conc' is already
False for every n >= 1 when conc == 0, preserving the same observable behaviour.

Tests:
- test_multi_node_flag_in_non_leading_position_is_detected: flag embedded in a
  multi-flag string ('--tp 8 --cuda-graph-max-bs 32') pins re.search vs re.match
- test_conc_unset_defaults_to_64: monkeypatch.delenv exercises the os.environ.get
  default '64' path (mutating '64' → '1' now turns this red)
- test_conc_empty_string_defaults_to_64: CONC='' exercises the 'or 64' branch
  (mutating 'or 64' → 'or 1' now turns this red)
- Add comment to test_conc_zero_does_not_drop explaining it documents observable
  behaviour rather than the (now-removed) guard.

Addresses zoroyihan7 review on PR AMD-AGI#1325.
…grid

Review nits on AMD-AGI#1325: the dropped reason is now self-describing
(cuda_graph_max_bs=32 plus CONC=64), extra_server_args=None pins the
`or ""` guard, and the single-node `kept is grid` identity is documented
as the production no-copy contract.
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

Addressed the three mutation holes and dropped the unreachable conc > 0 guard (regex (\d+) already guarantees conc >= 0; n < conc is false for every n >= 1 when conc == 0, so behaviour is unchanged). Also pinned .search vs .match with a non-leading flag, CONC-unset / CONC-empty / extra_server_args is None, and the cuda_graph_max_bs=32 half of the dropped reason. Left kept is grid as identity with a comment — that is the production single-node return grid, [] contract.

@zoroyihan7 zoroyihan7 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.

Re-reviewed. All three holes closed, and dropping the conc > 0 guard with the reasoning recorded in the test comment is the right call.

Five mutations, all red now:

  • .search(.match(red (the non-leading-flag variant does it)
  • os.environ.get("CONC", "64")"1"red
  • or 64or 1red
  • v.extra_server_args or ""v.extra_server_argsred
  • < conc<= concred (4 tests)

I also checked the guard removal is behaviour-preserving: the regex captures (\d+), so the parsed value is always >= 0 and n < conc is already False for any conc <= 0, negatives included. 8 passed. LGTM.

One heads-up: this PR now touches _grid_variant_filter.py, so it isn't test-only any more. I'd added skip-e2e-test when it was — removing it, since the workflow scopes that label to PRs that don't touch runtime code.

@zoroyihan7 zoroyihan7 removed the skip-e2e-test It's a PR that doesn't need to be e2e tested label Sep 1, 2026
@jiagaoxiang
jiagaoxiang enabled auto-merge (squash) September 1, 2026 03:35
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