-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
[Frontend] Set MAX_AUDIO_CLIP_FILESIZE_MB via env var instead of hardcoding #21374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Frontend] Set MAX_AUDIO_CLIP_FILESIZE_MB via env var instead of hardcoding #21374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request successfully makes MAX_AUDIO_CLIP_FILESIZE_MB
configurable via an environment variable. However, the current implementation is not robust against invalid values for the environment variable, which could lead to a crash on startup. I've suggested a change to handle this case gracefully.
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
cc @NickLucche |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks a lot for contributing!
So the intended usage of SpeechToTextConfig
is to hold configs for speech-to-text models.
The way I am thinking about MAX_AUDIO_CLIP_FILESIZE_MB
is more from a server managing point of view, purely to limit the amount of bytes streamed and handle traffic/networking rates.
In my view this shouldn't be model-dependent so I would either stick with a configurable env var or a startup argument.
In both cases it would be great if you could add a line to the openai server docs, I am pretty sure I left a TODO.
Thanks for taking a look! Makes sense that this isn't per model config, just updated the PR to remove it from `SpeechToTextConfig. Are these the openai server docs? I can add a line! |
@NickLucche Just added in a note to |
Set VLLM_MAX_AUDIO_CLIP_FILESIZE_MB
0c54e1b
to
5c7b9bb
Compare
Signed-off-by: Deven Labovitch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just store the env var value once then we're good here
Signed-off-by: Deven Labovitch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT, thanks for your work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
Signed-off-by: Deven Labovitch <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]> Signed-off-by: 董巍 <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]> Signed-off-by: 董巍 <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]> Signed-off-by: avigny <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]> Signed-off-by: shuw <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]> Signed-off-by: x22x22 <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]>
…coding (vllm-project#21374) Signed-off-by: Deven Labovitch <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
Purpose
Set
MAX_AUDIO_CLIP_FILESIZE_MB
via env var inSpeechToTextConfig
instead of hardcoding to 25Test Plan
Startup vllm locally with and without
VLLM_MAX_AUDIO_CLIP_FILESIZE_MB
set. EnsureVLLM_MAX_AUDIO_CLIP_FILESIZE_MB
defaults to 25 when unsetTest Result
Passed