Reduce connection-pool warnings in the vLLM client

[vladbataev]

What does this PR do?

VLLMClient dispatches up to 64 concurrent requests for chat, image preprocessing, and generation from image features, but its HTTP adapter retains only 10 connections per host by default. When connections return to a full pool, urllib3 closes them and logs Connection pool is full, discarding connection.

Size the pool to match the existing concurrency defaults using a shared constant, and add regression tests that check the actual HTTP and HTTPS pool capacities.

The intended benefit is fewer pool-full warnings and less unnecessary connection churn. No training speedup is claimed; training throughput and step time have not been measured.

Related to #5361, where this pool-size mismatch was investigated. This PR does not claim to resolve the reported training hang.

Validation

• Controlled local HTTP smoke test with two batches of 64 concurrent chat requests: 108 pool-full warnings before, 0 after. New TCP connections in the second batch fell from 54 to 0. This was a transport check, not a training benchmark.
• pytest tests/test_vllm_client_server.py -q: 7 passed, 35 skipped; vLLM is not installed locally.
• Both new protocol cases fail when the pool-size argument is removed in memory.
• Ruff lint, formatting, and git diff --check passed.

Before submitting

• Read the contributor guidelines.
• Linked the related investigation: Multimodal generation hangs in vllm server mode with large images (works in colocate), manual resizing required #5361.
• Reviewed documentation needs; no public API or default-concurrency changes require documentation updates.
• Added regression tests.

AI writing disclosure

• No AI usage: the PR was written entirely by a human.
• AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.
• AI-generated: the PR was mostly or fully generated by an AI tool.

---

Note

Low Risk
Transport-layer tuning only; default concurrency behavior is unchanged and covered by new regression tests.

Overview
Aligns the vLLM HTTP client’s urllib3 connection pool with its default batch concurrency so pooled connections are not discarded under normal load.

VLLMClient now mounts HTTPAdapter with pool_maxsize set to a shared DEFAULT_MAX_CONCURRENT_REQUESTS (64) constant, matching the default max_concurrent_requests on image_features, _generate_from_features, and chat. Those methods use the constant for their defaults instead of a hard-coded 64.

Adds TestConnectionPoolSize (mocked client, parametrized over http and https) to assert each mounted pool’s maxsize is at least the maximum default concurrency across those methods.

^{Reviewed by Cursor Bugbot for commit dddbe18. Bugbot is set up for automated code reviews on this repo. Configure here.}

[vladbataev]

Hi @qgallouedec, could you review the PR when you have a chance? It’s a small vLLM client pool-sizing fix to reduce pool-full warnings, with regression tests included.

[albertvillanova]

Closing: we don't review fully AI-generated PRs from first-time contributors, see the AI usage policy.

[vladbataev]

@albertvillanova I marked this AI-generated to disclose its origin accurately. I’ve read and reviewed the implementation: the client already dispatches up to 64 concurrent requests, and the change aligns the connection pool’s retained capacity with that default.

I understand the concern about contributors being unable to explain their changes. I can discuss this implementation and address review feedback. Would you consider reopening it for review?

[albertvillanova]

Reopening. Thanks for engaging, and for disclosing the origin accurately rather than under-reporting it.

The concern behind that policy is the case where a contributor cannot discuss the change or vouch for it. Your description matches the code (the client methods default to 64 concurrent requests while the mounted adapter keeps the default pool of 10), so that concern does not apply here.

I will review it shortly. The expectation from here is the usual one: you address the review comments yourself.

[bot-ci-comment]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[albertvillanova]

Thanks, this holds up. I checked the premise rather than the prose, and it is accurate: requests mounts HTTPAdapter with pool_maxsize=10 and pool_block=False, so when image_features, _generate_from_features and chat each fan out to 64 threads against a single host, urllib3 opens 64 connections, returns 10 to the pool and closes the other 54 with the warning from HTTPConnectionPool._put_conn. Nothing in the repo passes max_concurrent_requests, so 64 is what actually runs. Your 108 warnings over two batches and 54 fresh connections in the second batch are exactly what that arithmetic predicts. The server-mode client is built on the main process only, so this trades 10 idle sockets for 64 on one process rather than per rank. Thanks also for keeping #5361 as context rather than a claimed fix; that one is about base64 image payloads and is a separate problem.

Three things before this can go in.

1. get_sequence_logprobs keeps a literal 4. The constant is named DEFAULT_MAX_CONCURRENT_REQUESTS, but get_sequence_logprobs has the same parameter and stays at 4. Was leaving it out deliberate, and what is the reasoning? I have a view, but I would rather hear yours first, because it decides the shape of the change: a constant with that name covering three of the four is the one outcome to avoid. Either it is genuinely the default for every max_concurrent_requests and that method adopts it too, or the two numbers mean different things and the constant should be named for what it actually sizes.

2. The test asserts against itself. It reads the expected value out of inspect.signature(...).parameters["max_concurrent_requests"].default, so the pass condition is derived from the same source lines it is guarding. Lower those defaults to 5 and the test still passes against a pool of 64; rename or drop the parameter and it fails with a KeyError rather than a useful message. Assert the number instead: assert pool.pool.maxsize >= 64. It is shorter, it is readable without knowing the introspection API, it says what the test is actually for, and it removes the need to reach into the private _generate_from_features from a test. This repo prefers the blunt version of a check over the clever one.

3. Constant naming. The two existing module-level constants in this file are _HAS_STATEFUL_TRAINER_ENGINE and _HAS_WEIGHT_UPDATE_LIFECYCLE. If the constant survives point 1, prefix it to match.

Minor, take or leave: one HTTPAdapter instance is mounted under both http:// and https://, so both parametrized cases read pool_maxsize off the same object and the second adds little.