[Refactor][MoE] Separate dispatched expert compute - #1738
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
9712807 to
4edf0fe
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Ibuki-wind
left a comment
There was a problem hiding this comment.
Overall
One kernel-selection bug remains, and the draft needs a substantial size reduction before it is reviewable as a maintainable refactor.
Cross-cutting concerns
- The four new benchmark scripts repeat timing loops, tensor/model setup, output formatting, and correctness machinery across roughly 900 lines; consolidate the common harness and keep each entry point to scenario definitions plus backend adapters.
| n=ffn_size * 2, | ||
| k=hidden_size, | ||
| dtype=dtype, | ||
| kernel_map={"moe_grouped_gemm_kernel": kernel_cls, **(kernel_map or {})}, |
There was a problem hiding this comment.
The alignment fallback can be silently undone here and again for _gemm_down: after kernel_cls is changed to MoeGroupedGemmNopadKernel, a caller-supplied kernel_map['moe_grouped_gemm_kernel'] is expanded last and reinstates the misaligned 3WG kernel that the preceding check rejected. → Resolve one effective GEMM class first and either reject a conflicting override or build the sub-op maps without re-merging that key.
|
|
||
|
|
||
| @pytest.mark.smoke | ||
| def test_public_interface_has_no_routing_or_communication_inputs(): |
There was a problem hiding this comment.
This exact-signature introspection test (together with the adjacent dataclass-default checks) duplicates declarations instead of exercising behavior and will fail harmless parameter/default refactors. → Delete these contract-shape tests and keep the behavioral reference, capacity-tail, and graph-replay tests.
|
|
||
|
|
||
| @pytest.mark.smoke | ||
| def test_expert_batch_mock_dispatch_combine_applies_weights_once(): |
There was a problem hiding this comment.
This 100-line mock test never calls a dispatch or combine implementation: it manually builds the sorted batch, runs the expert op already covered above, then compares a manual weighted index_add_ against the same manual reference. → Delete it; the local-dispatch integration test in test_expert_dispatch.py covers the real composition with far less duplicate setup.
Ibuki-wind
left a comment
There was a problem hiding this comment.
Overall
The bounded-row kernel is a justified compiled specialization because valid_rows must remain device-side. Its source generation should be consolidated with the existing FusedGated strategies.
|
|
||
|
|
||
| @functools.lru_cache(maxsize=32) | ||
| def _make_fused_gated_direct_bounded( |
There was a problem hiding this comment.
These two bounded factories add another direct/explicit pair that repeats the same column mapping and gate/value/op_func/store template already owned by _make_fused_gated_direct and _make_fused_gated_explicit. The device-side row loop/guard is the real specialization; the pointwise strategy is not new. → Keep distinct compiled signatures (valid_rows must stay device-side), but use one source generator per strategy with a compile-time row policy: the normal policy emits the existing 2-D/flattened row mapping, and the bounded policy emits grid_rows, the persistent row loop, and row < valid_rows[0]. Alternatively, make one bounded generator parameterized by static num_per_thread (1 for direct). Do not add a runtime bounded/unbounded branch. Acceptance criterion: adding or changing the fused-gated epilogue or column mapping requires editing one source implementation, not four factories.
|
Does it now support the Megakernel MoE ? |
Summary
Review follow-up
Validation
M=1smoke passes with FP32 correctness checks.git diff --checkpass.Scope
expert_offsets[-1:]valid-row view.