fix: voxtralprocessor broken#3255
Conversation
📝 WalkthroughWalkthroughThe pull request adds VoxtralProcessor support to the axolotl framework. Changes include updating documentation and configuration examples to specify VoxtralProcessor, implementing early-return logic in the processor loader for VoxtralProcessor instantiation, and adding a comment to the Mistral3Processor class regarding planned removal. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
📖 Documentation Preview: https://6912c1bad2f283313408d456--resonant-treacle-0fd729.netlify.app Deployed on Netlify from commit 766b69f |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/multimodal.qmd(1 hunks)examples/voxtral/voxtral-mini-audio-qlora.yml(1 hunks)src/axolotl/loaders/processor.py(1 hunks)src/axolotl/utils/mistral/mistral3_processor.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T01:12:27.280Z
Learnt from: winglian
Repo: axolotl-ai-cloud/axolotl PR: 3019
File: src/axolotl/core/trainers/base.py:610-669
Timestamp: 2025-08-07T01:12:27.280Z
Learning: When a method has a TODO comment indicating it's a temporary override from upstream (like "TODO(wing): remove once https://github.com/huggingface/transformers/pull/39866/files is merged"), extensive refactoring suggestions may not be worthwhile since the code will be removed once the upstream changes are available.
Applied to files:
src/axolotl/utils/mistral/mistral3_processor.py
🧬 Code graph analysis (1)
src/axolotl/loaders/processor.py (1)
src/axolotl/utils/mistral/mistral_tokenizer.py (1)
HFMistralTokenizer(14-220)
⏰ 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). (7)
- GitHub Check: PyTest (3.11, 2.9.0)
- 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 from Source Dist (3.11, 2.7.1)
- GitHub Check: PyTest (3.11, 2.7.1)
- GitHub Check: preview
🔇 Additional comments (5)
examples/voxtral/voxtral-mini-audio-qlora.yml (1)
2-2: LGTM! Explicit processor specification for Voxtral.This change correctly specifies VoxtralProcessor to ensure proper loading of the Voxtral model, aligning with the corresponding loader logic changes.
src/axolotl/utils/mistral/mistral3_processor.py (1)
33-33: LGTM! Helpful context about temporary code.The comment appropriately documents that this processor is temporary and will be removed once transformers V5 changes are available upstream.
Based on learnings.
src/axolotl/loaders/processor.py (2)
49-53: LGTM! Cleaned up unused processor_kwargs.The removal of the unused
processor_kwargssimplifies the code while maintaining the necessarytokenizerandtrust_remote_codeparameters.
21-34: No issues found with the monkey patch implementation.The global replacement of
MistralCommonTokenizerwithHFMistralTokenizeris safe becauseHFMistralTokenizeris a permanent subclass that maintains the parent interface. All code throughout the codebase either:
- Imports dynamically after the patch opportunity, or
- Expects
HFMistralTokenizerexplicitly (e.g.,Mistral3Processor)No strict type identity checks exist, and the Liskov substitution principle is preserved. The patch is properly scoped within
load_processor()and only applied whencfg.tokenizer_use_mistral_commonis enabled.docs/multimodal.qmd (1)
127-128: LGTM! Documentation updated to reflect VoxtralProcessor requirement.The documentation correctly shows users how to configure Voxtral models with the explicit
processor_typespecification, consistent with the code changes and example configuration.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
Due to the Mistral3Processor addition, it broke Voxtral loading. This PR fixes that.
The patch also fixes the potential bug when we upgrade to transformers v5. https://github.com/huggingface/transformers/pull/41633/files
Motivation and Context
How has this been tested?
Manual runs across Magistral, Voxtral, Mistral3 to ensure running normally.
Screenshots (if appropriate)
Types of changes
Social Handles (Optional)
Summary by CodeRabbit
New Features
Documentation