-
Notifications
You must be signed in to change notification settings - Fork 553
Fix Diffusers export dtype resolution #2225
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?
Changes from all commits
1e9d346
cc2c218
8b9f723
aaac495
22a10db
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 |
|---|---|---|
|
|
@@ -826,11 +826,15 @@ def _dispatch_export_handler(name: str, sub_module: nn.Module, ctx: ExportContex | |
|
|
||
| def _resolve_export_dtype(model: nn.Module, dtype: torch.dtype | None) -> torch.dtype: | ||
| """Return the export dtype, defaulting to the model's own and warning on a mismatch.""" | ||
| configured_dtype = getattr(model.config, "torch_dtype", None) | ||
| if dtype is None: | ||
| return model.config.torch_dtype | ||
| if dtype != model.config.torch_dtype: | ||
| if configured_dtype is not None: | ||
| return configured_dtype | ||
| first_parameter = next(model.parameters(), None) | ||
| return first_parameter.dtype if first_parameter is not None else torch.float16 | ||
|
Collaborator
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.
This is a verbatim re-implementation of |
||
| if configured_dtype is not None and dtype != configured_dtype: | ||
| warnings.warn( | ||
| f"Model's original dtype ({model.config.torch_dtype}) differs from target dtype " | ||
| f"Model's original dtype ({configured_dtype}) differs from target dtype " | ||
| f"({dtype}), which may lead to numerical errors." | ||
| ) | ||
| return dtype | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
|
|
||
| """Tests for tied-weight helpers in unified_export_hf.""" | ||
|
|
||
| from types import SimpleNamespace | ||
|
|
||
| import pytest | ||
| import torch | ||
| from _test_utils.torch.quantization.tied_modules import ( | ||
|
|
@@ -29,9 +31,44 @@ | |
| postprocess_state_dict, | ||
| sync_tied_input_amax, | ||
| ) | ||
| from modelopt.torch.export.unified_export_hf import _resolve_export_dtype | ||
| from modelopt.torch.quantization.nn import TensorQuantizer | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("configured_dtype", "dtype", "expected_dtype", "warning_count"), | ||
| [ | ||
| (None, None, torch.float32, 0), | ||
| (None, torch.float16, torch.float16, 0), | ||
| (torch.bfloat16, None, torch.bfloat16, 0), | ||
| (torch.bfloat16, torch.bfloat16, torch.bfloat16, 0), | ||
| (torch.bfloat16, torch.float16, torch.float16, 1), | ||
| ], | ||
| ) | ||
| def test_resolve_export_dtype(configured_dtype, dtype, expected_dtype, warning_count, recwarn): | ||
| model = torch.nn.Linear(1, 1) | ||
| model.config = ( | ||
| SimpleNamespace(torch_dtype=configured_dtype) if configured_dtype is not None else object() | ||
| ) | ||
|
|
||
| assert _resolve_export_dtype(model, dtype) == expected_dtype | ||
| assert len(recwarn) == warning_count | ||
| if warning_count: | ||
| assert str(recwarn[0].message) == ( | ||
| "Model's original dtype (torch.bfloat16) differs from target dtype " | ||
| "(torch.float16), which may lead to numerical errors." | ||
| ) | ||
|
|
||
|
|
||
| def test_resolve_export_dtype_with_empty_diffusers_config(): | ||
|
Collaborator
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.
This case is behaviorally identical to the already-parameterized |
||
| # Import locally so Diffusers stays optional during torch-only test collection. | ||
| frozen_dict = pytest.importorskip("diffusers.configuration_utils").FrozenDict() | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| model = torch.nn.Linear(1, 1) | ||
| model.config = frozen_dict | ||
|
|
||
| assert _resolve_export_dtype(model, None) == torch.float32 | ||
|
|
||
|
|
||
| def test_hf_all_tied_weights_keys_contract(): | ||
| """Pin the transformers API we build tied_map from, so a version bump fails loud here. | ||
|
|
||
|
|
||
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.
How does a Diffusers export reach this function?
export_hf_checkpoint()dispatchesis_diffusers_object(model)to_export_diffusers_checkpoint(), which resolves dtype throughinfer_dtype_from_model()and never readsmodel.config.torch_dtype;_resolve_export_dtypeis only reachable from the transformers (resident + streaming) paths. If a diffusers pipeline/component is landing here, that meansis_diffusers_object()returnedFalse(most likelyHAS_DIFFUSERS/_HAS_DIFFUSERSbeingFalsebecause thefrom diffusers import ...guard failed under the minimum-transformers combo), and the root cause is the detection, not the missingtorch_dtype— this patch would only push the failure to the next transformers-only step (TiedWeightMap(model),requantize_resmooth_fused_llm_layers(model),model.named_modules()on a pipeline). Please include the traceback in the PR body so the actual failing dispatch is on record.Also,
getattr(model.config, ...)still raises if the object has no.configat all;getattr(getattr(model, "config", None), "torch_dtype", None)would be the fully defensive form if that's the intent.