Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Manifold-Constrained Hyper Connections (mHC) implementation by sharing the compute_sigmoid_gate helper function between the layer and the Pallas kernel, ensuring identical gating behavior across both paths. The local mapping method in src/maxtext/layers/mhc.py has been removed in favor of this shared helper, which computes in float32 and is subsequently cast back to the activation dtype. Additionally, the unit tests in tests/unit/mhc_test.py have been refactored to reduce duplication using helper methods and a new context manager for kernel interpretation, and new tests have been added to verify that the Pallas kernel matches the reference path and that the sigmoid gate computes correctly in float32. There are no review comments, so no further feedback is provided.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
14a680e to
e18a3cc
Compare
|
هل تم انتهاء المشكله |
|
@dandragona thanks for the review. CI job failedhttps://github.com/AI-Hypercomputer/maxtext/actions/runs/36363999320/job/109071898923?pr=5179 Root Cause
Note: The test is new, but the problem is not. Enabling Fix Options
|
|
Hi @RissyRan @dandragona @bvandermoon, I've pushed new changes that fix the tests that were failing during the recent CI runs. Could you please take a look at the updates and approve the pending CI workflows so the tests can execute? Thanks! DescriptionLets the mHC Pallas kernels ( ProblemA bare SolutionEvery mHC operation (the RMS norm, the projections and the gates) works within a single token, reducing only over Making the kernels run correctly inside
Implementation
TestsNew and updated tests in
|
Note: this PR also fixed #5682 (see comment #5179 (comment) for details)
Bug report: #5195
Description
Motivation:
The reference path had its own
mappinghelper that castalpha_scaleandbetadown toself.dtypebefore computingsigmoid(alpha_scale * h + beta),while the Pallas path called
common.compute_sigmoid_gate, which keeps thescale and bias in float32. With
dtype: bfloat16the two branches thereforegated on different numbers, so toggling
use_mhc_pallas_kernelchanged thelayer's output beyond accumulation noise.
Implementation:
Drop
ManifoldConstrainedHyperConnections.mappingand call the sharedcompute_sigmoid_gatefrom both pre- and post-mapping, casting the float32result back to
self.dtypeso the following GEMM is unchanged. Export thehelper from
maxtext.kernels.mhcso the layer can reach it.Tests:
test_pallas_kernel_matches_reference_pathruns the same weights throughboth branches and asserts they agree; the
_tpuvariant repeats it againstthe Mosaic-compiled kernel instead of interpret mode.
test_sigmoid_gate_computes_in_float32pins the helper's precision directly,since the end-to-end comparison cannot resolve a bfloat16 regression there.
_interpreted_mhc_kernelcontext manager, and the repeated module/MLPconstruction into
_build_mhc_and_mlp.Tests
Unit tested on CPU and TPU v6e-1
Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.