Skip to content

[CK] [FlyDSL] [CI] [Bugfix] Explicit gfx in shipped fused-MoE tuning CSVs - #5315

Open
amd-bartgips wants to merge 12 commits into
mainfrom
users/bartgips/silotiger-1026-fmoe-explicit-gfx
Open

amd-bartgips wants to merge 12 commits into
mainfrom
users/bartgips/silotiger-1026-fmoe-explicit-gfx

Conversation

@amd-bartgips

@amd-bartgips amd-bartgips commented Sep 7, 2026 •

Copy link
Copy Markdown

Update (2026-09-17)

This branch was merged with current main (e44c1d0bc) so the PR is reviewable again. Head: a17eac67e.

The catalogue claims are unchanged versus today's main: still 16 migrated files / 1,816 rows, field-for-field identical once gfx is stripped. Three of those tables (dsv3_fp4, kimik2_fp4, qwen3_8_2400b_fp4) were retuned on main in the meantime; this merge kept those rows and only prepended gfx950. The inventory of fused-MoE tables that already carried gfx is now 18 (was 16 when this description was first written). The 16-CSV migration set is unchanged.

Why

Selection is keyed on (gfx, cu_num), but 16 shipped fused-MoE tables carry no gfx, so architecture is guessed from CU count on load. gfx950 and gfx1250 both report 256 CUs, so 827 shipped 256-CU rows cannot be selected as a distinct gfx1250 catalogue: the loader would stamp them gfx950.

This PR records the architecture on every shipped row. It does not change kernel choice, measured time, or lookup fallback. Once the added gfx field is stripped, every migrated data row is field-for-field identical to main.

What changed

  1. Shipped tables. Prepend gfx on the 16 legacy fused-MoE CSVs (canonical + model overlays). Eighteen other fused-MoE tables already had explicit gfx and were left untouched.
  2. Legacy load and merge. Files that still omit gfx, or that use placeholder cells ("", "0", numeric 0/0.0, NaN, "nan", "None"), are backfilled from the historical CU map only (80/304 → gfx942, 256 → gfx950) and warn once per file. Unknown CU counts raise instead of labeling the row with the live GPU. Family merge (update_config_files) runs the same backfill before gfx-aware dedup, so a placeholder gfx=0 row and the equivalent explicit gfx950 row collide as one key and keep lowest us rather than both surviving until load time.
  3. AOT job architecture. FlyDSL AOT uses the same rule via resolve_job_arch: explicit gfx wins; placeholders fall through to the CU map; unknown values raise. That helper is used by fused-MoE AOT and by GEMM / GDN / grouped MoE / FHMoE so the same CU-count collision cannot compile those jobs as gfx950 by accident. This is the same defect, not a default-kernel swap.
  4. What the 16-CSV sweep does and does not cover. The historical CU map stays {80, 304, 256}. gfx1250 also reports 96 CUs; those rows already live on GEMM tables (bf16_tuned_gemm and model overlays) with explicit gfx=gfx1250, so 96 is not added to the infer map. Every other FlyDSL AOT family CSV (GEMM, GDN opt, grouped MoE, FHMoE, and the fused-MoE tables that already had gfx) already carries a real gfx on every data row. Load, merge, and AOT share one CU map and one is_missing_gfx helper. test_fmoe_gfx_schema walks those AOT-family tuned files through resolve_job_arch so a later table that omits gfx on an unusual CU count fails that suite instead of aborting AOT at build time. That module is in the scheduled tuning-tests.yaml level 0+1 job; standard PR CI does not collect op_tests/tuning_tests/.

Why the diff looks large

Review-size note: 16 CSV files account for most of the line count. Adding one field at the start of a CSV row makes Git display the entire row as removed and re-added. The mechanical data migration is about 92% of the changed lines.

How the existing rows were labelled

Architecture was taken from the history of each table, not guessed at migration time:

  • gfx942, 80 CUs: 942 rows
  • gfx942, 304 CUs: 47 rows
  • gfx950, 256 CUs: 827 rows

All 1,816 data rows are byte-identical to main after removing the new gfx value. Row order, kernel names, timings, errors, and tags are unchanged. No row had an unresolved architecture.

Per-file migration counts
  • configs/tuned_fmoe.csv: 617 rows — gfx942 / 80 CUs
  • a8w8_blockscale_tuned_fmoe_ds_v3.csv: 395 rows — 325 gfx942 / 80 CUs, 15 gfx942 / 304 CUs, 55 gfx950 / 256 CUs
  • a8w8_blockscale_tuned_fmoe_glm5.csv: 16 rows — gfx950 / 256 CUs
  • a8w8_blockscale_tuned_fmoe_minimax-m2_5.csv: 64 rows — gfx950 / 256 CUs
  • a8w8_blockscale_tuned_fmoe_qwen3_235b.csv: 28 rows — gfx950 / 256 CUs
  • a8w8_blockscale_tuned_fmoe_qwen3_5_397b.csv: 16 rows — gfx950 / 256 CUs
  • dsv3_fp4_tuned_fmoe.csv: 95 rows — gfx950 / 256 CUs
  • glm47_fp8_tuned_fmoe.csv: 68 rows — gfx950 / 256 CUs
  • gptoss_fp4_tuned_fmoe.csv: 24 rows — gfx950 / 256 CUs
  • kimik2_fp4_tuned_fmoe.csv: 80 rows — gfx950 / 256 CUs
  • kimik2_fp8fp4_tuned_fmoe.csv: 63 rows — gfx950 / 256 CUs
  • kimik2_i4_tuned_fmoe.csv: 88 rows — gfx950 / 256 CUs
  • minimax_m25_fp4_tuned_fmoe.csv: 64 rows — gfx950 / 256 CUs
  • qwen3_235b_bf16_tuned_fmoe.csv: 48 rows — 32 gfx942 / 304 CUs, 16 gfx950 / 256 CUs
  • qwen3_8_2400b_fp4_tuned_fmoe.csv: 32 rows — gfx950 / 256 CUs
  • qwen3next_80b_fp8_tuned_fmoe.csv: 118 rows — gfx950 / 256 CUs

This PR does not invent MI350P rows. A full-device MI350P entry should be written explicitly as gfx950 with 128 CUs.

Validation

This is a selection-key / catalogue change, not a new kernel and not a default-op swap. There is no kernel latency claim to re-measure; the invariant to check is that timings in the CSVs did not move.

Family merge is the real runtime path (aiter/configs/<name>.csv plus every model_configs/*<name>*.csv). Please run the collision guard against that merge, not against a single file:

python3 -m unittest \
  op_tests.tuning_tests.test_config_shape_collision \
  op_tests.tuning_tests.test_fmoe_gfx_schema \
  op_tests.tuning_tests.test_csv_validation \
  op_tests.tuning_tests.test_online_tune -v

Those four test_*.py files are why an automated single-target kernel validator cannot pick a unique --target here. They are a local / scheduled CPU guard, not a per-PR job: standard PR CI collects op_tests/*.py at depth 1, so nothing under op_tests/tuning_tests/ runs there. test_fmoe_gfx_schema is included in the scheduled tuning-tests.yaml level 0+1 invocation. Evidence is that CPU suite (schema, placeholder handling including pandas 0.0, unknown-CU raise, gfx950+gfx1250 256-CU coexistence on the real merge path, AOT-family CSV sweep including 96-CU gfx1250 GEMM rows, merge-time placeholder vs explicit-gfx collision) plus one --run_config smoke of a migrated glm5 row on MI355X/gfx950.

.claude/skills/aiter-config-shape and .claude/skills/review-pr apply; validate-kernel-pr does not have a unique kernel route for this diff and should be treated as N/A rather than a missing PASS.

The 80/304/256 maps are the historical CU backfill, not a new if arch == ... in fused-MoE dispatch. Unknown CU counts raise (including 96). Load-path, merge, and AOT share one placeholder helper and one CU map so gfx=0 / 0.0 cannot become FLYDSL_GPU_ARCH=0.

Downstream CI

Serving stacks consume fused_moe lookup. Kernels and numeric results are unchanged; only the catalogue key is explicit. ci:sglang and ci:vllm are requested so lookup still hits on those paths. Those jobs, and the standard Aiter GPU tests, run only after the PR leaves draft. This is not a FlyDSL/Triton kernel upgrade, so ci:atom_full is not requested.

Related work

  • #5182 improves device identity and MI350P naming, but does not change fused-MoE tables or lookup.
  • #5340 (AITER_GPU_TARGETS) filters tuned rows at build time on (gfx, cu_num); that filter cannot distinguish a 256-CU gfx1250 fused-MoE catalogue from gfx950 until those rows carry explicit gfx, so this PR should land first.
  • #5342 then filters FlyDSL GEMM AOT jobs by those build targets, and currently has to skip the other four OpKinds because they record only half a key (('', 256) for MoE); this PR fills in gfx on those jobs so that filter can be extended without inventing an architecture.

@github-actions

github-actions Bot commented Sep 7, 2026

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; main branch always runs both MI35X and MI300X
multigpu 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: GPT-OSS-120B, DeepSeek-R1-0528, Kimi-K2.5
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 5315 --add-label <label>

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 opt this PR out.

@amd-bartgips
amd-bartgips force-pushed the users/bartgips/silotiger-1026-fmoe-explicit-gfx branch 4 times, most recently from 2c5ed21 to 9613831 Compare September 7, 2026 14:51
@amd-bartgips
amd-bartgips force-pushed the users/bartgips/silotiger-1026-fmoe-explicit-gfx branch from 434257f to 9e94ae0 Compare September 8, 2026 08:39
amd-bartgips and others added 7 commits September 8, 2026 08:43
Fused-MoE selection is keyed on (gfx, cu_num), but 16 shipped tables
(1,816 rows) carried no gfx column, so each row's architecture was
reconstructed on load from a CU-count heuristic. That heuristic cannot
be correct in general: gfx950 and gfx1250 both report 256 CUs.

Add a leading gfx column derived from each table's tuning provenance
(80/304 -> gfx942, 256 -> gfx950). Every row is otherwise byte-identical;
tables that already carried explicit gfx are unchanged. Add artifact and
cross-gfx merge regression coverage so two same-CU architectures survive
a merge while a true same-gfx duplicate is still rejected.

Co-authored-by: Cursor <cursoragent@cursor.com>
Centralize the legacy gfx backfill so lookup, DP-shared lookup, multi-file
merge, incremental tuning, and FlyDSL AOT share one policy and emit one
warning per loaded external file. An explicit row gfx always overrides
cu_num inference.

Carry each row's gfx into FlyDSL fused-MoE AOT job identity and
FLYDSL_GPU_ARCH so two architecture targets no longer deduplicate into a
single compile; the gfx950-only mxfp4 path rejects a row explicitly
labelled for another architecture. Update tuning docs and tests for the
gfx-first schema. Selection fallbacks are unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
…fx950

Rename job_arch to resolve_job_arch and keep only two legal sources: an
explicit row gfx, or a known legacy CU mapping. Missing or unknown values
now fail rather than compiling for the wrong architecture.

Co-authored-by: Cursor <cursoragent@cursor.com>
…and FHMoE

Stop defaulting missing gfx to gfx950 or gfx1250 in the remaining FlyDSL AOT
entry points. Compile targets now come from an explicit row gfx or a known
legacy CU mapping, and unknown values fail instead of compiling the wrong arch.

Co-authored-by: Cursor <cursoragent@cursor.com>
AOT was compiling gfx=0 as a real architecture, and CSV load labeled unknown CU counts with the live GPU. Both paths now fall through only to the known 80/304/256 maps or raise.

Co-authored-by: Cursor <cursoragent@cursor.com>
Placeholder gfx cells must mean the same thing on the load path and in resolve_job_arch, otherwise AOT can compile gfx=0 as a real architecture.

Co-authored-by: Cursor <cursoragent@cursor.com>
… CSV to resolve_job_arch

gfx1250 reports 96 CUs as well as 256; those shipped rows already carry gfx, so adding 96 to the historical CU map would guess an arch the tuner already wrote. Share one map between load and AOT and sweep every AOT-family tuned CSV so a future table without gfx cannot hard-abort on an unusual CU count unnoticed.

Co-authored-by: Cursor <cursoragent@cursor.com>
@amd-bartgips
amd-bartgips force-pushed the users/bartgips/silotiger-1026-fmoe-explicit-gfx branch from 9e94ae0 to e22c59d Compare September 8, 2026 08:44
@samremes
samremes requested a balanced review from Copilot September 8, 2026 08:56

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

Numeric zero placeholders can escape backfilling, and the schema test parses CU values differently from production AOT parsers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds explicit GPU architecture keys to fused-MoE tuning data and aligns runtime/AOT legacy resolution.

Changes:

  • Migrates 16 fused-MoE CSVs to explicit gfx values.
  • Centralizes legacy CU-to-architecture backfilling.
  • Adds schema, merge, lookup, and AOT regression tests.
File summaries
File Description
aiter/aot/flydsl/README.md Documents AOT architecture resolution.
aiter/aot/flydsl/chunk_gdn_h.py Uses explicit job architecture.
aiter/aot/flydsl/common.py Adds shared architecture resolver.
aiter/aot/flydsl/fhmoe.py Resolves and validates FHMoE architecture.
aiter/aot/flydsl/gemm.py Applies shared resolver to GEMM AOT.
aiter/aot/flydsl/grouped_moe.py Threads target identity into grouped MoE jobs.
aiter/aot/flydsl/moe.py Propagates gfx through MoE AOT.
aiter/aot/flydsl/mxfp4_moe.py Validates gfx950-only AOT jobs.
aiter/configs/model_configs/a8w8_blockscale_tuned_fmoe_ds_v3.csv Adds explicit row architectures.
aiter/configs/model_configs/a8w8_blockscale_tuned_fmoe_glm5.csv Adds explicit row architectures.
aiter/configs/model_configs/a8w8_blockscale_tuned_fmoe_minimax-m2_5.csv Adds explicit row architectures.
aiter/configs/model_configs/a8w8_blockscale_tuned_fmoe_qwen3_235b.csv Adds explicit row architectures.
aiter/configs/model_configs/a8w8_blockscale_tuned_fmoe_qwen3_5_397b.csv Adds explicit row architectures.
aiter/configs/model_configs/dsv3_fp4_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/glm47_fp8_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/gptoss_fp4_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/kimik2_fp4_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/kimik2_fp8fp4_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/kimik2_i4_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/minimax_m25_fp4_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/qwen3_235b_bf16_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/qwen3_8_2400b_fp4_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/model_configs/qwen3next_80b_fp8_tuned_fmoe.csv Adds explicit row architectures.
aiter/configs/tuned_fmoe.csv Adds explicit gfx942 architecture keys.
aiter/fused_moe.py Centralizes legacy table backfilling.
aiter/fused_moe_dp_shared_expert.py Uses shared backfill logic.
aiter/jit/core.py Backfills legacy rows during configuration merging.
aiter/jit/utils/chip_info.py Implements strict legacy architecture inference.
aiter/jit/utils/gfx_placeholders.py Defines shared placeholders and CU mapping.
csrc/ck_gemm_moe_2stages_codegen/README.md Documents the new tuning schema.
csrc/ck_gemm_moe_2stages_codegen/gemm_moe_tune.py Migrates legacy tuner inputs.
op_tests/tuning_tests/README.md Documents added regression coverage.
op_tests/tuning_tests/test_config_shape_collision.py Tests cross-architecture coexistence.
op_tests/tuning_tests/test_csv_validation.py Validates explicit shipped gfx values.
op_tests/tuning_tests/test_fmoe_gfx_schema.py Tests backfill and AOT resolution.
op_tests/tuning_tests/test_online_tune.py Updates lookup fixtures for gfx.
op_tests/tuning_tests/test_tune_pipeline.py Adds gfx to pipeline keys.
Review details
  • Files reviewed: 35/37 changed files
  • Comments generated: 2
  • Review effort level: 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/utils/chip_info.py Outdated
Comment thread op_tests/tuning_tests/test_fmoe_gfx_schema.py Outdated

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

Agent review: aiter PR #5315 — Explicit fused-MoE architecture

Reviewed: #5315
Title: [Bugfix] Explicit gfx in shipped fused-MoE tuning CSVs
Local worktree: /home/samremes/dev/aiter-pr-5315-review
Base: origin/main @ e8eac17259db6efcb48eba2aa0d9a3bf422e7b35
Head: e22c59dffc010d3d4d5533fe64b68ce0c8528edb
Dirty files in this worktree: none
Review date: 2026-09-08
Skills applied: FlyDSL kernel testing, AITER operation-test guidance

This document is a read-only review. I ran 59 targeted CPU unit tests. All tests passed. I also verified all 1,816 migrated rows in 16 CSV files. Each new row contains only the added gfx field, and every old field is identical. The recorded kernel names, tags, and us values are unchanged. I ran no GPU test, benchmark, or FlyDSL AOT compile.

Verdict

Request changes before merge. The runtime lookup and CSV migration are consistent with the stated architecture policy. Two AOT error paths still need correction. One path can silently discard an explicit incompatible architecture row. The other path retries deterministic architecture errors as worker crashes.

Review context

Item Value
Change class AITER integration, dispatch, AOT, and tuning-catalog migration
Targets gfx942, gfx950, and gfx1250; no kernel or wave-size change
Dtypes Existing fused-MoE and GEMM catalog dtypes; no numerical operation change
Declared FlyDSL pin flydsl==0.3.2
Imported FlyDSL 0.3.2 from /opt/venv/lib/python3.14/site-packages/flydsl
Diff vs supplied local base 179 files, +11,047 / -3,538
Pull-request scope 37 files, +2,340 / -1,940

The supplied merge-base diff contains changes that are already on the current upstream branch. The pull request contains 37 files. This review covers those 37 files only.

Changed files:

  • aiter/aot/flydsl/ (shared architecture resolution and AOT callers)
  • aiter/fused_moe.py and aiter/fused_moe_dp_shared_expert.py (runtime lookup)
  • aiter/jit/core.py and aiter/jit/utils/ (CSV merge and legacy backfill)
  • csrc/ck_gemm_moe_2stages_codegen/ (tuner output schema)
  • aiter/configs/ (16 migrated fused-MoE tuning tables)
  • op_tests/tuning_tests/ (schema, merge, lookup, and AOT tests)

The migrated rows preserve the old inferred architecture mapping. Rows with 80 or 304 CUs now contain gfx942. Rows with 256 CUs now contain gfx950. Runtime lookup now uses (gfx, cu_num, ...), so these migrated rows select the same kernel names as before on the historical targets. The static check proves that the stored timings are identical. It does not prove runtime latency.

Findings

Findings are ordered by severity. Each finding has a location, a trigger, a reason, and a proposed fix.

P2 — MXFP4 AOT deduplication drops architecture before validation

Where: aiter/aot/flydsl/mxfp4_moe.py lines 48-91, _job_key (the custom AOT identity); lines 109-114, _add (deduplication before job insertion); lines 387-394, compile_one_config (architecture validation after deduplication)

Trigger: One input CSV contains otherwise identical MXFP4 jobs for gfx950 and another architecture with the same CU count. This case is possible for 256-CU rows. The result depends on row order.

Reason: _job_key does not include gfx. _add removes the second row before compile_one_config calls resolve_job_arch. If the valid gfx950 row is first, the incompatible row disappears and the promised gfx950-only validation does not run. If the incompatible row is first, the valid row disappears and AOT fails. A direct check at the reviewed revision confirms that two stage-1 jobs that differ only between gfx950 and gfx1250 produce the same _job_key.

Proposed fix: Make every explicit architecture row reach validation. One sufficient approach is to include resolved gfx in the parser deduplication key. Another sufficient approach is to resolve and validate each row before _add, then deduplicate only valid gfx950 jobs.

P2 — Deterministic architecture errors enter the transient worker retry path

Where: aiter/aot/flydsl/common.py lines 193-200, _run_one_to_file (uncaught worker call); lines 316-339, retry_or_drop and reap (all nonzero exits are retried as crashes); aiter/aot/flydsl/moe.py line 1022, compile_one_config; aiter/aot/flydsl/gemm.py line 663, compile_one_config; aiter/aot/flydsl/chunk_gdn_h.py line 307, compile_one_config; aiter/aot/flydsl/mxfp4_moe.py lines 387-390, compile_one_config

Trigger: A central run_aot build reads a row with missing architecture data, an unknown CU count, or a placeholder gfx with an unknown CU count.

Reason: The changed callers invoke resolve_job_arch before their local compile try blocks. Its intended ValueError escapes the child process. _run_file_pool sees a nonzero exit and classifies the deterministic data error as a transient worker crash. It launches the same invalid job up to three times with the default retry count. The final summary reports a worker death or timeout instead of preserving the architecture-resolution error. This behavior makes strict validation more expensive and less actionable.

Proposed fix: Preserve deterministic resolver failures as normal failed-job results. One sufficient approach is to resolve all job architectures before process submission. Another sufficient approach is to catch ValueError in each worker and return compile_time=None with the original diagnostic. Keep retries for abnormal exits such as a signal, an out-of-memory kill, or a timeout.

P3 — The placeholder test pins module-object identity

Where: op_tests/tuning_tests/test_fmoe_gfx_schema.py lines 78-86, test_load_and_aot_share_placeholder_set (identity assertions); aiter/jit/utils/gfx_placeholders.py lines 29-31 (dual import-name registration)

Trigger: A future refactor loads equivalent immutable placeholder values through two normal module paths without preserving the same Python object identity.

Reason: The operation contract requires equal placeholder values and equal CU mappings. It does not require both imports to return the same frozenset or dictionary object. assertIs couples the test to the sys.modules alias implementation. An equivalent implementation can fail this test without changing CSV load or AOT behavior.

Proposed fix: Test value equality and resolver behavior. Keep one test that sends each placeholder through both the load path and the AOT path. Do not make object identity an executable compatibility requirement.

Verification gaps

  • I did not run fused-MoE dispatch on gfx942, gfx950, or gfx1250.
  • I did not compile or load an AOT cache for any architecture.
  • I did not run an end-to-end run-only or cache-hit test.
  • I did not measure runtime latency. The CSV comparison proves only that stored us values and kernel selections in the migrated rows are unchanged.
  • I did not test external tuning files from downstream users. The CPU tests cover synthetic legacy files, placeholders, unknown CU counts, same-CU cross-architecture merge behavior, and the shipped catalogs.

@amd-bartgips

Copy link
Copy Markdown
Author

Thanks @samremes — I treated the two P2s as real and the P3 as a test smell.

MXFP4 dedup: _job_key now includes gfx, so a same-shape gfx950 row and a gfx1250 row both reach compile_one_config. The gfx950-only check still fails the incompatible job (compile_time=None) instead of dropping one of them by insertion order.

Worker retries: resolve_job_arch now runs inside the existing compile try on the MoE/GEMM/GDN/mxfp4 workers, and _run_one_to_file catches leftover ValueErrors so a missing/unknown architecture is a failed job (exit 0), not a crashed worker that gets retried as OOM.

Placeholder identity: the schema test now checks value equality and that each placeholder cell resolves the same on load and AOT. It no longer requires the sys.modules alias to return the same object.

Pushed in 418f935. Copilot's pandas-0.0 hole and the int(float(cu_num)) sweep mismatch are in the same commit.

MXFP4 job identity omitted gfx, so a same-shape gfx1250 row could drop the gfx950 job (or vice versa) before the gfx950-only check. resolve_job_arch also ran outside the compile try, so a missing gfx looked like a dead worker and was retried. Treat pandas 0.0 as a placeholder and stop pinning placeholder object identity in the schema test.

Co-authored-by: Cursor <cursoragent@cursor.com>
@amd-bartgips
amd-bartgips force-pushed the users/bartgips/silotiger-1026-fmoe-explicit-gfx branch from 418f935 to 277b3c4 Compare September 8, 2026 11:42
@samremes
samremes requested a balanced review from Copilot September 8, 2026 11:44

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

Placeholder normalization can bypass merge deduplication, numeric placeholders evade validation, and the new regression suite is not wired into CI.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

op_tests/tuning_tests/test_fmoe_gfx_schema.py:175

  • This regression suite is not executed by CI: .github/workflows/tuning-tests.yaml:96 hard-codes the level 0+1 modules and does not include test_fmoe_gfx_schema (nor does it derive the command from this README). Consequently, the promised CI guard for missing/ambiguous AOT architecture rows can regress while CI remains green. Add this module to the workflow's unittest invocation.
  • Files reviewed: 35/37 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread aiter/jit/core.py Outdated
Comment thread op_tests/tuning_tests/test_csv_validation.py Outdated
amd-bartgips and others added 2 commits September 8, 2026 11:58
Merge only backfilled a missing gfx column, so gfx=0 and gfx950 stayed distinct keys and the loader later kept first-seen instead of lowest us. Use is_missing_gfx in the shipped-CSV guard and run the schema suite in the scheduled level 0+1 job.

Co-authored-by: Cursor <cursoragent@cursor.com>
test_csv_validation is a torch-free level 0 module; importing is_missing_gfx inside the test made it error rather than skip where aiter is unavailable. Guard the import like the other tuning tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@amd-bartgips

Copy link
Copy Markdown
Author

Copilot's second pass is addressed in 9f5499e and b3cbdda; the two inline threads have replies with the details.

The third item had no inline thread (it was reported as a suppressed/previously-missed comment), so answering it here: test_fmoe_gfx_schema was indeed not executed by any workflow. .github/workflows/tuning-tests.yaml:96 hard-codes its module list and did not include it, so the guard this PR adds could have regressed while CI stayed green. It is now in that job's unittest invocation, and docs/autotuning_pipeline.md lists it alongside the other level 0+1 modules so the workflow and op_tests/tuning_tests/README.md agree.

One caveat worth stating plainly rather than leaving implied: that workflow is schedule + workflow_dispatch, not PR CI. Standard PR tests collect op_tests/*.py at depth 1, so nothing under op_tests/tuning_tests/ has ever run per-PR — that is pre-existing and not something this PR changes. Within the suite that does run on a schedule, the new module is now wired in.

Local run of the CPU suite on this head (b3cbddac), including the aiter-config-shape collision guard:

python3 -m unittest op_tests.tuning_tests.test_csv_validation \
  op_tests.tuning_tests.test_fmoe_gfx_schema \
  op_tests.tuning_tests.test_config_shape_collision \
  op_tests.tuning_tests.test_tuner_infra \
  op_tests.tuning_tests.test_mp_tuner_logic \
  op_tests.tuning_tests.test_online_tune
-> Ran 98 tests ... OK

samremes
samremes previously approved these changes Sep 8, 2026

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

Agent review: aiter PR #5315 — Explicit fused-MoE architecture

Reviewed: #5315
Title: [Bugfix] Explicit gfx in shipped fused-MoE tuning CSVs
Local worktree: /home/samremes/dev/aiter-pr-5315-review
Base: origin/main @ 3a7a22f8c58618c6862c195de8c4857d535f57c9
Head: 8ee14d9e11c10369f0991573c38664a3273e2d39
Dirty files in this worktree: none
Review date: 2026-09-08
Skills applied: FlyDSL kernel testing; AITER operation-test placement rules from the reviewer skill (this worktree has no aiter-op-test skill file)

This document is a read-only second-pass review. I ran 62 CPU unit tests from the worktree:

python3 -m unittest op_tests.tuning_tests.test_config_shape_collision op_tests.tuning_tests.test_fmoe_gfx_schema op_tests.tuning_tests.test_csv_validation op_tests.tuning_tests.test_online_tune -v

All 62 tests passed. I ran no GPU test, FlyDSL AOT compile, or cache-hit test.

Verdict

Merge is acceptable. The first-pass P2 and P3 defects are closed in the current head. The Copilot comments describe real defects on the then-current code, and the follow-up commits close them. Remaining risk is unverified GPU and AOT execution, not an open code defect.

Review context

Item Value
Change class AITER integration, dispatch, AOT, and tuning-catalog migration
Targets gfx942, gfx950, and gfx1250; no kernel or wave-size change
Dtypes Existing fused-MoE and GEMM catalog dtypes; no numerical operation change
Declared FlyDSL pin flydsl==0.3.2 in requirements.txt
Imported FlyDSL 0.3.2 from /opt/venv/lib/python3.14/site-packages/flydsl
Diff vs origin/main 39 files, +2498 / −1954
Merge-base with HEAD 2e682f2dd531d0577cc4233cd526335b4b23ae11
Prior review head e22c59dff (2026-09-08, request changes)

Changed files:

  • aiter/aot/flydsl/ (shared architecture resolution, worker error path, AOT callers)
  • aiter/fused_moe.py and aiter/fused_moe_dp_shared_expert.py (runtime lookup)
  • aiter/jit/core.py and aiter/jit/utils/ (CSV merge, placeholders, legacy backfill)
  • csrc/ck_gemm_moe_2stages_codegen/ (tuner output schema)
  • aiter/configs/ (migrated fused-MoE tuning tables)
  • op_tests/tuning_tests/ (schema, merge, lookup, and AOT tests)
  • .github/workflows/tuning-tests.yaml (level 0+1 collection of test_fmoe_gfx_schema)

Follow-up commits since the first pass: 277b3c4a4, 9f5499e6d, b3cbddacd, and merge of main as 8ee14d9e1.

First-pass findings at this head:

  • P2 MXFP4 _job_key omits gfx: fixed. _job_key includes gfx for stage-1, stage-2, and layout-v2 jobs. test_same_shape_jobs_for_two_gfx_are_not_deduped proves a same-shape gfx950 job and a gfx1250 job produce distinct keys.
  • P2 resolve_job_arch escapes the worker: fixed. Callers resolve inside the compile try (moe.py compile_one_config, gemm.py compile_one_config, chunk_gdn_h.py compile_one_config, mxfp4_moe.py compile_one_config, grouped_moe.py compile_one_config). _run_one_to_file also catches ValueError and writes compile_time=None. test_rejects_explicit_incompatible_architecture returns a failed job, not a process crash.
  • P3 assertIs on placeholder objects: fixed. test_load_and_aot_share_placeholder_set checks value equality and resolver behavior.
Copilot comment Relevant Current status
chip_info.py ~152: pandas 0.0/NaN gfx is not a placeholder yes fixed
test_fmoe_gfx_schema.py ~174: schema accepted 80.5/80.0 via float yes fixed
core.py ~342: placeholder gfx not normalized before merge dedup yes fixed
test_csv_validation.py ~141: shipped-gfx guard missed pandas "0.0" yes fixed
test_fmoe_gfx_schema.py not in tuning-tests.yaml level 0+1 yes fixed

Copilot dispositions:

  • Numeric zero: is_missing_gfx treats 0, 0.0, "0.0", and NaN as missing. backfill_dataframe_gfx promotes the column off float. test_numeric_zero_gfx_is_treated_as_placeholder covers the pandas float column.
  • Schema cu_num: the AOT-family sweep uses int(cu_raw) and records unparsable cells as failures.
  • Merge keys: update_config_files calls backfill_dataframe_gfx when gfx is in the union column set. test_placeholder_and_explicit_gfx_collide_as_one_key drives that merge and gets a duplicate-shape collision (lowest-us auto-resolve).
  • Shipped-gfx guard: test_shipped_fmoe_configs_have_explicit_gfx maps is_missing_gfx. The import skip is only when aiter is not importable; CI builds AITER before this suite.
  • CI collection: .github/workflows/tuning-tests.yaml level 0+1 now runs op_tests.tuning_tests.test_fmoe_gfx_schema. That workflow still runs on schedule and workflow_dispatch, not on pull_request. That trigger is the existing contract for this workflow.

Findings

No actionable findings remain.

Verification gaps

  • I did not run fused-MoE dispatch on gfx942, gfx950, or gfx1250.
  • I did not compile or load an AOT cache for any architecture.
  • I did not run an end-to-end run-only or cache-hit test.
  • I did not measure runtime latency.
  • I did not execute the scheduled GitHub Actions job. I only read that test_fmoe_gfx_schema is on the level 0+1 unittest command.
  • I did not test external tuning files from downstream users. The CPU tests cover synthetic placeholders, pandas 0.0, merge collision, unknown CU counts, same-CU cross-architecture coexistence, and the shipped catalogs.

@amd-bartgips

Copy link
Copy Markdown
Author

Thanks @samremes — the two P2s, the P3, and the Copilot follow-ups are closed in this head. Remaining GPU/AOT execution is the serving-lookup path (ci:sglang / ci:vllm), which starts once this leaves draft.

@amd-bartgips

Copy link
Copy Markdown
Author

@lalala-sh @valarLip (and other fused-MoE owners): this PR changes the fused-MoE catalogue key, not the kernel or the public op signature. Shipped tables now carry explicit gfx; lookup is already (gfx, cu_num, ...). Legacy files without gfx still load via the historical CU map (256→gfx950, 80/304→gfx942) and warn; unknown CU counts raise instead of following the live GPU.

I would like an explicit ack before merge because fused_moe.py / jit/core.py are downstream contracts. Serving lookup is the interesting path, so ci:sglang and ci:vllm are requested rather than ci:all / ci:atom_full (kernels are unchanged). Happy to widen CI if you want it.

CSV rows are byte-identical to main once the new gfx field is stripped. The collision guard (test_config_shape_collision, including gfx950+gfx1250 256-CU coexistence) is the merge check from .claude/skills/aiter-config-shape.

@amd-bartgips
amd-bartgips marked this pull request as ready for review September 8, 2026 12:37
@amd-bartgips
amd-bartgips requested a review from a team September 8, 2026 12:37
@github-actions github-actions Bot changed the title [Bugfix] Explicit gfx in shipped fused-MoE tuning CSVs [CK] [FlyDSL] [CI] [Bugfix] Explicit gfx in shipped fused-MoE tuning CSVs Sep 8, 2026
@zufayu
zufayu requested a review from yadaish September 9, 2026 01:24
Keep main's retuned fused-MoE kernels and AOT job fields, and keep explicit
gfx on the three tables that main rewrote without the column.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

No activity for 15 days, so this is now labelled stale. A push or a comment clears it; keep-open exempts it. Nothing is closed automatically.

@github-actions github-actions Bot added the stale The PR hasn't been updated in 2 weeks label Oct 7, 2026

This branch has not been deployed

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

Labels

ci:sglang ci:vllm CI CK FlyDSL JIT stale The PR hasn't been updated in 2 weeks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants