Skip to content

[JIT] Check single-file tuned configs for duplicate shapes - #6365

Merged
yzhou103 merged 3 commits into
ROCm:mainfrom
yzhou103:config-dedup-single-file
Oct 14, 2026
Merged

yzhou103 merged 3 commits into
ROCm:mainfrom
yzhou103:config-dedup-single-file

Conversation

@yzhou103

Copy link
Copy Markdown
Contributor

Motivation

The duplicate-shape check in update_config_files only ran when a family had
more than one table to merge. Two short-circuits kept single-table families out
of it entirely:

  • get_config_file returns the canonical aiter/configs/<name>.csv untouched
    when model_configs/ has no matching sibling, so update_config_files is
    never called.
  • update_config_files returns early on a one-element path list.

A family that never grows a per-model table was therefore never validated. A
lone CSV can still hold two rows with the same key, which is what happens when
a retune is appended instead of replacing the existing row.

aiter/configs/tuned_grouped_fmoe.csv was in exactly that state: two rows for
gfx1250 / token=512 / model_dim=7168 / inter_dim=3072 differing only in tuning
parameters. The row was removed in #6332, but the mechanism that let it through
was not changed, so test_config_shape_collision.py::test_grouped_fmoe stayed
green throughout — it drives a resolution path that never reached the check.

6 of the 18 registered families currently resolve through this unchecked path

Technical Details

Extract the check into _dedup_or_raise and call it from both the merge path
and the single-file path, so the key derivation, the error text, and the
keep-lowest-us write-back stay in one place. env_name is threaded through,
so the newly covered paths still name the AITER_CONFIG_* variable in their
error (#6307).

Behavior

The only intended change is that a duplicate in a single-table family is now
reported instead of silently accepted. Verified by running the same ten
scenarios against main and this branch and diffing the returned path, the
merged output, the exception type, the error text, and the source write-back:

Scenario Result
Multi-file: clean / duplicate / no untuned schema Byte-identical
Single file: clean / missing / header-only / no untuned schema / unparseable Byte-identical
Single file: duplicate passed through → RuntimeError
Single file: duplicate, no us column passed through → RuntimeError

Note for deployments: a single-table family whose CSV already contains a
duplicate will now raise from get_config_file where it previously did not.
After #6332 no in-tree family is in that state.

_Mxfp8Config.update_config_files keeps its own single-path short-circuit —
that op validates individual rows in its loader — so its behavior is unchanged.

Test Plan

The existing self-checks only drove the cross-file merge: _build_synthetic_family
always wrote a model_configs/ sibling. Since removing the duplicate row makes
test_grouped_fmoe green again without proving the check runs, the new controls
use synthetic trees and do not depend on repository data:

  • a duplicate planted in a lone CSV must be reported
  • two distinct shapes in a lone CSV must not be
  • an unparseable lone CSV must not raise out of get_config_file; this path
    previously handed the file back unread, so the parse error surfaced from the
    op's own loader, and reading it for the check must not move that failure
    earlier or change its type

_build_synthetic_family takes with_model_file to select the path and
canonical_raw for malformed input. _run_synthetic now catches Exception
rather than RuntimeError, because pandas' ParserError is a ValueError and
would otherwise escape as a test error instead of a readable assertion.

Confirmed by mutation testing that these fail for the right reason:

Mutation Result
Revert the single-file check Only the planted-duplicate control fails
Keep the check, drop the parse guard Only the unparseable-CSV control fails

test_config_shape_collision.py 25 passed; 224 passed across
test_mixed_mxfp_tuning, test_tuner_infra, test_csv_validation, and
test_gemm_group32_interface

Test Result

Submission Checklist

@yzhou103
yzhou103 requested review from a team and a balanced review from Copilot October 10, 2026 07:01
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:gfx1250-ffm-triton Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x Run an additional Triton test job on MI300X in PRs (added automatically when gfx942 configs change); main branch always runs both MI35X and MI300X
ci:triton-355 Run the full Triton test suite on MI35X, not only the tests the change affects
ci:multi-gpu Aiter multi-GPU tests on the 8-GPU runner
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: DeepSeek-V4-Pro, MiniMax-M3-MXFP4
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 6365 --add-label <label>

One backend per PR:
A PR changes one kernel backend: [Triton/Gluon] (Triton and Gluon count as one), [HIP], [ASM], [CK], [OPUS] or [FlyDSL]. If the title ends up with two backend tags, split the PR -- as stacked pull requests when one part cannot merge without the other.

PR title tags & labels:
Component tags ([Triton/Gluon], [HIP], [CK], [ASM], ...) are added to the PR title and as PR labels automatically from the changed files and re-synced on every push — change-type tags like [fix]/[Perf], op tags like [MLA], and human labels (ci:*) are left untouched. Add the no-auto-title label to stop the title rewrites; labels stay in sync either way.

@github-actions github-actions Bot changed the title [Config] Check single-file tuned configs for duplicate shapes [JIT] Check single-file tuned configs for duplicate shapes Oct 10, 2026
@github-actions github-actions Bot added the JIT label Oct 10, 2026

Copilot AI 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.

🟡 Changes recommended

Read-only config sources can raise PermissionError, hiding the intended duplicate diagnostic and recovery guidance.

1 open finding
What changed in this PR

Extends duplicate-shape validation to single-file tuned configuration families.

Changes:

  • Extracts shared duplicate detection and write-back logic.
  • Validates canonical CSVs without model-specific siblings.
  • Adds synthetic single-file collision and malformed-CSV tests.
File Description
aiter/​jit/​core.py Adds shared deduplication and single-file validation.
op_tests/​tuning_tests/​test_config_shape_collision.py Covers clean, duplicate, and malformed single-file configurations.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread aiter/jit/core.py Resolved
The duplicate-shape check only ran when a family had more than one table
to merge. Two short-circuits kept single-file families out of it
entirely: get_config_file returns the canonical CSV untouched when
model_configs/ has no sibling, and update_config_files returns early on
a one-element path list. A family that never grows a per-model table was
therefore never validated, and a lone CSV can still hold two rows with
the same key when a retune is appended instead of replacing the old row.

aiter/configs/tuned_grouped_fmoe.csv was in exactly that state: two rows
for gfx1250 token=512 model_dim=7168 inter_dim=3072 differing only in
tuning parameters. The row was removed in ROCm#6332, but the mechanism that
let it through was not, and test_grouped_fmoe stayed green throughout
because the resolution path it drives never reached the check.

Extract the check into _raise_on_duplicate_shapes and call it from both
paths, so the key derivation, the error text, and the keep-lowest-us
write-back stay in one place. env_name is threaded through, so the paths
reached for the first time still name the AITER_CONFIG_* variable.

Only the single-file duplicate case changes behavior: it is now reported
instead of silently accepted. Multi-file merges, and single files that
are clean, missing, header-only or unparseable, resolve as before.

_Mxfp8Config.update_config_files keeps its own single-path short-circuit;
that op validates individual rows in its loader, so its behavior does not
change.

Signed-off-by: yzhou103 <yzhou103@amd.com>
The self-checks only drove the cross-file merge: _build_synthetic_family
always wrote a model_configs/ sibling, so the path where a family has no
sibling and resolves to its canonical CSV directly was never exercised.
That is the path that skipped the duplicate check, which is how
tuned_grouped_fmoe.csv kept a duplicate row (removed in ROCm#6332) while
every test here was green.

Removing the row makes test_grouped_fmoe green again without proving the
check runs, so add controls that do not depend on repository data:

- a duplicate planted in a lone CSV must be reported
- two distinct shapes in a lone CSV must not be
- an unparseable lone CSV must not raise out of get_config_file, since
  this path used to hand the file back unread and the parse error
  surfaced from the op's own loader

_build_synthetic_family takes with_model_file to choose the path and
canonical_raw for malformed input. _run_synthetic now catches Exception
rather than RuntimeError: pandas' ParserError is a ValueError, so the
malformed case has to be observable rather than escaping as a test error.

Verified by mutation: reverting the single-file check fails only the
first, and dropping the parse guard fails only the third.

Signed-off-by: yzhou103 <yzhou103@amd.com>
@yzhou103
yzhou103 force-pushed the config-dedup-single-file branch from c1ec897 to 1636e97 Compare October 10, 2026 07:26
Copilot AI balanced review requested due to automatic review settings October 10, 2026 07:26

Copilot AI 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.

🟢 Approval recommended

The shared validation preserves merge behavior and directly tests the newly covered resolution path.

1 open finding

🧠 Review effort: Balanced

The dedup write-back runs before the RuntimeError that reports the
duplicates, so a source that cannot be written replaces that diagnostic
with a bare PermissionError. That drops the dedup key and the
AITER_CONFIG_* hint precisely in a read-only install or container, which
is the environment those were added for (ROCm#6307, ROCm#5184), and
abandons any remaining source half-rewritten.

Catch OSError per source, collect the failures, and report them under
"Not updated" without counting them as updated. The resolution line now
says not every file could be rewritten when that happened, so the message
cannot claim a write that did not land.

This affected merges before and now also affects lone config files, since
those reach the same write-back.

Reported by Copilot review.

Signed-off-by: yzhou103 <yzhou103@amd.com>
Copilot AI balanced review requested due to automatic review settings October 10, 2026 09:41

Copilot AI 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.

🟡 Changes recommended

Unreadable untuned schemas can introduce new failures, and failed rewrites produce a misleading success message.

1 open finding
1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Handle unreadable untuned schemas like missing files

aiter/​jit/​core.py:465

The single-file parse guard does not cover this untuned-schema read. With a valid lone tuned CSV but an unreadable or malformed matching untuned CSV, get_config_file now raises OSError/ParserError; before this change it returned the tuned path without touching the untuned file. Treat an unreadable untuned schema like a missing one so the duplicate check does not introduce a new runtime failure.

🧠 Review effort: Balanced

Comment thread aiter/jit/core.py Resolved
@yzhou103
yzhou103 merged commit 745c9cb into ROCm:main Oct 14, 2026
zhiding512 added a commit that referenced this pull request Oct 14, 2026
Conflicts:

- aiter/aot/flydsl/mxfp4_moe.py: kept. The MXFP4 tuner's --fast-scan
  (#6405) precompiles its candidates with jobs_from_rows and
  compile_job_batch. It stays out of the AOT build, which replays these rows
  through fused_moe; its docstring and the README say so.
- aiter/jit/core.py: #6365 moved the duplicate-shape check into
  _raise_on_duplicate_shapes, shared with the single-file path. Comparing
  key columns as numbers, so 256 and 256.0 stay one shape, and ordering on a
  numeric `us` move there with it, and the single file is read as text like
  the merged ones, so a dedup rewrite keeps every cell as written.

Signed-off-by: zhimding <zhimding@amd.com>
yzhou103 added a commit to yzhou103/aiter that referenced this pull request Oct 15, 2026
…m#6365)

- Document that _raise_on_duplicate_shapes now runs on both single-file
  and multi-file paths (previously only cross-file merges were checked)
- Note graceful OSError handling for read-only containers
- Add _raise_on_duplicate_shapes to the references section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants