-
Notifications
You must be signed in to change notification settings - Fork 531
Bug fix: 6542481 #2064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Bug fix: 6542481 #2064
Changes from all commits
e7aae46
3990f0c
24cf9f3
e8d77d7
5363203
7a116e7
46596f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -991,16 +991,10 @@ def postprocess_state_dict( | |
| "weight_shape", | ||
| ] | ||
|
|
||
| # For modelopt-trained LoRA models, we need to remove the base_layer prefix from the keys for deployment | ||
| if is_modelopt_qlora: | ||
| replacements.update( | ||
| { | ||
| "base_layer.weight": "weight", | ||
| "base_layer.input_scale": "input_scale", | ||
| "base_layer.weight_scale": "weight_scale", | ||
| } | ||
| ) | ||
| skip_keys.append("base_layer") | ||
| def _export_key(key: str) -> str: | ||
| # PEFT nests the quantized module under `base_layer`, which deployment does not expect. | ||
| # Strip it generically so new key types (bias, scales) do not need to be enumerated here. | ||
| return key.replace(".base_layer.", ".") if is_modelopt_qlora else key | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMPORTANT Export] The generic
post_state_dict[_export_key(key)] = value # last-write-wins on collisionThe old code had the same class of bug but failed loudly downstream (missing key → dequant error), whereas this fails silently with a plausible-looking checkpoint. Since the whole point of this PR is that a wrong def _set(target: dict, key: str, value):
export_key = _export_key(key)
if export_key in target:
logger.warning(
f"Export key collision: '{key}' maps to '{export_key}', which is already "
f"populated. Keeping the first value."
)
return
target[export_key] = valueand call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and thanks for posting the correction below rather than leaving it standing — I reached the same conclusion independently while checking it. Not taking the The zero-match warning from your other comment is different and I did take it — there the silent failure was demonstrably reachable, since it is exactly how this NVBug hid. Your key-by-key trace of what the generic strip now newly admits matches mine, including that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction to my comment above — downgrading this from IMPORTANT to SUGGESTION. I asserted a concrete collision (
So there is no live bug here, and the strip itself is correct. I also traced every key the generic strip now newly admits, and they all behave right:
The
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged — answered on the parent thread. Short version: agreed the collision is unreachable, so I am not adding the |
||
|
|
||
| post_state_dict = {} | ||
|
|
||
|
|
@@ -1012,7 +1006,7 @@ def postprocess_state_dict( | |
|
|
||
| # Skip keys not related to quantizers | ||
| if all(skip_key not in key for skip_key in skip_keys): | ||
| post_state_dict[key] = value | ||
| post_state_dict[_export_key(key)] = value | ||
| continue | ||
|
|
||
| # Apply replacements if the key matches any suffix in the replacements dict | ||
|
|
@@ -1033,7 +1027,7 @@ def postprocess_state_dict( | |
| logger.warning( | ||
| "Large KV activations detected. Quantized KV cache may lead to higher accuracy drop." | ||
| ) | ||
| post_state_dict[prefix + new_suffix] = value | ||
| post_state_dict[_export_key(prefix + new_suffix)] = value | ||
| break | ||
|
|
||
| # Squeeze scales with a leading dimension of 1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,16 +110,34 @@ def _restore_qtensor_wrappers(model, model_path): | |
| q_tensor_state = mode_config.get("metadata", {}).get("q_tensor_state", {}) | ||
| if not q_tensor_state: | ||
| continue | ||
| for name, module in model.named_modules(): | ||
| if ( | ||
| isinstance(module, RealQuantLinear) | ||
| and name in q_tensor_state | ||
| and not isinstance(module.weight, QTensorWrapper) | ||
| ): | ||
| module._parameters["weight"] = QTensorWrapper( | ||
| qtensor=module.weight.data, | ||
| metadata=q_tensor_state[name]["metadata"], | ||
| ) | ||
| # PEFT nests the quantized linear as `<name>.base_layer`, and either the saved keys or the | ||
| # live names may carry that suffix. Normalize both so the lookup works in either direction. | ||
| q_tensor_state = {k.removesuffix(".base_layer"): v for k, v in q_tensor_state.items()} | ||
|
|
||
| pending = [ | ||
| (name, module) | ||
| for name, module in model.named_modules() | ||
| if isinstance(module, RealQuantLinear) and not isinstance(module.weight, QTensorWrapper) | ||
| ] | ||
| matched = 0 | ||
| for name, module in pending: | ||
| key = name.removesuffix(".base_layer") | ||
| if key not in q_tensor_state: | ||
| continue | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMPORTANT ModeState] The fix is correct, but the loop still fails silently when nothing matches — which is the precise failure mode this PR exists to fix. Before this PR, Since you already know how many entries you expect to re-wrap, the check is nearly free: state = load_modelopt_state(modelopt_state_path)
for _, mode_config in state["modelopt_state_dict"]:
q_tensor_state = mode_config.get("metadata", {}).get("q_tensor_state", {})
if not q_tensor_state:
continue
matched = 0
for name, module in model.named_modules():
if not isinstance(module, RealQuantLinear) or isinstance(module.weight, QTensorWrapper):
continue
key = name if name in q_tensor_state else name.removesuffix(".base_layer")
if key not in q_tensor_state:
continue
module._parameters["weight"] = QTensorWrapper(
qtensor=module.weight.data,
metadata=q_tensor_state[key]["metadata"],
)
matched += 1
if not matched:
warnings.warn(
f"Found {len(q_tensor_state)} saved compressed weight(s) in {modelopt_state_path} "
"but re-wrapped none — module names may have been remapped by a wrapper. "
"The model will likely fail when the packed weights are used."
)Note
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taken in 42c4671 — thanks for the follow-up narrowing it to the right condition. I used if pending and not matched:
warnings.warn(
f"Found {len(q_tensor_state)} compressed weight(s) in {modelopt_state_path} but "
f"re-wrapped none of the {len(pending)} candidate module(s); their names may have "
"been remapped. The model will likely fail when the packed weights are used."
)Covered by The reverse-direction case you raise in the same comment is also fixed — normalizing the |
||
| module._parameters["weight"] = QTensorWrapper( | ||
| qtensor=module.weight.data, | ||
| metadata=q_tensor_state[key]["metadata"], | ||
| ) | ||
| matched += 1 | ||
|
|
||
| # A total miss means some wrapper renamed the modules. Warn instead of letting it surface | ||
| # as an opaque shape error at dequantization. | ||
| if pending and not matched: | ||
| warnings.warn( | ||
| f"Found {len(q_tensor_state)} compressed weight(s) in {modelopt_state_path} but " | ||
| f"re-wrapped none of the {len(pending)} candidate module(s); their names may have " | ||
| "been remapped. The model will likely fail when the packed weights are used." | ||
| ) | ||
|
|
||
|
|
||
| def _new_from_pretrained(cls, /, pretrained_model_name_or_path, *args, **kwargs): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,12 @@ | |
| # limitations under the License. | ||
|
|
||
|
|
||
| import json | ||
|
|
||
| import pytest | ||
| import torch | ||
| from _test_utils.examples.run_command import run_example_command | ||
| from safetensors.torch import load_file | ||
|
|
||
| # Mapping from backend name to accelerate config file | ||
| BACKEND_CONFIGS = { | ||
|
|
@@ -86,6 +90,18 @@ def _run_train(config: str, extra_cmd_args: list[str], backend: str = "fsdp2", c | |
| setup_free_port=True, | ||
| ) | ||
|
|
||
|
|
||
| def _run_export(ckpt_dir: str, export_dir: str): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to see the export step added to the e2e -- but note this is the only coverage for the A cheap unit test would guard the rename directly: a stub module tree containing a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That unit test exists — cc60139 added it, one commit before this review, and it is close to what you sketch: It builds a real compressed One trap worth recording for anyone extending it: the setup must On the e2e cost — agreed it should not be the only coverage, which is why the unit test carries the real load. The export step stays because it is the only thing that exercises the |
||
| run_example_command( | ||
| [ | ||
| "python", "export.py", | ||
| "--pyt_ckpt_path", ckpt_dir, | ||
| "--export_path", export_dir, | ||
| ], | ||
| "llm_qat", | ||
| ) | ||
|
|
||
|
|
||
| def test_dataset_utils_pretokenize(tiny_qwen3_path, tmp_path): | ||
| """Test dataset_utils.py standalone CLI pre-tokenization.""" | ||
| cache_dir = tmp_path / "dataset_cache" | ||
|
|
@@ -152,18 +168,43 @@ def test_qwen3_lora_qat_nvfp4(tiny_qwen3_path, tmp_path): | |
| ) | ||
|
|
||
| # Step 2: LoRA QAT | ||
| lora_qat_output_dir = tmp_path / "lora_qat" | ||
| _run_train( | ||
| "configs/train/qat_nvfp4.yaml", | ||
| [ | ||
| "--model_name_or_path", str(ptq_output_dir), | ||
| "--do_train", "True", | ||
| "--lora", "True", | ||
| "--output_dir", str(tmp_path / "lora_qat"), | ||
| "--output_dir", str(lora_qat_output_dir), | ||
| ], | ||
| backend="fsdp2", | ||
| cache_dir=cache_dir, | ||
| ) | ||
|
|
||
| # Step 3: Export. This checkpoint is fake-quantized, so the calibrated amaxes rather than | ||
| # packed weights are what must survive the load. | ||
| export_dir = tmp_path / "lora_qat_export" | ||
| _run_export(str(lora_qat_output_dir), str(export_dir)) | ||
|
|
||
| base_model_dir = export_dir / "base_model" | ||
| with open(base_model_dir / "hf_quant_config.json") as f: | ||
| assert json.load(f)["quantization"]["quant_algo"] == "NVFP4" | ||
|
|
||
| base_weights = load_file(base_model_dir / "model.safetensors") | ||
| assert not any("base_layer" in k or k.endswith("_amax") for k in base_weights) | ||
|
|
||
| # LoRA freezes the base model, so a direct PTQ export is a trusted oracle for every calibrated | ||
| # value. This catches scales that keep their key but were reset to defaults. | ||
| ptq_export_dir = tmp_path / "ptq_export" | ||
| _run_export(str(ptq_output_dir), str(ptq_export_dir)) | ||
| reference = load_file(ptq_export_dir / "model.safetensors") | ||
|
|
||
| scales = [k for k in reference if k.endswith(("_scale", "_scale_2"))] | ||
| assert scales, "no NVFP4 scales in the reference PTQ export" | ||
| for key in scales: | ||
| assert key in base_weights, f"{key} missing from the LoRA-QAT export" | ||
| assert torch.equal(base_weights[key], reference[key]), f"{key} does not match PTQ export" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("backend", [ | ||
| "fsdp2", | ||
|
|
@@ -219,14 +260,37 @@ def test_qwen3_qlora_nvfp4(tiny_qwen3_path, tmp_path): | |
| ) | ||
|
|
||
| # Step 2: QLoRA training | ||
| qlora_output_dir = tmp_path / "qlora" | ||
| _run_train( | ||
| "configs/train/qlora_nvfp4.yaml", | ||
| [ | ||
| "--model_name_or_path", str(ptq_output_dir), | ||
| "--do_train", "True", | ||
| "--lora", "True", | ||
| "--output_dir", str(tmp_path / "qlora"), | ||
| "--output_dir", str(qlora_output_dir), | ||
| ], | ||
| backend="ddp", | ||
| cache_dir=cache_dir, | ||
| ) | ||
|
|
||
| # Step 3: Export the QLoRA checkpoint for deployment | ||
| export_dir = tmp_path / "qlora_export" | ||
| _run_export(str(qlora_output_dir), str(export_dir)) | ||
|
|
||
| # The base model is exported compressed; the adapters stay at the top level. | ||
| base_model_dir = export_dir / "base_model" | ||
| assert (export_dir / "adapter_model.safetensors").is_file() | ||
| assert (base_model_dir / "hf_quant_config.json").is_file() | ||
|
|
||
| with open(base_model_dir / "hf_quant_config.json") as f: | ||
| assert json.load(f)["quantization"]["quant_algo"] == "NVFP4" | ||
|
|
||
| # NVFP4 needs the packed weight and *both* scales to be dequantizable downstream. | ||
| base_weights = load_file(base_model_dir / "model.safetensors") | ||
| packed_weights = [k for k, v in base_weights.items() if k.endswith(".weight") and v.dtype == torch.uint8] | ||
| assert packed_weights, "no NVFP4-packed weights found in the exported base model" | ||
| for key in packed_weights: | ||
| prefix = key.removesuffix(".weight") | ||
| assert f"{prefix}.weight_scale" in base_weights | ||
| assert f"{prefix}.weight_scale_2" in base_weights | ||
| assert not any("base_layer" in k or "lora" in k for k in base_weights) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this bug from 0.45?