fix(glm53): split-K head gate 리뷰 15건 처리 — N=32 형상, 결정적 v2 커널, 검사기·도구·문서 정정 - #231
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…·문서 정정
리뷰 결과 모듈이 플릿에서 한 번도 돌지 않았다: 체크포인트 index_n_heads=32 인데
헬퍼는 N<=16 만 받고 BN=16 고정, 수치 전부 합성 N=16. v2 커널로 교체:
- K 를 32 프로그램으로 분할, 가중치 슬라이스 1회 읽기, bf16 레지스터 캐스트,
[16,N] 부분합 → 두 번째 커널이 고정 순서 합산. atomic·memset 없음, 리플레이·
랭크 간 비트 동일 (첫 판 atomic 은 49/49 리플레이가 달랐다). 발사 2개.
- 헬퍼가 x 2-D·unit stride·K 일치·N<=32 를 검사, 라우팅을 형상별 1회 로그
("[indexer-gate] ... -> split-K" / "shape not admitted").
- N=32 콜드: M=8 89.5 → 12.7 us, M=16 87.7 → 16.8; M=24 부터 stock 17 us 유지.
천장 11 × 76.8 us = 0.84 ms = C=1 의 ~1.2%. 수치 2.4e-6, 순위 뒤집힘 0.
- fastpath: ModuleNotFoundError(해당 모듈)만 조용히, 나머지 ImportError 는 로그,
노브 켜졌는데 모듈 없으면 경고. requires 비움(의존 방향 역전 정정).
- probes/indexer_gate_check.py: 실제 라우팅·체크포인트 형상(--config)·결정성·
stride/K 불일치/M=0·콜드 L2 M=1..32 교차점, 실패 시 비-0 종료.
- probes/moe_gate_tile_sweep.py: 배포 커널 import(사본 제거), BM32 stages 3
사다리, E=288 기본, M<=32 가드.
- tests: 심볼 검사기를 상대 임포트 해석 + 고아 모듈(requires 폐포) 포함 + 파일당
1회 파싱으로 (68 → 89 계약, 7 s → 0.3 s); 항진 검사 제거; 계약 테스트 v2.
- tools/trace_common.py: 앵커 폴백(prep_fused / sampler), 스트림 합집합 유휴,
빈 시퀀스 가드. 9월 1일 트레이스 유휴 5.65 → 9.24 ms/스텝(13.0%), 동시
실행분 3.58 ms — 보충 분해 2 의 합집합 8.9 ms 와 일치.
- 문서: 수치는 모듈 README 한 곳, 프로필·RUNBOOK·원장·커널맵 정정(gemmSN 귀속,
0.65 → 1.2%, "5 ms 동시 실행" 문장 철회), EXP-9 게이트에 pos-1 수용률 ±2pct,
EXP-7 부팅에만 얹음(EXP-8 제외), profiles/README 모듈 표 4개 추가·고아 2개 정정.
41,244 checks OK.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
(cherry picked from commit 95fd6d7)
4734d32 to
839797c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The updated probe probes/moe_gate_tile_sweep.py has a real CLI robustness bug (--nw < 3 or --nw=0) and an O(n²) indexing pattern that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses review feedback around GLM-5.3’s split-K head-gate experiment (EXP-9) and accompanying validation/tooling: it corrects the fleet checkpoint shape (N=32), makes the split-K path deterministic (no fp32 atomics), strengthens applicability guards and contract checks, and updates trace-analysis tooling/docs to match measured behavior (including union-of-stream idle accounting).
Changes:
- Update the split-K head-gate implementation to deterministic two-stage reduction, enforce N≤32 + layout/K checks, and adjust wiring/contract tests/logging accordingly.
- Refactor trace analysis scripts into shared utilities (
trace_common.py) and correct idle/overlap accounting using interval union across streams. - Refresh probes, runbook/docs, and profile metadata to reflect corrected measurements, gating rules, and experiment stacking guidance.
File summaries
| File | Description |
|---|---|
| tools/trace_step_timeline.py | Refactors timeline inspection to use shared trace utilities and adds step-index selection. |
| tools/trace_step_composition.py | Reworks per-step composition to use union-of-intervals busy/idle and shared categorization. |
| tools/trace_common.py | Adds shared loader/step-cutter/category utilities with anchor fallbacks and union-busy calculation. |
| tests/test_logic.py | Expands overlay symbol contract checks to per-composition (incl. orphans) and resolves relative imports. |
| STEP_KERNEL_MAP.md | Updates step composition and EXP-9 notes to corrected N=32 measurements and union-idle accounting. |
| RUNBOOK_KERNEL_CAMPAIGN2.md | Revises EXP-9 guidance (N=32, deterministic reduction, stacking rules, gate criteria). |
| profiles/README.md | Updates module table entries and adds GLM53 experiment modules including EXP-6..9. |
| profiles/glm53.env | Updates EXP-9 knob documentation to N=32, determinism, gating, and logging semantics. |
| probes/moe_gate_tile_sweep.py | Switches to importing shipped kernel directly and updates sweep/printing logic. |
| probes/indexer_gate_check.py | Updates probe to validate routing/determinism/timing using real entrypoint and N from config. |
| overlay/modules/glm53_model_wiring/glm53_prefill_fastpath.py | Tightens optional helper import behavior: log internal ImportErrors; warn when knob set but module missing. |
| overlay/modules/glm53_indexer_gate_splitk/requires | Removes glm53_model_wiring dependency to keep the module self-contained. |
| overlay/modules/glm53_indexer_gate_splitk/README.md | Consolidates corrected N=32 measurements, determinism guarantees, routing/logging, and operational guidance. |
| overlay/modules/glm53_indexer_gate_splitk/glm53_indexer_gate.py | Implements deterministic split-K (partial + fixed-order reduce), adds applicability guards and one-time routing logs. |
| MEASUREMENTS.md | Records review corrections and updated measurements (N=32, determinism, union-idle fix, tooling updates). |
| build/glm53/glm53_prefill_fastpath.py | Mirrors the wiring fastpath import/logging changes in the build output. |
| build/glm53/glm53_indexer_gate.py | Mirrors deterministic split-K kernel + guards/logging in the build output. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def time_graph(fn, n, reps=20): | ||
| for i in range(3): | ||
| fn(xs[i], ws[i]) | ||
| fn(i) | ||
| torch.cuda.synchronize() |
| for t, BN, BK, warps, stages, exact in rows: | ||
| tag = " <- shipped" if (BN, BK, warps, stages) == (16, 512, 4, shipped_stages(BM)) else "" | ||
| if t != float("inf") and (rows.index((t, BN, BK, warps, stages, exact)) < 12 or tag): | ||
| print(f" {t:6.1f} us BN{BN:2d} BK{BK:4d} w{warps} s{stages} programs={args.E//BN:3d} " | ||
| f"bit-exact-vs-shipped={exact}{tag}") |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95fd6d7493
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for a, b in zip(starts[5:-1], starts[6:]): | ||
| seg = ev[a:b] | ||
| t0 = seg[0]["ts"] | ||
| t1 = max(e["ts"] + e["dur"] for e in seg) |
There was a problem hiding this comment.
In the new multi-stream trace analysis, when a kernel on a secondary stream starts before the next preparation anchor but finishes after it, using the maximum kernel end makes this step extend into the following step. Its post-boundary duration is consequently included in this step's span, busy union, idle calculation, and category totals, so the reported 9.24 ms idle figure can be skewed precisely for the concurrent traces this change targets. Use ev[b]["ts"] as the step end and clip intervals and durations to that anchor boundary.
Useful? React with 👍 / 👎.
| if M <= mod.MAX_M: | ||
| t_sk = _graph_us(lambda i: mod.head_gate_splitk(xs[i], ws[i])) | ||
| print(f" M={M:2d}: torch.mm {t_mm:6.1f} us | split-K {t_sk:6.1f} us | routed: split-K") |
There was a problem hiding this comment.
When --config describes a checkpoint outside the admitted shape, such as index_n_heads > 32 or a hidden size not divisible by 128, the routing checks record a failure but this loop still chooses the route solely from M and invokes head_gate_splitk directly. That can benchmark and label as routed a kernel production would never call; for a non-divisible K the raw helper omits the trailing K elements, and for K below 128 it attempts a zero-sized launch. Time the real head_gate entry point or require splitk_applicable(xs[i], ws[i]) before invoking the raw kernel.
Useful? React with 👍 / 👎.
| # --- numerics vs torch.mm (routing on) | ||
| worst_abs = worst_rel = 0.0 | ||
| flips1 = flips4 = rows = 0 | ||
| top4 = lambda a: a.topk(4, dim=1).indices.sort(1).values # noqa: E731 |
There was a problem hiding this comment.
For --N 1, --N 2, or --N 3 (or a config with that many index heads), the kernel's stated 0 < N <= 32 applicability contract admits the shape, but this unconditional topk(4) raises because the requested k exceeds N. The probe therefore cannot validate otherwise supported shapes and exits with an exception rather than its documented RESULT status; use min(4, N) or skip the top-4 comparison when fewer than four heads exist.
Useful? React with 👍 / 👎.
| for BN, BK, warps, stages in itertools.product((8, 16, 32), (256, 512, 1024), (2, 4, 8), (2, 3, 4)): | ||
| if args.E % BN: continue | ||
| BM = 16 if M <= 16 else 32 | ||
| if (BM + BN) * BK * 2 * (stages - 1) > 99 * 1024: continue | ||
| if args.E % BN or (BM + BN) * BK * 2 * (stages - 1) > 99 * 1024: | ||
| continue |
There was a problem hiding this comment.
For a valid shipped shape whose K is divisible by 512 but not every swept tile, such as --K 4608, this filter still admits BK=1024. The imported kernel iterates to K in full BLOCK_K chunks and performs unmasked x/weight loads, so that candidate's final tile reads beyond both tensors and can poison the CUDA context, aborting the remainder of the sweep. Add args.K % BK to the candidate rejection condition.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def _load_module(): | ||
| os.environ.setdefault("VLLM_MOE_GATE_FUSED", "1") # the kernel is defined under this gate |
There was a problem hiding this comment.
When the probe is launched from an environment that explicitly sets VLLM_MOE_GATE_FUSED=0 or another false value, setdefault preserves that value. The imported module defines _deneb_gate_partial_kernel only when this gate is enabled, so the first run then fails with AttributeError instead of performing the sweep. Because this probe unconditionally requires that private kernel, assign the gate to "1" before import or reject a disabled value with a clear error.
Useful? React with 👍 / 👎.
#228/#229 리뷰(15건) 전부 처리. 41,244 checks OK.
치명 3건
index_n_heads=32→_wp_fp32[4096,32], 헬퍼는 N<=16 만 허용·BN=16 고정. v2 로 N<=32 지원, 실측을 N=32 로 다시 함.v2 실측 (N=32, K=4096, 콜드 L2,
probes/indexer_gate_check.py --config <ckpt>/config.json, RESULT OK)torch.mm천장 11 × 76.8 us = 0.84 ms = C=1 의 ~1.2%. 수치 2.4e-6 절대·7.2e-7 상대, 순위 뒤집힘 0. cuBLAS N=32 콜드 88 us 는 트레이스 in-situ 88 us 와 일치 (첫 판의 47 us 는 N=16 값).
나머지
ModuleNotFoundError(해당 모듈)만 조용히, 내부 ImportError 는 로그, 노브 켜졌는데 모듈 없으면 경고. 헬퍼가 형상별 1회[indexer-gate] ... -> split-K로그.requires비움.tools/trace_common.py(앵커 폴백 prep_fused/sampler, 스트림 합집합 유휴, 가드). 유휴 정정 5.65 → 9.24 ms/스텝(13.0%), 동시 실행 3.58 ms — 보충 분해 2 와 일치.🤖 Generated with Claude Code
Note
Medium Risk
Opt-in path changes fp32 head-gate numerics and sparse indexer pool selection on replicated TP ranks; mitigated by strict routing guards, offline probes, and bracket gates, but still a served-numerics axis.
Overview
Addresses 15 review findings on PR #228 for
glm53_indexer_gate_splitk(EXP-9). The first version never ran in production: fleetindex_n_heads=32was rejected by N≤16 / BN=16 guards, benchmarks used synthetic N=16, and fp32 atomic reduction was non-deterministic across replays and TP ranks.Kernel v2 splits K across 32 programs, writes partials, then reduces in fixed order (no atomics/memset). Supports N≤32 with layout checks (2-D x, unit inner stride, K match).
head_gatelogs routing once per shape; fastpath import failures are surfaced when split-K is armed. Modulerequirescleared (optional import from wiring only).Validation & tooling:
probes/indexer_gate_check.pyexercises realhead_gateentry,--configcheckpoint shape, routing/determinism/timing gates with non-zero exit on failure. MoE gate sweep imports the shipped kernel. Trace tools gaintrace_common.py(multi-anchor steps, stream-union idle); docs/runbook/profile comments updated (~1.2% ceiling, EXP-9 rides EXP-7 only, pos-1 acceptance ±2pct, idle 9.24 ms not 5.65 ms).test_logic.pyexpands overlay symbol checks (relative imports, orphan compositions) and v2 gate contracts.Reviewed by Cursor Bugbot for commit 839797c. Bugbot is set up for automated code reviews on this repo. Configure here.