Save processor in quantizer CLI#3290
Conversation
|
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 📝 WalkthroughWalkthroughThe quantize command module now supports multimodal model quantization by conditionally loading, saving, and syncing processors alongside models and tokenizers when multimodal configurations are present. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/axolotl/cli/quantize.py (1)
70-73: Multimodal processor loading looks correct, with a small robustness tweak possibleConditionally initializing
processorand callingload_processor(cfg, tokenizer)only whencfg.is_multimodalis true cleanly gates the new behavior and keeps the non‑multimodal path unchanged.To avoid any edge cases with truthiness on custom processor types, you might consider using an explicit
is not Nonecheck later:- processor = None - if cfg.is_multimodal: - processor = load_processor(cfg, tokenizer) + processor = None + if cfg.is_multimodal: + processor = load_processor(cfg, tokenizer)and pair it with:
- if processor: + if processor is not None:This is optional but removes any coupling to how
__len__/truthiness is implemented on processor classes.Please confirm that
cfg.is_multimodalis always defined in your quantization configs (or via defaults); otherwise you may want to guard it viagetattr(cfg, "is_multimodal", False).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/axolotl/cli/quantize.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/axolotl/cli/quantize.py (1)
src/axolotl/loaders/processor.py (1)
load_processor(17-82)
🪛 GitHub Actions: lint
src/axolotl/cli/quantize.py
[error] 1-1: pre-commit (ruff) failed. 1 issue fixed by the hook: removed unused import 'TorchAoConfig' from the imports in quantize.py. The hook exited with code 1 to indicate the modification.
🪛 Ruff (0.14.6)
src/axolotl/cli/quantize.py
116-116: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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). (6)
- GitHub Check: PyTest from Source Dist (3.11, 2.7.1)
- GitHub Check: PyTest from Source Dist (3.11, 2.8.0)
- GitHub Check: PyTest from Source Dist (3.11, 2.9.0)
- GitHub Check: PyTest (3.11, 2.8.0)
- GitHub Check: PyTest (3.11, 2.9.0)
- GitHub Check: PyTest (3.11, 2.7.1)
🔇 Additional comments (3)
src/axolotl/cli/quantize.py (3)
8-11: Imports align with the new multimodal processor flowBringing
load_processorinto the CLI and wiring it next toload_tokenizeris consistent with how processors are handled elsewhere; nothing blocking here from a dependency/wiring standpoint.If you haven’t already, re-run your pre-commit hooks (
ruffin particular) to ensure there are no new import-related lint issues with the added symbols.
126-127: Processorpush_to_hubmirrors model/tokenizer behavior appropriatelyConditionally pushing the processor to the same
hub_model_idwhen present keeps the artifact set on the Hub in sync for multimodal models and does not affect the non‑multimodal path.No changes needed here.
It’s worth verifying with at least one multimodal config (e.g., one that uses
load_processorinaxolotl.loaders.processor) thatprocessor.push_to_hubworks as expected and that all three artifacts (model, tokenizer, processor) appear under the same repo on the Hub.
115-118: RUF010 is not enabled in this project's Ruff configurationThe Ruff configuration in
pyproject.tomlselects only["E", "F", "W", "C90", "B", "I"]rules, which does not include the "RUF" ruleset. RUF010 (explicit-f-string-type-conversion) is part of the RUF rules and is therefore not being flagged by Ruff in CI or pre-commit hooks. The code at lines 116 and 130 usingstr(Path(...))inside f-strings is not currently triggering a lint error in this project.The suggested refactors are stylistically sound, but there is no existing lint violation to fix.
Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.