feat: add the models namespace to the existing client - #69
Conversation
Model operations get a namespace on the client integrators already construct — client.models on both Comfy and AsyncComfy — instead of a second client object, which would fork credential handling, base URL, transport and timeout configuration. The namespace holds the host client's transport itself rather than a copy of its settings, so a change made on the client after construction (a rotated key, a different timeout) applies through models with no re-wiring. It exposes that shared configuration read-only as base_url and timeout, backed by new read-only properties of the same names on both comfy_low transports so the layer above does not have to reach into private attributes. Nothing is added to the top-level package: from comfy_sdk import Comfy stays the only entry point and client.models is the whole surface. The sync/async parity guard gains the new pair, and the README documents the namespace.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Limit details: You’ve used the included review currently available. Your 102 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Comment |
mattmillerai
added
cursor-review
4 participants
ELI-5
The client you already build (
Comfy(...)/AsyncComfy(...)) now has amodelsdrawer on it:client.models. It is a drawer in the same cabinet, not a second cabinet — it uses the client's key, its address, its connection pool and its timeout, and if you change one of those on the client afterwards the drawer sees the change immediately.What changed
client.modelsnamespace on bothComfyandAsyncComfy(src/comfy_sdk/models.py:Models/AsyncModels). It is constructed by the client with the client's own transport, so credentials, base URL, connection pool and timeout are shared rather than duplicated.models.base_urlandmodels.timeoutread through to the live transport, so a post-construction change on the client (rotated key, new timeout) is visible throughmodelswith nothing to re-sync.base_url/timeoutproperties onComfyLowandAsyncComfyLowso the layer above reports that configuration without reaching into private attributes. Additive and read-only; neither name was previously an attribute on either transport (only_p,_own_client,_clientare set in__init__), so nothing is shadowed or overridden.## The models namespacesection in the README plus module/class docstrings.tests/test_models_namespace.py(13 tests) and the newModels/AsyncModelspair added to the sync/async public-surface parity guard.Why a namespace and not a second client
A separate client object for model operations would fork credential handling, base URL resolution, transport and retry/timeout configuration — four things that would then have to be kept in agreement. Holding the host client's transport object by identity makes that impossible by construction, which is what the tests assert (
client.models._low is client._low, plus a live timeout change and a live credential rotation both observed throughmodels).Scope
This adds the namespace and its shared configuration only. A model-run method, credential resolution, exception mapping and retry policy are each separate follow-on work and are deliberately absent here. No dead-end was added for them: there is no stub method that raises "not supported" — the attribute simply does not exist yet, so nothing in this diff denies a capability or has to be falsified. Sizing the part not covered here: the vendored contract this SDK is generated from and typed against (
spec/openapi.yaml,spec/VERSION2.0.0) declares 11 operationIds, 0 of which are model routes, so a model method could not be typed against a committed contract in this change even if it were in scope.Judgment calls
Models/AsyncModelsare not exported fromcomfy_sdkand are not in__all__, sofrom comfy_sdk import Comfyremains the only import a caller needs; the namespace is reached asclient.models. That is a deliberate departure fromAssetFactory/WorkflowFactory, which are exported — this requirement asked for no new caller-facing import path. A test guards it. The implementation modulecomfy_sdk.modelsnecessarily exists (any implementation needs a module) but is not part of the documented surface; annotating the type iscomfy_sdk.models.Models._ModelsBase._lowis typed as the sync-or-async union, with each subclass's constructor pinning the concrete transport. Narrowing the attribute per subclass is a mypy override error; the constructor signatures are what enforce the pairing today, and a future sync/async method narrows locally.models.timeoutreturnshttpx.Timeout, the transport's live value, rather than thefloat | Nonepassed to the constructor — the constructor value is not retained anywhere, andhttpxis already part of this SDK's public surface elsewhere.Not exercised here
The specification and design documents that settle this namespace's name and its placement inside the existing client live in an internal tool that is not reachable from the environment this was built in, as do the related tracker issues; those requirements were taken as given rather than re-read at the source, and nothing in this diff depends on an unread detail of them beyond the namespace's name and the on-the-existing-client constraint. Everything verifiable locally — the client construction path, both transports, the vendored spec, the full test suite and every CI gate — was exercised, below.
Provenance
ruff check .clean;ruff format --check .clean (44 files);mypy srcno issues in 17 files;pytest180 passed / 4 skipped;scripts/check_drift.pymodels in sync with the spec;scripts/check_public_repo_hygiene.pyclean