Speed up local vLLM ASR client requests#1487
Conversation
Patch httpcore's async connection-pool assignment hot path by default so high-concurrency OpenAI-compatible clients do not spend the event loop rebuilding connection-state lists. Signed-off-by: Dongji Gao <dongjig@nvidia.com>
Default local vLLM multimodal audio requests to file:// URLs instead of inline base64, while retaining the base64 path for external APIs and as an explicit fallback. Signed-off-by: Dongji Gao <dongjig@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an Changeshttpcore Connection Pool Monkeypatch
VLLMMultimodalModel audio_as_path Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Replace the connection-pool assignment hot path with a single-pass O(N+M) algorithm ported from encode/httpcore#1035 (still unmerged in 1.0.9). Categorize connections once, assign queued requests from the available bucket (popping each so a connection is never handed to two requests in one pass), then enforce keepalive. HTTP/2 pools fall back to the original since they need per-connection stream-capacity tracking. This removes the short-circuit scan-depth dependence of the prior patch (cost varied ~2x with where a free connection sat in the list), trading a slightly slower best case for consistent, lower worst-case cost. Signed-off-by: Dongji Gao <dongjig@nvidia.com>
Add direct VLLMMultimodalModel.__init__ tests (stubbing only the heavy VLLMModel base) for audio_as_path defaulting from mode/format and the validation that rejects path mode for external APIs / non-audio_url formats, plus unsupported-format rejection. Addresses PR review feedback that the existing fixtures bypass the constructor. Signed-off-by: Dongji Gao <dongjig@nvidia.com>
701cbf0 to
bddb482
Compare
|
KunalDhawan
left a comment
KunalDhawan
left a comment
There was a problem hiding this comment.
Great work @DongjiGao! Added some comments below, other than that LGTM
| raise ValueError(f"Unsupported audio_format '{audio_format}'. Use 'audio_url' or 'input_audio'.") | ||
| self.audio_format = audio_format | ||
| if audio_as_path is None: | ||
| audio_as_path = not self._external_api_mode and self.audio_format == "audio_url" |
There was a problem hiding this comment.
This defaults audio_as_path=True for all local audio_url requests, but vLLM rejects file:// media unless the server is launched with --allowed-local-media-path (it's disabled by default for security). serve_vllm.py (the command at L44–53) doesn't pass that flag, and it doesn't appear anywhere in the repo, so the default path here will fail at request time on a stock server, the same local-vLLM ASR path this PR is named for. The green e2e run likely had a server with the flag set manually, which hides this.
Could we either (a) have serve_vllm.py add --allowed-local-media-path (pointed at data_dir) when audio is in use, or (b) flip the default to audio_as_path=False so it's strictly opt-in, with the server requirement documented? Right now turning it on by default puts the burden on every user to also reconfigure the server.
| LOG.debug("httpcore is not installed; skipping async connection-pool patch") | ||
| return | ||
|
|
||
| original_assign = async_connection_pool.AsyncConnectionPool._assign_requests_to_connections |
There was a problem hiding this comment.
This function depends on httpcore 1.0.9 internals (_connections, _requests, _http2, _max_connections, _max_keepalive_connections, pool_request.is_queued(), connection.can_handle_request()). The only safeguard is the _nemo_skills_fast_assign double-patch marker, but there's no version check. If httpcore bumps, this will either silently change behavior or raise inside the event loop. Could we gate the patch on httpcore.__version__ (patch only the validated version, log-and-skip otherwise) or add some other guards?
There was a problem hiding this comment.
Thanks @KunalDhawan , both points make sense.
-
For the httpcore patch, I gated it to the validated version only (
httpcore==1.0.9) and log/skip otherwise, so we don't patch unknown private internals after a dependency bump. I also added a small unit test covering both the apply and skip cases. -
For
audio_as_path, agreed that defaulting tofile://is unsafe without--allowed-local-media-path. I flipped the default toFalse, kept it opt-in for localaudio_url, and updated the docstring/tests to call out the server requirement.
Keep file:// audio opt-in unless vLLM is explicitly configured for local media, and gate the private httpcore pool patch to the validated 1.0.9 internals. Signed-off-by: Dongji Gao <dongjig@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_httpcore_patch.py (1)
54-62: ⚡ Quick winConsider adding a test for double-patch prevention.
The patch function includes explicit logic to skip patching if
_nemo_skills_fast_assignis already set. A test that calls_patch_httpcore_connection_pool_assignment()twice on the same fake module would validate this guard works correctly.🧪 Example test implementation
def test_httpcore_patch_prevents_double_patching(monkeypatch): pool_cls = _install_fake_httpcore(monkeypatch, "1.0.9") _patch_httpcore_connection_pool_assignment() first_patched = pool_cls._assign_requests_to_connections _patch_httpcore_connection_pool_assignment() second_patched = pool_cls._assign_requests_to_connections assert first_patched is second_patched assert getattr(first_patched, "_nemo_skills_fast_assign", False) is True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_httpcore_patch.py` around lines 54 - 62, Add a new test function called test_httpcore_patch_prevents_double_patching that verifies the patch idempotency guard works correctly. The test should call _install_fake_httpcore to set up a fake module, call _patch_httpcore_connection_pool_assignment twice on the same pool class, and then assert that the patched _assign_requests_to_connections method returned after the first call is identical to the one returned after the second call, confirming that the second call did not re-patch the method. Also verify that the _nemo_skills_fast_assign attribute remains set to True.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_httpcore_patch.py`:
- Around line 54-62: Add a new test function called
test_httpcore_patch_prevents_double_patching that verifies the patch idempotency
guard works correctly. The test should call _install_fake_httpcore to set up a
fake module, call _patch_httpcore_connection_pool_assignment twice on the same
pool class, and then assert that the patched _assign_requests_to_connections
method returned after the first call is identical to the one returned after the
second call, confirming that the second call did not re-patch the method. Also
verify that the _nemo_skills_fast_assign attribute remains set to True.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c278ce7b-ac96-4d63-bc19-dcd2ac204575
📒 Files selected for processing (4)
nemo_skills/inference/generate.pynemo_skills/inference/model/vllm_multimodal.pytests/test_httpcore_patch.pytests/test_vllm_audio.py
🚧 Files skipped from review as they are similar to previous changes (2)
- nemo_skills/inference/generate.py
- tests/test_vllm_audio.py
Preserve httpcore's ability to replace expired connections in the same assignment pass, tolerate missing httpcore version metadata, and emit escaped file:// URIs for local audio paths. Signed-off-by: Dongji Gao <dongjig@nvidia.com>
Apply the fast assignment monkeypatch to the inspected 1.0.3-1.0.9 internals instead of only 1.0.9, while keeping older/different internals unpatched. Signed-off-by: Dongji Gao <dongjig@nvidia.com>
3 participants
@/tmp/tmp.HobmUsFmEc
Summary by CodeRabbit
Release Notes
New Features
audio_as_pathoption to vLLM multimodal models for more efficient handling of local audio files using URL references instead of inline encoding