Onboard Qwen3.5 397b fp8 - #5182
Conversation
… llama 3.1 and qwen 3.5 fp8 - linears.py: simplify dequantize_weight to handle scalar (Llama 3.1) and block scales (Qwen 3.5); remove DeepSeekV4GroupedLinear quantization hooks and redundant parameter accessors. - common_types.py: replace cascading fnmatch loops in get_weight_dtype with static leaf lookup UNQUANTIZED_MODULE_LEAF_NAMES. - initializers.py: use explicit is_fp8_dtype instead of itemsize heuristics. - moe.py: replace np.ceil with integer division and clean up repeated hasattr guards. - maxengine.py: revert out-of-scope cache casting changes.
…ck-scaled quantization
…stion in to_maxtext
…ides in to_maxtext
There was a problem hiding this comment.
Code Review
This pull request introduces support for FP8 weight-only storage with dynamic dequantization in MaxText, specifically targeting Qwen3.5 models. It implements dynamic dequantization utilities, integrates them into linear and MoE layers, and adds support for dequantizing weights on-the-fly during checkpoint loading. The review feedback highlights several critical issues, including a reversed loop structure in MoE expert parameter mapping that causes crashes, a runtime AttributeError in Qwen3NextRMSNorm from calling get_value() on an NNX parameter, and potential shared memory exhaustion from automatically setting HF_HOME to /dev/shm. Additionally, the reviewer recommends addressing code duplication for key resolution and tensor stacking, fixing a potential NameError with str2bool, and avoiding hardcoded absolute paths in test scripts.
| if "HF_HOME" not in os.environ and os.path.exists("/dev/shm"): | ||
| os.environ["HF_HOME"] = "/dev/shm/hf_cache" |
There was a problem hiding this comment.
Setting HF_HOME to /dev/shm/hf_cache automatically when /dev/shm exists can easily exhaust the shared memory (shm) space, especially for extremely large models like Qwen3.5 397B (which is hundreds of gigabytes). Shared memory is typically limited in size and storing large model checkpoints there can lead to 'No space left on device' or bus errors. It is safer to rely on the default Hugging Face cache directory or let the user configure HF_HOME explicitly.
| y = jax.lax.with_sharding_constraint(y, out_sharding) | ||
| return y | ||
|
|
||
| scale = self.scale.get_value() |
There was a problem hiding this comment.
The method get_value() does not exist on flax.nnx.Param (or Variable). Calling it will raise an AttributeError at runtime. To get the parameter value, use self.scale.value or self.scale[...], which is consistent with how other parameters are accessed in this codebase (e.g., in linears.py and moe.py).
| scale = self.scale.get_value() | |
| scale = self.scale[...] |
| resolved_key = key | ||
| shard_name = self.shard_map.get(resolved_key) | ||
| if shard_name is None: | ||
| # Check fallback for .weight_scale vs .scale and inverse scales | ||
| if resolved_key.endswith(".weight_scale"): | ||
| for suffix in [".scale", ".weight_scale_inv", ".scale_inv"]: | ||
| alt_key = resolved_key[:-len(".weight_scale")] + suffix | ||
| if alt_key in self.shard_map: | ||
| resolved_key = alt_key | ||
| shard_name = self.shard_map[resolved_key] | ||
| break | ||
| elif resolved_key.endswith(".scale"): | ||
| for suffix in [".weight_scale", ".scale_inv", ".weight_scale_inv"]: | ||
| alt_key = resolved_key[:-len(".scale")] + suffix | ||
| if alt_key in self.shard_map: | ||
| resolved_key = alt_key | ||
| shard_name = self.shard_map[resolved_key] | ||
| break | ||
|
|
There was a problem hiding this comment.
The fallback key resolution logic is duplicated here and inside the _ram_lock block. Extracting this into a local helper function _resolve_key makes the code cleaner, more maintainable, and eliminates the duplication.
def _resolve_key(k, container):
if k in container:
return k
if k.endswith(".weight_scale"):
base, suffixes = k[:-13], [".scale", ".weight_scale_inv", ".scale_inv"]
elif k.endswith(".scale"):
base, suffixes = k[:-6], [".weight_scale", ".scale_inv", ".weight_scale_inv"]
else:
return k
for suffix in suffixes:
alt_key = base + suffix
if alt_key in container:
return alt_key
return k
resolved_key = _resolve_key(key, self.shard_map)
shard_name = self.shard_map.get(resolved_key)| final_key = resolved_key | ||
| if final_key not in f.keys(): | ||
| if final_key.endswith(".weight_scale"): | ||
| for suffix in [".scale", ".weight_scale_inv", ".scale_inv"]: | ||
| alt_key = final_key[:-len(".weight_scale")] + suffix | ||
| if alt_key in f.keys(): | ||
| final_key = alt_key | ||
| break | ||
| elif final_key.endswith(".scale"): | ||
| for suffix in [".weight_scale", ".scale_inv", ".weight_scale_inv"]: | ||
| alt_key = final_key[:-len(".scale")] + suffix | ||
| if alt_key in f.keys(): | ||
| final_key = alt_key | ||
| break | ||
| t = f.get_tensor(final_key) |
| if config.scan_layers: | ||
| # If it's a standard scanned layer, we use the configured param_scan_axis. | ||
| axis_to_stack = config.param_scan_axis | ||
| axis_to_stack = config.param_scan_axis if len(target_shape) > config.param_scan_axis else 0 |
There was a problem hiding this comment.
The function _build_single_axis_stacked_tensor is duplicated across three files: to_maxtext.py, utils/tensor_handling.py, and utils/utils.py. This duplication makes maintenance difficult and error-prone (as seen in this PR where the same fix had to be applied to all three files). Consider defining this function once in a shared utility module (e.g., utils/tensor_handling.py) and importing it where needed.
| elif arg.startswith("lazy_load_tensors="): | ||
| lazy_load_tensors = str2bool(arg.split("=", 1)[1]) |
There was a problem hiding this comment.
Using str2bool here might raise a NameError if it is not imported or defined in this scope. A safer and more standard way to parse the boolean value from the command-line argument string is to check if it is in ('true', '1').
| elif arg.startswith("lazy_load_tensors="): | |
| lazy_load_tensors = str2bool(arg.split("=", 1)[1]) | |
| elif arg.startswith("lazy_load_tensors="): | |
| lazy_load_tensors = arg.split("=", 1)[1].lower() in ("true", "1") |
| if torch is not None and isinstance(v, torch.Tensor): | ||
| if v.dtype == torch.bfloat16: | ||
| v = v.to(torch.float32) | ||
| return v.to(torch.float32).cpu().numpy().astype(ml_dtypes.bfloat16) |
There was a problem hiding this comment.
The check if v.dtype == torch.bfloat16: v = v.to(torch.float32) is redundant because v.to(torch.float32) is called unconditionally on the very next line in the return statement. You can simplify this to a single line.
if torch is not None and isinstance(v, torch.Tensor):
return v.to(torch.float32).cpu().numpy().astype(ml_dtypes.bfloat16)| "load_parameters_path=/dev/shm/maxtext_qwen3.5_35b_fp8/0/items", | ||
| "scan_layers=true", | ||
| "per_device_batch_size=1", | ||
| "max_prefill_predict_length=4", | ||
| "max_target_length=4", | ||
| "async_checkpointing=false", | ||
| "sparse_matmul=false", | ||
| "ici_fsdp_parallelism=1", | ||
| "ici_expert_parallelism=-1", | ||
| "matmul_precision=highest", | ||
| "float32_logits=true", | ||
| "float32_qk_product=true", | ||
| "--golden_logits_path=/dev/shm/golden_qwen3.5_35b_fp8.pkl", | ||
| "--max_kl_div=0.2", |
There was a problem hiding this comment.
The test script hardcodes absolute paths in /dev/shm/ (e.g., /dev/shm/maxtext_qwen3.5_35b_fp8/0/items and /dev/shm/golden_qwen3.5_35b_fp8.pkl). These paths are environment-specific and will cause the script to fail on other systems or in CI/CD pipelines. Consider using environment variables or command-line arguments to specify these paths, or at least document how to set up these files.
64ad4e1 to
1bb4b03
Compare
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| # model config for qwen3.5-397b-a17b-fp8 (FP8 weight-only storage with dynamic dequantization) |
There was a problem hiding this comment.
what does dynamic dequant mean? Aren't the scales known ahead of time?
gobbleturk
left a comment
There was a problem hiding this comment.
Can you include performance/xprof links in the PR description? I thought block scaling was inefficient except for blocks>512 - at least block=256 to fit our MXU shape?
a1e42e1 to
93a2b66
Compare
Description
Onboards
Qwen/Qwen3.5-397B-A17B-FP8to MaxText and fixes scale-tensor sharding for block-scaled FP8 modules:Qwen3.5-397B-FP8 Model Onboarding:
qwen3.5-397b-a17b-fp8.ymlconfiguring FP8 weight storage (weight_dtype: "float8_e4m3fn"), 128x128 block scaling (weight_block_size: 128), and full-precision preservation forunquantized_modules.hf_model_configs.py,hf_shape.py,param_mapping.py,globals.py,types.py).Block Scale Sharding Fixes:
DenseGeneral: Replicates block-compressed scale dimensions instead of inheriting the kernel's mesh axes. Inheriting kernel sharding causes divisibility crashes when FSDP/TP mesh degree exceeds the compressed dimension (e.g., FSDP > 32 on a 32-block axis).RoutedMoE: Shards expert scales strictly along the expert axis (wi_kernel_axes[0]) and replicates the block-tile dimensions.tests/unit/linears_test.pyto assert scale replication on block-tiled layers.Tests
JAX_PLATFORMS=cpu PYTHONPATH=src pytest tests/unit/configs_test.py tests/unit/pyconfig_test.py -qJAX_PLATFORMS=cpu PYTHONPATH=src pytest tests/unit/linears_test.py -qqwen3.5-397b-a17b-fp8BUGS: b/557372531
Checklist
gemini-reviewlabel.