-
Notifications
You must be signed in to change notification settings - Fork 527
[NVBug: 6524370] use sequential device_map for DiffusionGemma #2041
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -304,6 +304,20 @@ def is_speculative(hf_config): | |
| ) | ||
|
|
||
|
|
||
| def is_diffusion_gemma(hf_config) -> bool: | ||
| """Check if the model architecture is DiffusionGemma. | ||
|
|
||
| Underscores are ignored: the family is spelled ``diffusion_gemma`` in configs | ||
| and ``DiffusionGemma`` in class names. The nested ``text_config`` is checked too, | ||
| since multi-modal wrappers keep the family name there. | ||
| """ | ||
| names = [] | ||
| for cfg in (hf_config, getattr(hf_config, "text_config", None)): | ||
| names.append(getattr(cfg, "model_type", None) or "") | ||
| names.extend(getattr(cfg, "architectures", None) or []) | ||
| return any("diffusiongemma" in name.lower().replace("_", "") for name in names) | ||
|
|
||
|
|
||
| def get_tokenizer(ckpt_path, trust_remote_code=False, **kwargs) -> PreTrainedTokenizerBase: | ||
| print(f"Initializing tokenizer from {ckpt_path}") | ||
|
|
||
|
|
@@ -696,6 +710,19 @@ def get_model( | |
| model_kwargs = config_kwargs.copy() | ||
| model_kwargs.setdefault("dtype", "auto") | ||
|
|
||
| # DiffusionGemma ties encoder/decoder weights. device_map "auto" (balanced) can split | ||
| # a tied pair across GPUs, leaving one side on the meta device and breaking generation. | ||
| # Sequential packs the model onto GPU 0 first (up to gpu_mem_percentage), keeping tied | ||
| # modules together for checkpoints that fit; larger ones can still spill and split a | ||
| # tied pair, and need an explicit single-device map. Multi-GPU only: a single-GPU split | ||
| # cannot separate a tied pair, and sequential would needlessly cap max_memory there. | ||
| if device != "cpu" and torch.cuda.device_count() > 1 and is_diffusion_gemma(hf_config): | ||
| print( | ||
| "Detected DiffusionGemma model. Using device_map='sequential'; the balanced " | ||
|
juhi10071998 marked this conversation as resolved.
|
||
| "'auto' mapping can split its tied encoder/decoder weights across GPUs." | ||
| ) | ||
| use_seq_device_map = True | ||
|
Comment on lines
+713
to
+724
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. [SUGGESTION] No escape hatch: this override is unconditional, so a multi-GPU DiffusionGemma user can no longer get The forced switch changes two things for every multi-GPU DiffusionGemma load — The Happy to defer this to a follow-up if you'd rather keep the bugfix minimal.
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 on the analysis, deferring to a follow-up. The distinction you draw is the right one: Keeping this PR scoped to the NVBug 6524370 fix. A tri-state flag is new CLI surface and deserves its own review; the README note belongs with it so the documented workaround matches whatever the flag ends up being. Tracking alongside the Noting the escape hatch that exists today: |
||
|
|
||
| if use_seq_device_map: | ||
| device_map = "sequential" | ||
| # If we use sequential, set max_memory limit to ensure that the model does not occupy the full GPU | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.