Remove audio optional dependency for mistral-common #28722
Conversation
Signed-off-by: Julien Denize <[email protected]>
github-actions
bot
added
the
ready
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| msgspec | ||
| gguf >= 0.13.0 | ||
| mistral_common[image,audio] >= 1.8.5 | ||
| mistral_common[image] >= 1.8.5 |
There was a problem hiding this comment.
Removing mistral_common[audio] from the default requirements means packages like librosa and soundfile are no longer installed when setting up the repo via requirements/common.txt. Several audio tests import librosa unconditionally (e.g. tests/multimodal/test_audio.py, tests/models/multimodal/generation/test_phi4_multimodal.py, tests/entrypoints/openai/test_transcription_validation.py), so the test suite now raises ModuleNotFoundError before those modules can mark themselves skipped. Unless the audio tests are explicitly split into an optional suite, the dependency should stay in the base requirements or the tests must guard their imports, otherwise CI and developers running the default setup will be unable to execute the audio-related tests or demos.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Test dependencies kept audio optional dependency.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes audio dependencies optional by removing [audio] from the mistral_common requirement. While this is a good initiative to keep the base installation lean, the current implementation is incomplete and will break audio functionality for users. I've left a critical comment on requirements/common.txt detailing the necessary accompanying changes, which include setting up an [audio] extra for installation and updating the documentation to guide users. Without these, the change would be a silent breaking change.
Signed-off-by: Julien Denize <[email protected]>
Head branch was pushed to by a user without write access
|
Documentation preview: https://vllm--28722.org.readthedocs.build/en/28722/ |
|
@DarkLight1337 sorry added some docs as we removed the dependency to inform users. |
Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]> Signed-off-by: Julien Denize <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: George D. Torres <[email protected]>
Signed-off-by: Julien Denize <[email protected]> Signed-off-by: Julien Denize <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Co-authored-by: Cyrus Leung <[email protected]> Signed-off-by: Bram Wasti <[email protected]>
3 participants
Purpose
Remove audio optional dependency from mistral-common to fix #8030.
I left it to tests to ensure Voxtral can be tested.
LMK @DarkLight1337 if it works out for you.
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.