Repository navigation
[Config] remove duplicated csv row - #6332
Merged
Merged
Conversation
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
One backend per PR: PR title tags & labels: |
github-actions
Bot
changed the title
gyohuangxin
approved these changes
Oct 9, 2026
1 task
yzhou103
added a commit
to yzhou103/aiter
that referenced
this pull request
Oct 10, 2026
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>
yzhou103
added a commit
to yzhou103/aiter
that referenced
this pull request
Oct 10, 2026
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
added a commit
that referenced
this pull request
Oct 14, 2026
* [Config] Check single-file tuned configs for duplicate shapes 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 #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> * [Config] Cover the single-file path in the collision self-checks 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 #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> * [Config] Keep the duplicate diagnostic when a source cannot be rewritten 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 (#6307, #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> --------- Signed-off-by: yzhou103 <yzhou103@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Technical Details
Test Plan
Test Result
Submission Checklist