feat(client,transport): expose httpx connection-pool limits on Comfy/AsyncComfy - #173
mattmillerai wants to merge 1 commit into
Conversation
`models.run` is awaited server-side, so each concurrent run holds its pooled connection for the whole generation. The pool was always httpx's default (`max_connections=100`) and nothing on the public surface could change it, so a caller fanning out past 100 concurrent runs had its 101st call queue behind a generation and — if nothing freed up — end in `httpx.PoolTimeout` after the run timeout rather than in a result. `Comfy`/`AsyncComfy` (and `ComfyLow`/`AsyncComfyLow` underneath) now take a keyword-only `limits: httpx.Limits | None = None`. `None` is dropped rather than forwarded — httpx types the parameter as a `Limits` and does not accept `None` — so the default path stays httpx's own default exactly as before. An injected `client=` owns its pool and ignores `limits`, as it already does `timeout`.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 21 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 21 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 30 minutes for your next included review. Limit details: You’ve used the included review currently available. Your 139 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Repository: Comfy-Org/comfy-python-sdk/.coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Every reviewer in the matrix failed to contribute — see the panel summary for which cells errored, and the run logs for the underlying cause.
Panel: 0/6 reviewers contributed findings.
Reviewers that did not contribute: claude-opus-5-thinking-max:adversarial (error), gpt-5.6-sol-max:adversarial (error), kimi-k3-high:adversarial (error), claude-opus-5-thinking-max:edge-case (error), gpt-5.6-sol-max:edge-case (error), kimi-k3-high:edge-case (error)
ELI-5
models.runis 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 thatcomfy_sdk.retry's fast class (60s elapsed budget) will not retry.The lever is
httpx.Limits, and now it is reachable:What changed
Comfy/AsyncComfytake a keyword-onlylimits: httpx.Limits | None = None, positioned aftertimeout, and pass it down toComfyLow/AsyncComfyLow. Both class docstrings say what it is for and why amodels.runfan-out is the case that needs it.ComfyLow/AsyncComfyLowtake the same keyword and hand it to thehttpx.Client/httpx.AsyncClientthey build.Noneis dropped rather than forwarded.httpx.Clienttypeslimitsas aLimits(defaultDEFAULT_LIMITS) and does not acceptNone, so passing it through would break the default path. The constructor builds a kwargs dict and only setslimitswhen one was given — which keeps the default exactly httpx's own, rather than importing the privatehttpx._config.DEFAULT_LIMITSto restate it.client=still owns its pool.limitsis ignored on that path, exactly astimeoutalready is, and both__init__docstrings say so.Unreleased/Addedentry.Tests
Five new cases in
tests/test_models_namespace.py, next to the existingtimeoutview tests — no httpx mocks, they construct real clients against theserverfixture stub:Comfy(limits=httpx.Limits(max_connections=250))and theAsyncComfyequivalent build a transport whose pool reports250.Comfy()andAsyncComfy()still report100, which pins "Nonekeeps 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 reports100, not5.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 whetherlimits=arrived at all. The comment above the block says as much. A read-onlylimitsview 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
test_paired_methods_take_the_same_parametersstays green; the default path is byte-identical in behaviour to before.MODEL_RUN_TIMEOUT's 600s, because the single positional argument tohttpx.Timeout(600.0, connect=10.0)setspoolalong withreadandwrite. 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.README.md. This paragraph is additive and sits in themodels.runsection; whichever lands first, the other rebases cleanly or needs a one-hunk resolution.Provenance
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.pywas not run — no generated model, spec file or Router error type is touched by this diff.Residual
Not fixed by this PR, each of them named as out of scope by the work that prompted it:
limitsview onComfyLow/AsyncComfyLoworclient.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-callhttpx.AsyncClientinfrom_urlis untouched. It builds its own client per call and so never uses the shared pool;limitsdoes not reach it. If a large concurrentfrom_urlfan-out ever matters, that is a separate change (it should probably use the shared transport rather than gain its ownlimits).MODEL_RUN_TIMEOUT.poolis 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:
models.runcalls queue and then raisePoolTimeoutis 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.