[PR #173] feat(client,transport): expose httpx connection-pool limits on Comfy/AsyncComfy #173

New issue

Open

opened 2026-10-31 22:02:01 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:02:01 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/173
Author: @mattmillerai
Created: 9/19/2026
Status: 🔄 Open

Base: main ← Head: matt/be-15952-httpx-limits

📝 Commits (3)

2bf66df feat(client,transport): expose httpx connection-pool limits
4d51c44 Merge remote-tracking branch 'origin/main' into matt/be-15952-httpx-limits
9a3af4e fix(tests): reconcile the credits_used tripwire now that the spec declares it

📊 Changes

6 files changed (+129 additions, -32 deletions)

View changed files

📝 CHANGELOG.md (+6 -0)
📝 README.md (+17 -0)
📝 src/comfy_low/transport.py (+26 -2)
📝 src/comfy_sdk/client.py (+23 -0)
📝 tests/test_models_namespace.py (+48 -0)
📝 tests/test_router_spec_contract.py (+9 -30)

📄 Description

ELI-5

models.run is awaited server-side: the server holds the connection open until the generation finishes. That connection comes out of the client's httpx pool, and the pool has always been httpx's default — max_connections=100 — with no way to change it from the public surface.

So a caller fanning out more than 100 concurrent runs did not get 101 runs. It got 100 runs and one call sitting in httpx's pool queue, waiting for a generation to finish. If nothing freed up, that call waited out the run timeout and raised httpx.PoolTimeout — a ten-minute stall ending in an error that comfy_sdk.retry's fast class (60s elapsed budget) will not retry.

The lever is httpx.Limits, and now it is reachable:

client = Comfy(limits=httpx.Limits(max_connections=250))

What changed

Comfy / AsyncComfy take a keyword-only limits: httpx.Limits | None = None, positioned after timeout, and pass it down to ComfyLow / AsyncComfyLow. Both class docstrings say what it is for and why a models.run fan-out is the case that needs it.
ComfyLow / AsyncComfyLow take the same keyword and hand it to the httpx.Client / httpx.AsyncClient they build.
None is dropped rather than forwarded. httpx.Client types limits as a Limits (default DEFAULT_LIMITS) and does not accept None, so passing it through would break the default path. The constructor builds a kwargs dict and only sets limits when one was given — which keeps the default exactly httpx's own, rather than importing the private httpx._config.DEFAULT_LIMITS to restate it.
An injected client= still owns its pool. limits is ignored on that path, exactly as timeout already is, and both __init__ docstrings say so.
README gains a paragraph directly after the run-timeout paragraph, and CHANGELOG an Unreleased / Added entry.

Tests

Five new cases in tests/test_models_namespace.py, next to the existing timeout view tests — no httpx mocks, they construct real clients against the server fixture stub:

Comfy(limits=httpx.Limits(max_connections=250)) and the AsyncComfy equivalent build a transport whose pool reports 250.
Comfy() and AsyncComfy() still report 100, which pins "None keeps httpx's default" rather than leaving it as an assumption about a code path nothing exercises.
ComfyLow(base_url, key, client=httpx.Client(), limits=httpx.Limits(max_connections=5)) keeps the injected client's pool — the transport object is the injected one and its pool still reports 100, not 5.

These read client._low._client._transport._pool._max_connections, which is httpcore internals. That is deliberate and it is the judgment call in this PR: httpx exposes no public getter for the limits a client was built with, so the alternative is asserting nothing about whether limits= arrived at all. The comment above the block says as much. A read-only limits view on the transport would remove the need, but it would mean caching the constructor value, and it is out of scope here.

Notes for the reviewer

The public API grows by one optional keyword on four constructors and nothing else changes. Both clients gained it, so test_paired_methods_take_the_same_parameters stays green; the default path is byte-identical in behaviour to before.
The README quotes ten minutes deliberately. On this base the pool timeout is MODEL_RUN_TIMEOUT's 600s, because the single positional argument to httpx.Timeout(600.0, connect=10.0) sets pool along with read and write. The open PR that lifts that run timeout to 660s would move this figure to eleven minutes — and the paragraph immediately above mine, which already says "10-minute timeout", moves with it. They are adjacent on purpose so a future edit catches both.
This is additive only. It denies no capability, adds no refusal path and flips no test to assert a dead end, so the negative-claim falsification check does not apply; there was no capability to go looking for before shipping a denial.
Several open PRs also touch README.md. This paragraph is additive and sits in the models.run section; whichever lands first, the other rebases cleanly or needs a one-hunk resolution.

Provenance

Authored by: agent-work loop
Verified: uv run --extra dev pytest: 980 passed, 9 skipped, 0 failed (the 9 skips are the network-gated e2e module); ruff check .: all checks passed; ruff format --check .: 57 files already formatted; mypy src: no issues in 21 source files; python3 scripts/check_public_repo_hygiene.py: no internal-only references. scripts/check_drift.py was not run — no generated model, spec file or Router error type is touched by this diff.
Deviations: none — every acceptance criterion is met. See Residual for what was deliberately left out of scope and what could not be exercised here.

Residual

Not fixed by this PR, each of them named as out of scope by the work that prompted it:

No read-only limits view on ComfyLow / AsyncComfyLow or client.models. The other transport settings (base_url, timeout, authenticated) have read-through properties and this one does not, which is an asymmetry a reader will notice. httpx exposes no public getter for a client's pool limits, so a view means caching the constructor value — a second source of truth for something the injected-client path deliberately does not control. Left out until a reviewer asks for it. Its direct cost is visible in this PR: the tests have to reach into httpcore internals for the assertion.
assets.py's per-call httpx.AsyncClient in from_url is untouched. It builds its own client per call and so never uses the shared pool; limits does not reach it. If a large concurrent from_url fan-out ever matters, that is a separate change (it should probably use the shared transport rather than gain its own limits).
MODEL_RUN_TIMEOUT.pool is unchanged. Whether the pool-acquisition wait should be generation-scale at all — rather than short, so a saturated pool fails fast inside the retry budget instead of stalling for minutes — is the open question this parameter routes around rather than answers. It is settled as a deliberate trade elsewhere and is being changed in another open PR; this PR does not touch it.

Unexercised artifacts:

No live fan-out was run. The claim that >100 concurrent models.run calls queue and then raise PoolTimeout is not reproduced here: the suite is network-free by design (a stdlib stub server, and this repo's guidance forbids httpx mocks), and a real reproduction needs 100+ concurrent generations against Router — a costly, account-billing action, so it was not run. What is verified is the mechanism: the pool the client builds reports the ceiling it was given, and the default is still 100. The failure mode itself is carried over from the review thread that raised it, not re-measured.
The originating investigation's findings were not read directly. This PR was built from the plan handed to it; the underlying research notes are on a tracker this branch has no access to.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-python-sdk/pull/173 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-15952-httpx-limits` --- ### 📝 Commits (3) - [`2bf66df`](https://github.com/Comfy-Org/comfy-python-sdk/commit/2bf66dfa3ba1d658be164741774a1becd68dd4e8) feat(client,transport): expose httpx connection-pool limits - [`4d51c44`](https://github.com/Comfy-Org/comfy-python-sdk/commit/4d51c44ac50612510b0ed4dd53ce34377637126f) Merge remote-tracking branch 'origin/main' into matt/be-15952-httpx-limits - [`9a3af4e`](https://github.com/Comfy-Org/comfy-python-sdk/commit/9a3af4e5f49084f735694d33a2a2dba67e499037) fix(tests): reconcile the credits_used tripwire now that the spec declares it ### 📊 Changes **6 files changed** (+129 additions, -32 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+6 -0) 📝 `README.md` (+17 -0) 📝 `src/comfy_low/transport.py` (+26 -2) 📝 `src/comfy_sdk/client.py` (+23 -0) 📝 `tests/test_models_namespace.py` (+48 -0) 📝 `tests/test_router_spec_contract.py` (+9 -30) </details> ### 📄 Description ## ELI-5 `models.run` is awaited server-side: the server holds the connection open until the generation finishes. That connection comes out of the client's httpx pool, and the pool has always been httpx's default — `max_connections=100` — with no way to change it from the public surface. So a caller fanning out more than 100 concurrent runs did not get 101 runs. It got 100 runs and one call sitting in httpx's pool queue, waiting for a generation to finish. If nothing freed up, that call waited out the run timeout and raised `httpx.PoolTimeout` — a ten-minute stall ending in an error that `comfy_sdk.retry`'s fast class (60s elapsed budget) will not retry. The lever is `httpx.Limits`, and now it is reachable: ```python client = Comfy(limits=httpx.Limits(max_connections=250)) ``` ## What changed - **`Comfy` / `AsyncComfy` take a keyword-only `limits: httpx.Limits | None = None`,** positioned after `timeout`, and pass it down to `ComfyLow` / `AsyncComfyLow`. Both class docstrings say what it is for and why a `models.run` fan-out is the case that needs it. - **`ComfyLow` / `AsyncComfyLow` take the same keyword** and hand it to the `httpx.Client` / `httpx.AsyncClient` they build. - **`None` is dropped rather than forwarded.** `httpx.Client` types `limits` as a `Limits` (default `DEFAULT_LIMITS`) and does not accept `None`, so passing it through would break the default path. The constructor builds a kwargs dict and only sets `limits` when one was given — which keeps the default exactly httpx's own, rather than importing the private `httpx._config.DEFAULT_LIMITS` to restate it. - **An injected `client=` still owns its pool.** `limits` is ignored on that path, exactly as `timeout` already is, and both `__init__` docstrings say so. - **README** gains a paragraph directly after the run-timeout paragraph, and **CHANGELOG** an `Unreleased` / `Added` entry. ## Tests Five new cases in `tests/test_models_namespace.py`, next to the existing `timeout` view tests — no httpx mocks, they construct real clients against the `server` fixture stub: - `Comfy(limits=httpx.Limits(max_connections=250))` and the `AsyncComfy` equivalent build a transport whose pool reports `250`. - `Comfy()` and `AsyncComfy()` still report `100`, which pins "`None` keeps httpx's default" rather than leaving it as an assumption about a code path nothing exercises. - `ComfyLow(base_url, key, client=httpx.Client(), limits=httpx.Limits(max_connections=5))` keeps the injected client's pool — the transport object is the injected one and its pool still reports `100`, not `5`. These read `client._low._client._transport._pool._max_connections`, which is httpcore internals. That is deliberate and it is the judgment call in this PR: httpx exposes no public getter for the limits a client was built with, so the alternative is asserting nothing about whether `limits=` arrived at all. The comment above the block says as much. A read-only `limits` view on the transport would remove the need, but it would mean caching the constructor value, and it is out of scope here. ## Notes for the reviewer - **The public API grows by one optional keyword on four constructors** and nothing else changes. Both clients gained it, so `test_paired_methods_take_the_same_parameters` stays green; the default path is byte-identical in behaviour to before. - **The README quotes ten minutes deliberately.** On this base the pool timeout is `MODEL_RUN_TIMEOUT`'s 600s, because the single positional argument to `httpx.Timeout(600.0, connect=10.0)` sets `pool` along with `read` and `write`. The open PR that lifts that run timeout to 660s would move this figure to eleven minutes — and the paragraph immediately above mine, which already says "10-minute timeout", moves with it. They are adjacent on purpose so a future edit catches both. - **This is additive only.** It denies no capability, adds no refusal path and flips no test to assert a dead end, so the negative-claim falsification check does not apply; there was no capability to go looking for before shipping a denial. - Several open PRs also touch `README.md`. This paragraph is additive and sits in the `models.run` section; whichever lands first, the other rebases cleanly or needs a one-hunk resolution. ## Provenance - **Authored by:** agent-work loop - **Verified:** `uv run --extra dev pytest`: 980 passed, 9 skipped, 0 failed (the 9 skips are the network-gated e2e module); `ruff check .`: all checks passed; `ruff format --check .`: 57 files already formatted; `mypy src`: no issues in 21 source files; `python3 scripts/check_public_repo_hygiene.py`: no internal-only references. `scripts/check_drift.py` was not run — no generated model, spec file or Router error type is touched by this diff. - **Deviations:** none — every acceptance criterion is met. See Residual for what was deliberately left out of scope and what could not be exercised here. ## Residual Not fixed by this PR, each of them named as out of scope by the work that prompted it: - **No read-only `limits` view on `ComfyLow` / `AsyncComfyLow` or `client.models`.** The other transport settings (`base_url`, `timeout`, `authenticated`) have read-through properties and this one does not, which is an asymmetry a reader will notice. httpx exposes no public getter for a client's pool limits, so a view means caching the constructor value — a second source of truth for something the injected-client path deliberately does not control. Left out until a reviewer asks for it. Its direct cost is visible in this PR: the tests have to reach into httpcore internals for the assertion. - **`assets.py`'s per-call `httpx.AsyncClient` in `from_url` is untouched.** It builds its own client per call and so never uses the shared pool; `limits` does not reach it. If a large concurrent `from_url` fan-out ever matters, that is a separate change (it should probably use the shared transport rather than gain its own `limits`). - **`MODEL_RUN_TIMEOUT.pool` is unchanged.** Whether the pool-acquisition wait should be generation-scale at all — rather than short, so a saturated pool fails fast inside the retry budget instead of stalling for minutes — is the open question this parameter routes around rather than answers. It is settled as a deliberate trade elsewhere and is being changed in another open PR; this PR does not touch it. Unexercised artifacts: - **No live fan-out was run.** The claim that >100 concurrent `models.run` calls queue and then raise `PoolTimeout` is not reproduced here: the suite is network-free by design (a stdlib stub server, and this repo's guidance forbids httpx mocks), and a real reproduction needs 100+ concurrent generations against Router — a costly, account-billing action, so it was not run. What is verified is the mechanism: the pool the client builds reports the ceiling it was given, and the default is still 100. The failure mode itself is carried over from the review thread that raised it, not re-measured. - **The originating investigation's findings were not read directly.** This PR was built from the plan handed to it; the underlying research notes are on a tracker this branch has no access to. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-31 22:02:01 +00:00