Update the LTX2 API calls during the calibration#926
Conversation
Signed-off-by: Jingyu Xin <jingyux@nvidia.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes update the LTX2 pipeline configuration by replacing the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/diffusers/quantization/calibration.py (1)
123-142: Avoid sharing mutable guider defaults across batches.If
DEFAULT_*_GUIDER_PARAMSare mutable (e.g., dict/dataclass), passing the same instance each call risks cross-batch mutation. Consider cloning per call.🔧 Suggested change
+ from copy import deepcopy kwargs = { @@ - "video_guider_params": DEFAULT_VIDEO_GUIDER_PARAMS, - "audio_guider_params": DEFAULT_AUDIO_GUIDER_PARAMS, + "video_guider_params": deepcopy(DEFAULT_VIDEO_GUIDER_PARAMS), + "audio_guider_params": deepcopy(DEFAULT_AUDIO_GUIDER_PARAMS),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/diffusers/quantization/calibration.py` around lines 123 - 142, The code passes the shared mutable defaults DEFAULT_VIDEO_GUIDER_PARAMS and DEFAULT_AUDIO_GUIDER_PARAMS directly into kwargs (used in prompt batching), risking cross-batch mutation; change the call site where kwargs is built (the block creating "kwargs") to create per-call copies of those defaults (e.g., shallow or deep copy as appropriate) before assigning to "video_guider_params" and "audio_guider_params" so each batch gets its own independent instance and mutations do not leak between batches.examples/diffusers/quantization/models_utils.py (1)
166-167: Keep LTX2 resolution defaults single-sourced.Now that LTX2 defaults are 768×1280, consider centralizing any fallback usage (e.g., in calibration) to avoid drift if values change again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/diffusers/quantization/models_utils.py` around lines 166 - 167, Extract the hard-coded LTX2 resolution (height: 768, width: 1280) into a single shared constant (e.g., LTX2_DEFAULT_RESOLUTION or LTX2_DEFAULT_HEIGHT and LTX2_DEFAULT_WIDTH) in models_utils.py and replace all ad-hoc fallbacks in the codebase (particularly any calibration routines that currently reference literal 768/1280) to reference that constant; ensure the constant is exported/imported where needed so calibration and model init code use the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/diffusers/quantization/pipeline_manager.py`:
- Around line 216-218: The current assignment to fp8_quantization uses "or"
which causes an explicit False for the new flag to be overridden by the legacy
fp8transformer key; change the logic so the explicit new flag wins: check
whether "fp8_quantization" exists in params and use
params.pop("fp8_quantization") if present, otherwise fall back to
params.pop("fp8transformer", False). Update the code that sets the
fp8_quantization variable (and remove the single-line or expression) so the
precedence is: explicit fp8_quantization > legacy fp8transformer.
---
Nitpick comments:
In `@examples/diffusers/quantization/calibration.py`:
- Around line 123-142: The code passes the shared mutable defaults
DEFAULT_VIDEO_GUIDER_PARAMS and DEFAULT_AUDIO_GUIDER_PARAMS directly into kwargs
(used in prompt batching), risking cross-batch mutation; change the call site
where kwargs is built (the block creating "kwargs") to create per-call copies of
those defaults (e.g., shallow or deep copy as appropriate) before assigning to
"video_guider_params" and "audio_guider_params" so each batch gets its own
independent instance and mutations do not leak between batches.
In `@examples/diffusers/quantization/models_utils.py`:
- Around line 166-167: Extract the hard-coded LTX2 resolution (height: 768,
width: 1280) into a single shared constant (e.g., LTX2_DEFAULT_RESOLUTION or
LTX2_DEFAULT_HEIGHT and LTX2_DEFAULT_WIDTH) in models_utils.py and replace all
ad-hoc fallbacks in the codebase (particularly any calibration routines that
currently reference literal 768/1280) to reference that constant; ensure the
constant is exported/imported where needed so calibration and model init code
use the single source of truth.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/diffusers/quantization/calibration.pyexamples/diffusers/quantization/models_utils.pyexamples/diffusers/quantization/pipeline_manager.py
| fp8_quantization = params.pop("fp8_quantization", None) or params.pop( | ||
| "fp8transformer", False | ||
| ) |
There was a problem hiding this comment.
Explicit fp8_quantization=false can be overridden by legacy flag.
Using or means an explicit False is ignored if fp8transformer is truthy. Prefer “explicit new flag wins” semantics.
✅ Safer precedence
- fp8_quantization = params.pop("fp8_quantization", None) or params.pop(
- "fp8transformer", False
- )
+ if "fp8_quantization" in params:
+ fp8_quantization = params.pop("fp8_quantization")
+ else:
+ fp8_quantization = params.pop("fp8transformer", False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/diffusers/quantization/pipeline_manager.py` around lines 216 - 218,
The current assignment to fp8_quantization uses "or" which causes an explicit
False for the new flag to be overridden by the legacy fp8transformer key; change
the logic so the explicit new flag wins: check whether "fp8_quantization" exists
in params and use params.pop("fp8_quantization") if present, otherwise fall back
to params.pop("fp8transformer", False). Update the code that sets the
fp8_quantization variable (and remove the single-line or expression) so the
precedence is: explicit fp8_quantization > legacy fp8transformer.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #926 +/- ##
=======================================
Coverage 73.09% 73.09%
=======================================
Files 205 205
Lines 22301 22301
=======================================
Hits 16300 16300
Misses 6001 6001 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "width": 1280, | ||
| "num_frames": 121, | ||
| "frame_rate": 24.0, | ||
| "cfg_guidance_scale": 4.0, |
There was a problem hiding this comment.
Does it mean users cannot specify cfg?
There was a problem hiding this comment.
they can, but in our calibration script we leave it to the default inference parameters in LTX-2:
DEFAULT_AUDIO_GUIDER_PARAMS,
DEFAULT_VIDEO_GUIDER_PARAMS,
these 2 params contain the default cfg_guidance_scale value. We haven't got any related request for calibrating different guidance scale yet.
2 participants
What does this PR do?
Type of change: Bug fix
Overview:
Update LTX-2 integration to match latest upstream API
Usage
Testing
Before your PR is "Ready for review"
Additional Information
Summary by CodeRabbit
Release Notes