Re-enable once pure_nnx config issue for DeepseekV4 - #4808
Conversation
There was a problem hiding this comment.
Code Review
This pull request re-enables the deepseek4-284b model integration test by setting the pure_nnx flag for this model and updating the _DEEPSEEK4_MLP_SCANNED dictionary. Feedback suggests consolidating _DEEPSEEK4_MLP_SCANNED and _DEEPSEEK4_MLP since they are now identical, and using a set instead of a direct string comparison for checking is_pure_nnx to improve extensibility.
| "bias": None, | ||
| "kernel": mdn(reduction_axis=(0,), output_axis=(-1,)), | ||
| }, | ||
| "gate": {"kernel": mdn(reduction_axis=(0,), output_axis=(-1,))}, |
There was a problem hiding this comment.
With the removal of "bias": None from "gate", _DEEPSEEK4_MLP_SCANNED is now completely identical to _DEEPSEEK4_MLP (defined on line 249). To improve maintainability and reduce code duplication, consider consolidating them in a future refactor (e.g., by pointing _DEEPSEEK4_MLP_SCANNED = _DEEPSEEK4_MLP or removing one of them entirely).
| is_pure_nnx = model_name == "deepseek4-284b" | ||
| actual_output = muon_utils.get_model_mdn(model_name, scan_layers=True, pure_nnx=is_pure_nnx) |
There was a problem hiding this comment.
Hardcoding the model name check model_name == "deepseek4-284b" is fragile and less extensible if other models are migrated to pure_nnx in the future. Using a set of models or a prefix check makes it cleaner and easier to maintain.
| is_pure_nnx = model_name == "deepseek4-284b" | |
| actual_output = muon_utils.get_model_mdn(model_name, scan_layers=True, pure_nnx=is_pure_nnx) | |
| is_pure_nnx = model_name in {"deepseek4-284b"} | |
| actual_output = muon_utils.get_model_mdn(model_name, scan_layers=True, pure_nnx=is_pure_nnx) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
517ff37 to
67dbeaf
Compare
Description
Start with a short description of what the PR does and how this is a change from
the past.
https://b.corp.google.com/issues/544531325
If the change fixes a bug or a Github issue, please include a link, e.g.,:
FIXES: b/544531325
Notice 1: Once all tests pass, the "pull ready" label will automatically be assigned.
This label is used for administrative purposes. Please do not add it manually.
Notice 2: For external contributions, our settings currently require an approval from a MaxText maintainer to trigger CI tests.
Tests
Please describe how you tested this change, and include any instructions and/or
commands to reproduce.
Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.