[PR #215] feat(models): expose the queued submit's cost quote as RequestHandle.estimate #215

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19975-cost-estimate

📝 Commits (6)

1430fca feat(models): expose the queued submit's cost quote as RequestHandle.estimate
a7c3fb3 fix(models): drop a cost quote whose promised figure cannot be read
76855b9 fix(models): drop an estimated quote whose bounds are reversed
01584be Merge branch 'main' into matt/be-19975-cost-estimate
752b7b7 test(models): cover negative and non-finite estimate range bounds
0205d1c Merge remote-tracking branch 'origin/main' into matt/be-19975-cost-estimate

📊 Changes

9 files changed (+505 additions, -9 deletions)

View changed files

📝 CHANGELOG.md (+8 -0)
📝 README.md (+19 -0)
📝 src/comfy_sdk/__init__.py (+8 -1)
📝 src/comfy_sdk/model_requests.py (+225 -2)
📝 src/comfy_sdk/models.py (+11 -2)
📝 tests/conftest.py (+6 -0)
📝 tests/test_models_queue.py (+203 -1)
📝 tests/test_router_spec_contract.py (+23 -1)
📝 uv.lock (+2 -2)

📄 Description

ELI5

When you queue a model request, Router can tell you up front roughly what it will cost, and the SDK now keeps that quote for you instead of throwing it away. A quote that makes no sense, such as a price range whose low end is above its high end, is dropped rather than shown, so nobody gets an impossible price.

Update (brought current with `main`)

Inverted-range finding: already fixed in 76855b9. _estimate_of returns None when min_amount > max_amount (compared as Decimal), and the CodeRabbit thread is resolved, with a reply naming that commit. CodeRabbit has since re-reviewed 752b7b7 and approved it, which replaces its earlier CHANGES_REQUESTED review (on a7c3fb3).
New in this push:
- 01584be merges main into the branch. The 0.5.0 cut on main left [Unreleased] empty, and git's auto-merge silently filed this PR's changelog entry under the released 0.5.0 heading. This commit moves it back under [Unreleased].
- 752b7b7 adds malformed-quote cases for an estimated range whose min_amount or max_amount is negative, NaN, Infinity or a JSON number. Each one reads as no quote, alongside the existing reversed-range case. The tests only add coverage. The code already rejected all of these through the plain-decimal pattern.
The companion Router spec-sync PR (#211) has been de-conflicted against main with a merge commit (no rebase, no force-push). See Residual.

What

New exported symbol: CostEstimate (comfy_sdk.CostEstimate, comfy_sdk.model_requests.CostEstimate). It's a frozen dataclass that mirrors the spec's RouterCostEstimate: source, currency, provider, model, pricing_as_of, the optional amount / min_amount / max_amount (kept as the spec's decimal strings, never turned into floats), amount_cents / min_amount_cents / max_amount_cents / credits, reason, and raw. It also has is_exact / is_estimated / is_unknown. source is an open string, so any value other than exact/estimated reads as unknown, as the spec says.
RequestHandle.estimate / AsyncRequestHandle.estimate is a read-only CostEstimate | None. The docstring says what None means: the estimate is off for the caller, or the submit got an idempotent replay (and the SDK's own transport retry inside one submit can get one), or the handle was rebuilt with client.models.handle.
Both submits read the quote with _estimate_of(body), which never raises. The submit already succeeded at that point, so a missing or malformed quote just means no quote. A required field that is missing, blank or has the wrong type drops the whole quote. So does a quote whose source promises a figure that can't be read: exact with no readable amount, or estimated without both bounds or with min_amount above max_amount. A money string must be a plain non-negative decimal. A number must be finite and non-negative. Otherwise the field is left None and not converted. An unrecognised source carries no figure fields; they are still in raw. raw is left out of the hash, so a quote is hashable.
Both handle __init__s gain a trailing keyword-only estimate=None. The four positional parameters are unchanged, so client.models.handle() and the sync/async parity test don't change.
__repr__ on the handle adds , estimate='<source>' only when a quote is present.
Stub server: ServerState.queue_submit_estimate is sent verbatim on the submit body when it is set.
README (queued-surface section) and CHANGELOG (Unreleased → Added) are updated.

Judgment calls

_estimate_of also returns None when a numeric field is an int too large for a float (OverflowError). This keeps the "never raises" contract airtight.
The spec contract test checks more than the plan asked for. Besides "spec properties ⊆ CostEstimate fields", it checks that the spec's required list equals the dataclass's no-default fields, and that RouterQueueSubmitFields.estimate still points at RouterCostEstimate.
The README example is 6 lines instead of 4, so it can show the estimated range branch next to exact.

Provenance

Authored by: agent-work loop
Verified: on this branch's head after the main merge (752b7b7), on one local interpreter:
- ruff check .: clean. ruff format --check .: clean. mypy src: clean.
- pytest: 1299 passed, 9 skipped. The skips are the existing env-gated integration tests.
- python scripts/check_drift.py: all three checks OK. The models are byte-identical to the regenerated output, the router error types are blessed, and the run content types match.
- python3 scripts/check_public_repo_hygiene.py: OK.
- PR CI on 752b7b7 is green: Test on py3.10–3.13, codegen drift, build-check, public-repo-hygiene and CodeRabbit.
- Doc-drift sweep: the sweep flagged 373 references and showed 135. I judged the ones about the estimate (README queued-surface example and prose, CHANGELOG entry) and the AGENTS.md gate commands, about 25 lines in all. 1 was updated: the CHANGELOG entry had been misfiled under 0.5.0. The rest are generic spec keywords (properties, required) and were not judged one by one.
Deviations: the ticket asked for #211 to be rebased. It was brought current with a merge commit instead, because rewriting a pushed branch is not allowed here. The result is the same, a conflict-free branch on top of main, with no history rewrite.

Residual

Approval and merge are a human step. This PR needs a code-owner approval, then a maintainer merges. CodeRabbit approved 752b7b7, and every review thread is resolved.
#211 (cloud@9794097 Router spec sync) is effectively superseded on main. #209 squash-merged the same cloud@9794097 spec into main, together with the re-blessed ModelNotFound read marker (6c0e0e327e4d). After the merge, #211's whole diff against main is a 12-line rewording of the ModelNotFound docstring. The spec, the generated models and the read marker are all unchanged. _generated.py was regenerated by check_drift.py and is byte-identical; nothing is generated from the Router spec. The full gates pass on #211's merged head (1265 passed, 9 skipped; drift and hygiene OK). A maintainer should either merge #211 for the wording or close it as landed via #209. Both are safe.
The behaviour has only been checked against the in-repo stub server. A real Router 201 that carries estimate was not exercised, because this run had no credentials for an environment with the quote on. tests/integration/test_gateway_e2e.py does not assert on estimate.
The stub's queue_submit_status still defaults to 200, while the real route answers 201. This is cosmetic, since the SDK does not branch on the status.
The separate POST /v2/models/{provider}/{model}/estimate operation (estimateRouterModelCost) is still not wrapped by the SDK. It would be its own change.
Possible overlap: open PR #140 edits the same submit() signatures. Whichever lands second needs a merge from main.
amount_cents / min_amount_cents / max_amount_cents / credits that are negative or non-finite are set to None individually rather than dropping the whole quote, because they are optional fields. The decimal-string amount / min_amount / max_amount are what decide whether a quote survives.

Summary by CodeRabbit

New Features
- Request handles now expose optional upfront cost estimates for submitted requests, including exact amounts or estimated price ranges when available.
- Added the public CostEstimate type. Estimates may be unavailable, including for replayed or rehydrated requests, and are not price guarantees.
- Estimates are supported for both synchronous and asynchronous submissions.
Bug Fixes
- Invalid or incomplete estimates no longer prevent an otherwise successful submission.

_{🔄 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/215 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-19975-cost-estimate` --- ### 📝 Commits (6) - [`1430fca`](https://github.com/Comfy-Org/comfy-python-sdk/commit/1430fca3f3996bdc07ec348f26cfd18017d2e473) feat(models): expose the queued submit's cost quote as RequestHandle.estimate - [`a7c3fb3`](https://github.com/Comfy-Org/comfy-python-sdk/commit/a7c3fb33bc1c8865f5520239cde5ca55c85fe457) fix(models): drop a cost quote whose promised figure cannot be read - [`76855b9`](https://github.com/Comfy-Org/comfy-python-sdk/commit/76855b9435d368f12d6593aa01774f1e916d1db3) fix(models): drop an estimated quote whose bounds are reversed - [`01584be`](https://github.com/Comfy-Org/comfy-python-sdk/commit/01584be8a74660b6430c7c899e2e1feb08aea911) Merge branch 'main' into matt/be-19975-cost-estimate - [`752b7b7`](https://github.com/Comfy-Org/comfy-python-sdk/commit/752b7b70a5eaebcc4034162b408051f10b3d39dc) test(models): cover negative and non-finite estimate range bounds - [`0205d1c`](https://github.com/Comfy-Org/comfy-python-sdk/commit/0205d1c27c40763cf8186c67a024eb8a9e855067) Merge remote-tracking branch 'origin/main' into matt/be-19975-cost-estimate ### 📊 Changes **9 files changed** (+505 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+8 -0) 📝 `README.md` (+19 -0) 📝 `src/comfy_sdk/__init__.py` (+8 -1) 📝 `src/comfy_sdk/model_requests.py` (+225 -2) 📝 `src/comfy_sdk/models.py` (+11 -2) 📝 `tests/conftest.py` (+6 -0) 📝 `tests/test_models_queue.py` (+203 -1) 📝 `tests/test_router_spec_contract.py` (+23 -1) 📝 `uv.lock` (+2 -2) </details> ### 📄 Description ## ELI5 When you queue a model request, Router can tell you up front roughly what it will cost, and the SDK now keeps that quote for you instead of throwing it away. A quote that makes no sense, such as a price range whose low end is above its high end, is dropped rather than shown, so nobody gets an impossible price. ## Update (brought current with `main`) - **Inverted-range finding:** already fixed in 76855b9. `_estimate_of` returns `None` when `min_amount > max_amount` (compared as `Decimal`), and the CodeRabbit thread is resolved, with a reply naming that commit. CodeRabbit has since re-reviewed 752b7b7 and **approved** it, which replaces its earlier `CHANGES_REQUESTED` review (on a7c3fb3). - **New in this push:** - 01584be merges `main` into the branch. The 0.5.0 cut on `main` left `[Unreleased]` empty, and git's auto-merge silently filed this PR's changelog entry under the **released** `0.5.0` heading. This commit moves it back under `[Unreleased]`. - 752b7b7 adds malformed-quote cases for an `estimated` range whose `min_amount` or `max_amount` is negative, `NaN`, `Infinity` or a JSON number. Each one reads as no quote, alongside the existing reversed-range case. The tests only add coverage. The code already rejected all of these through the plain-decimal pattern. - The companion Router spec-sync PR (#211) has been de-conflicted against `main` with a merge commit (no rebase, no force-push). See Residual. ## What - **New exported symbol: `CostEstimate`** (`comfy_sdk.CostEstimate`, `comfy_sdk.model_requests.CostEstimate`). It's a frozen dataclass that mirrors the spec's `RouterCostEstimate`: `source`, `currency`, `provider`, `model`, `pricing_as_of`, the optional `amount` / `min_amount` / `max_amount` (kept as the spec's decimal **strings**, never turned into floats), `amount_cents` / `min_amount_cents` / `max_amount_cents` / `credits`, `reason`, and `raw`. It also has `is_exact` / `is_estimated` / `is_unknown`. `source` is an open string, so any value other than `exact`/`estimated` reads as unknown, as the spec says. - `RequestHandle.estimate` / `AsyncRequestHandle.estimate` is a read-only `CostEstimate | None`. The docstring says what `None` means: the estimate is off for the caller, or the submit got an idempotent replay (and the SDK's own transport retry inside one `submit` can get one), or the handle was rebuilt with `client.models.handle`. - Both `submit`s read the quote with `_estimate_of(body)`, which never raises. The submit already succeeded at that point, so a missing or malformed quote just means no quote. A required field that is missing, blank or has the wrong type drops the whole quote. So does a quote whose `source` promises a figure that can't be read: `exact` with no readable `amount`, or `estimated` without both bounds or with `min_amount` above `max_amount`. A money string must be a plain non-negative decimal. A number must be finite and non-negative. Otherwise the field is left `None` and not converted. An unrecognised `source` carries no figure fields; they are still in `raw`. `raw` is left out of the hash, so a quote is hashable. - Both handle `__init__`s gain a trailing **keyword-only** `estimate=None`. The four positional parameters are unchanged, so `client.models.handle()` and the sync/async parity test don't change. - `__repr__` on the handle adds `, estimate='<source>'` only when a quote is present. - Stub server: `ServerState.queue_submit_estimate` is sent verbatim on the submit body when it is set. - README (queued-surface section) and CHANGELOG (Unreleased → Added) are updated. ## Judgment calls - `_estimate_of` also returns `None` when a numeric field is an int too large for a float (`OverflowError`). This keeps the "never raises" contract airtight. - The spec contract test checks more than the plan asked for. Besides "spec properties ⊆ `CostEstimate` fields", it checks that the spec's `required` list equals the dataclass's no-default fields, and that `RouterQueueSubmitFields.estimate` still points at `RouterCostEstimate`. - The README example is 6 lines instead of 4, so it can show the `estimated` range branch next to `exact`. ## Provenance - **Authored by:** agent-work loop - **Verified:** on this branch's head after the `main` merge (752b7b7), on one local interpreter: - `ruff check .`: clean. `ruff format --check .`: clean. `mypy src`: clean. - `pytest`: 1299 passed, 9 skipped. The skips are the existing env-gated integration tests. - `python scripts/check_drift.py`: all three checks OK. The models are byte-identical to the regenerated output, the router error types are blessed, and the run content types match. - `python3 scripts/check_public_repo_hygiene.py`: OK. - PR CI on 752b7b7 is green: Test on py3.10–3.13, codegen drift, build-check, public-repo-hygiene and CodeRabbit. - Doc-drift sweep: the sweep flagged 373 references and showed 135. I judged the ones about the estimate (README queued-surface example and prose, CHANGELOG entry) and the AGENTS.md gate commands, about 25 lines in all. 1 was updated: the CHANGELOG entry had been misfiled under 0.5.0. The rest are generic spec keywords (`properties`, `required`) and were not judged one by one. - **Deviations:** the ticket asked for #211 to be *rebased*. It was brought current with a merge commit instead, because rewriting a pushed branch is not allowed here. The result is the same, a conflict-free branch on top of `main`, with no history rewrite. ## Residual - **Approval and merge are a human step.** This PR needs a code-owner approval, then a maintainer merges. CodeRabbit approved 752b7b7, and every review thread is resolved. - **#211 (cloud@9794097 Router spec sync) is effectively superseded on `main`.** #209 squash-merged the same cloud@9794097 spec into `main`, together with the re-blessed `ModelNotFound` read marker (`6c0e0e327e4d`). After the merge, #211's whole diff against `main` is a 12-line rewording of the `ModelNotFound` docstring. The spec, the generated models and the read marker are all unchanged. `_generated.py` was regenerated by `check_drift.py` and is byte-identical; nothing is generated from the Router spec. The full gates pass on #211's merged head (1265 passed, 9 skipped; drift and hygiene OK). A maintainer should either merge #211 for the wording or close it as landed via #209. Both are safe. - The behaviour has only been checked against the in-repo stub server. A real Router `201` that carries `estimate` was not exercised, because this run had no credentials for an environment with the quote on. `tests/integration/test_gateway_e2e.py` does not assert on `estimate`. - The stub's `queue_submit_status` still defaults to `200`, while the real route answers `201`. This is cosmetic, since the SDK does not branch on the status. - The separate `POST /v2/models/{provider}/{model}/estimate` operation (`estimateRouterModelCost`) is still not wrapped by the SDK. It would be its own change. - Possible overlap: open PR #140 edits the same `submit()` signatures. Whichever lands second needs a merge from `main`. - `amount_cents` / `min_amount_cents` / `max_amount_cents` / `credits` that are negative or non-finite are set to `None` individually rather than dropping the whole quote, because they are optional fields. The decimal-string `amount` / `min_amount` / `max_amount` are what decide whether a quote survives.   ## Summary by CodeRabbit * **New Features** * Request handles now expose optional upfront cost estimates for submitted requests, including exact amounts or estimated price ranges when available. * Added the public `CostEstimate` type. Estimates may be unavailable, including for replayed or rehydrated requests, and are not price guarantees. * Estimates are supported for both synchronous and asynchronous submissions. * **Bug Fixes** * Invalid or incomplete estimates no longer prevent an otherwise successful submission.  --- <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:24 +00:00