Fixed the CICD for diffusers #312
Conversation
|
Warning Rate limit exceeded@jingyu-ml has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds an optional override_model_path parameter to PipelineManager.create_pipeline_from and threads it from diffusion_trt.py; model_id resolution prefers the override when present. Also adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as diffusion_trt.py
participant PM as PipelineManager.create_pipeline_from
participant DP as Diffusers Pipeline
Note over CLI,PM: CLI may receive --override-model-path
User->>CLI: Run script (with optional --override-model-path)
CLI->>PM: create_pipeline_from(model_type, dtype, override_model_path)
alt override_model_path provided
PM->>PM: model_id = override_model_path
else no override
PM->>PM: model_id = MODEL_REGISTRY[model_type]
end
PM->>DP: Instantiate pipeline with model_id and dtype
DP-->>PM: Pipeline instance
PM-->>CLI: Return pipeline
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
jingyu-ml
force-pushed
the
jingyux/fixed-trtexec-cicd
branch
from
7b291bf to
5d4befc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/diffusers/quantization/quantize.py (1)
938-943: Bug: always passing class attribute for quantize_mha (ignores CLI flag).
QuantizationConfig.quantize_mhais a class attribute default; this will always be False and won’t reflect the user’s choice. Use the instance value fromquant_config.Apply this diff:
export_manager.export_onnx( pipe, backbone, model_config.model_type, quant_config.format, - quantize_mha=QuantizationConfig.quantize_mha, + quantize_mha=quant_config.quantize_mha, )
🧹 Nitpick comments (3)
examples/diffusers/quantization/quantize.py (2)
326-328: Guard against empty-string override_model_path.Treating "" as “no override” avoids surprising failures when an empty value gets wired in.
Apply this diff:
- model_id = ( - MODEL_REGISTRY[model_type] if override_model_path is None else override_model_path - ) + model_id = ( + MODEL_REGISTRY[model_type] if not override_model_path else override_model_path + )
342-343: Redundant try/except that just re-raises.Catching and re-raising loses context in logs and adds noise.
Apply this diff to simplify:
- try: + try: model_id = ( MODEL_REGISTRY[model_type] if override_model_path is None else override_model_path ) ... return pipe - except Exception as e: - raise e + except Exception: + raiseexamples/diffusers/quantization/diffusion_trt.py (1)
108-112: Minor readability: avoid mixing positional and keyword args.Using keywords for all args improves clarity and future-proofing.
Apply this diff:
- pipe = PipelineManager.create_pipeline_from( - MODEL_ID[args.model], - dtype_map[args.model_dtype], - override_model_path=args.override_model_path, - ) + pipe = PipelineManager.create_pipeline_from( + model_type=MODEL_ID[args.model], + torch_dtype=dtype_map[args.model_dtype], + override_model_path=args.override_model_path, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/diffusers/quantization/diffusion_trt.py(1 hunks)examples/diffusers/quantization/quantize.py(2 hunks)examples/diffusers/quantization/requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/diffusers/quantization/diffusion_trt.py (1)
examples/diffusers/quantization/quantize.py (2)
PipelineManager(294-433)create_pipeline_from(311-343)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docs
- GitHub Check: code-quality
🔇 Additional comments (3)
examples/diffusers/quantization/requirements.txt (1)
2-2: Confirm CI installs transformers & accelerate alongside diffusers
diffusers 0.34.0 includes FluxPipeline and StableDiffusion3Pipeline, and does not strictly pin transformers/accelerate versions—ensure your CI setup installs or upgrades both packages per the official installation guide to avoid runtime errors.examples/diffusers/quantization/quantize.py (1)
312-315: New override_model_path parameter — LGTM.Signature and typing are appropriate; matches downstream usage.
examples/diffusers/quantization/diffusion_trt.py (1)
108-112: Plumbing override_model_path through — LGTM.The call matches the updated signature; types align.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
=======================================
Coverage 73.87% 73.87%
=======================================
Files 172 172
Lines 17439 17439
=======================================
Hits 12883 12883
Misses 4556 4556 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -1,4 +1,5 @@ | |||
| cuda-python | |||
| diffusers==0.34.0 | |||
There was a problem hiding this comment.
why do we need this specific version only?
There was a problem hiding this comment.
diffusers changed its layer definition. Until we find a solution, let’s downgrade to the previous version.
#262
|
@jingyu-ml commits are not signed hence merging will be blocked |
jingyu-ml
force-pushed
the
jingyux/fixed-trtexec-cicd
branch
3 times, most recently
from
31c84a8 to
4bc7ed7
Compare
jingyu-ml
requested review from
ynankani,
gcunhase,
i-riyad and
kevalmorabia97
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
jingyu-ml
force-pushed
the
jingyux/fixed-trtexec-cicd
branch
from
4bc7ed7 to
b353787
Compare
Signed-off-by: jingyu <[email protected]> Signed-off-by: Jingyu Xin <[email protected]>
Signed-off-by: Jingyu Xin <[email protected]>
Signed-off-by: Jingyu Xin <[email protected]>
jingyu-ml
force-pushed
the
jingyux/fixed-trtexec-cicd
branch
from
2b03e6c to
43ed09f
Compare
Signed-off-by: jingyu <[email protected]> Signed-off-by: Jingyu Xin <[email protected]>
What does this PR do?
Type of change: Minor code change
Overview: Fixed the CI/CD for diffusers
Usage
Testing
Before your PR is "Ready for review"
Additional Information
Summary by CodeRabbit
New Features
Chores