[PR #210] docs(errors): list rate_limited explicitly as the fourth code both surfaces share #211

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19964-rate-limited-by-code

📝 Commits (2)

826aa2f docs(errors): list rate_limited explicitly as the fourth code both surfaces share
07253ba docs(errors): cover status-derived shared codes, fold changelog bullet, fix _BY_CODE comment

📊 Changes

5 files changed (+73 additions, -28 deletions)

View changed files

📝 CHANGELOG.md (+4 -0)
📝 README.md (+13 -6)
📝 src/comfy_sdk/exceptions.py (+19 -10)
📝 src/comfy_sdk/router_exceptions.py (+15 -9)
📝 tests/test_error_mapping.py (+22 -3)

📄 Description

ELI-5

When the jobs/assets API says "you're making requests too fast" (rate_limited), the SDK already raised comfy_sdk.router_exceptions.RateLimited — but only because a lookup in one table missed and it fell through to a second one. This PR writes that mapping down on purpose, in the first table, next to the three other codes that work the same way, and fixes the docs that still said "three". What gets raised doesn't change.

What changed

src/comfy_sdk/exceptions.py: "rate_limited": RateLimited added to _BY_CODE (next to insufficient_credits), RateLimited imported from .router_exceptions and added to __all__ (sorted) as the other shared classes are. The _BY_CODE comment, _class_for step 1, the to_sdk_error comment and the __all__ comment now say four, and the _BY_CODE comment says why: the v2 contract's account rate limit (components/responses/RateLimited) and Router's rate_limited bucket are the same windowed allowance spelled the same way, so one class serves both and .error_type == "rate_limited" is set from either surface.
src/comfy_sdk/router_exceptions.py: module docstring names four shared buckets including rate_limited; RateLimited's docstring keeps the Router meaning and the ConcurrencyLimitExceeded contrast, adds the v2 jobs/assets use, and narrows "detail names the window" to the Router surface. _spec_meaning_digest untouched (it hashes the Router spec's meaning, not this docstring; test_router_spec_contract.py and check_drift.py both pass).
README.md: the shared-names paragraph lists four and says a jobs/assets call throttled with 429 rate_limited raises RateLimited; the "never reached Router → plain ComfyError" sentence gets a clause for responses that land on one of the four shared codes — a v2 envelope naming one, or a bare 401/402/403 the status table maps to one — so except RouterError firing is not by itself proof Router answered.
tests/test_error_mapping.py: rate_limited added to test_a_bucket_both_surfaces_spell_keeps_its_v2_class; new test_a_v2_envelope_rate_limited_is_the_shared_router_class (no error_type, asserts exact type, RouterError ancestry, code, error_type, retry_after), with a comment marking the sharing as deliberate.
CHANGELOG.md: one bullet in Unreleased's existing ### Changed section.

Judgment calls

Package root left alone. comfy_sdk re-exports Unauthorized/Forbidden/InsufficientCredits at the root, but its own comment says per-bucket Router classes stay in comfy_sdk.router_exceptions; adding comfy_sdk.RateLimited would be new public surface, which is out of scope. The README says so explicitly ("and the first three by comfy_sdk too").
rate_limited removed from test_a_router_only_bucket_raises_its_typed_class. It's no longer a Router-only bucket, so leaving it there would make that test's name wrong. Its assertions (exact type, code, http_status, retry_after == 7 at 429) are all covered again by the new test, so no coverage is lost.
CHANGELOG placement: the entry sits in Unreleased's existing ### Changed section rather than a second heading. It also mentions the new comfy_sdk.exceptions.RateLimited re-export.

Residual

tests/test_head_refusal.py and the HEAD /assets/by-hash 429 synthesis belong to open PR #208 and weren't touched or run here (that file doesn't exist on main). When #208 lands, its type(exc) is RateLimited assertion now resolves through _BY_CODE instead of the fall-through. Same class, so no change is expected.
Open PRs #205 and #151 also add _BY_CODE lines. Expect a trivial merge conflict on adjacent dict lines, nothing semantic.
No behavior change, so no mutation test can tell this PR apart from main: removing the new _BY_CODE entry still passes because the fall-through gives the same class. The proof that behavior didn't change is that tests/test_job_metadata.py (which already pins v2 rate_limited → RateLimited) passes unmodified.
tests/integration/test_gateway_e2e.py skips without live credentials, so it wasn't exercised.

Provenance

Authored by: agent-work loop
Verified:
- Tests: full suite green on the current head: 1266 passed, 9 skipped (the skips include the credential-gated gateway e2e)
- Lint/format/types: ruff check, ruff format --check, mypy src all clean
- Public-repo hygiene script: clean
- README carve-out checked against comfy_low.errors._CODE_BY_STATUS (401/402/403 map to the shared codes; 429 maps to queue_full, not rate_limited)
- Not run locally: the 3.10/3.11/3.13 matrix legs, build-check, codegen drift (no generated or spec file touched)
Deviations: none from acceptance. The root-export and test-reshuffle choices are listed under Judgment calls.

_{🔄 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/210 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-19964-rate-limited-by-code` --- ### 📝 Commits (2) - [`826aa2f`](https://github.com/Comfy-Org/comfy-python-sdk/commit/826aa2fbfe4d67b26fdce7cb0035b5223c5d3bba) docs(errors): list rate_limited explicitly as the fourth code both surfaces share - [`07253ba`](https://github.com/Comfy-Org/comfy-python-sdk/commit/07253bad925c7b66bf83c20d94d63354faba0161) docs(errors): cover status-derived shared codes, fold changelog bullet, fix _BY_CODE comment ### 📊 Changes **5 files changed** (+73 additions, -28 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+4 -0) 📝 `README.md` (+13 -6) 📝 `src/comfy_sdk/exceptions.py` (+19 -10) 📝 `src/comfy_sdk/router_exceptions.py` (+15 -9) 📝 `tests/test_error_mapping.py` (+22 -3) </details> ### 📄 Description ## ELI-5 When the jobs/assets API says "you're making requests too fast" (`rate_limited`), the SDK already raised `comfy_sdk.router_exceptions.RateLimited` — but only because a lookup in one table missed and it fell through to a second one. This PR writes that mapping down on purpose, in the first table, next to the three other codes that work the same way, and fixes the docs that still said "three". What gets raised doesn't change. ## What changed - `src/comfy_sdk/exceptions.py`: `"rate_limited": RateLimited` added to `_BY_CODE` (next to `insufficient_credits`), `RateLimited` imported from `.router_exceptions` and added to `__all__` (sorted) as the other shared classes are. The `_BY_CODE` comment, `_class_for` step 1, the `to_sdk_error` comment and the `__all__` comment now say **four**, and the `_BY_CODE` comment says why: the v2 contract's account rate limit (`components/responses/RateLimited`) and Router's `rate_limited` bucket are the same windowed allowance spelled the same way, so one class serves both and `.error_type == "rate_limited"` is set from either surface. - `src/comfy_sdk/router_exceptions.py`: module docstring names four shared buckets including `rate_limited`; `RateLimited`'s docstring keeps the Router meaning and the `ConcurrencyLimitExceeded` contrast, adds the v2 jobs/assets use, and narrows "`detail` names the window" to the Router surface. `_spec_meaning_digest` untouched (it hashes the Router spec's `meaning`, not this docstring; `test_router_spec_contract.py` and `check_drift.py` both pass). - `README.md`: the shared-names paragraph lists four and says a jobs/assets call throttled with `429 rate_limited` raises `RateLimited`; the "never reached Router → plain `ComfyError`" sentence gets a clause for responses that land on one of the four shared codes — a v2 envelope naming one, or a bare `401`/`402`/`403` the status table maps to one — so `except RouterError` firing is not by itself proof Router answered. - `tests/test_error_mapping.py`: `rate_limited` added to `test_a_bucket_both_surfaces_spell_keeps_its_v2_class`; new `test_a_v2_envelope_rate_limited_is_the_shared_router_class` (no `error_type`, asserts exact type, `RouterError` ancestry, `code`, `error_type`, `retry_after`), with a comment marking the sharing as deliberate. - `CHANGELOG.md`: one bullet in Unreleased's existing `### Changed` section. ## Judgment calls - **Package root left alone.** `comfy_sdk` re-exports `Unauthorized`/`Forbidden`/`InsufficientCredits` at the root, but its own comment says per-bucket Router classes stay in `comfy_sdk.router_exceptions`; adding `comfy_sdk.RateLimited` would be new public surface, which is out of scope. The README says so explicitly ("and the first three by `comfy_sdk` too"). - **`rate_limited` removed from `test_a_router_only_bucket_raises_its_typed_class`.** It's no longer a Router-only bucket, so leaving it there would make that test's name wrong. Its assertions (exact type, `code`, `http_status`, `retry_after == 7` at 429) are all covered again by the new test, so no coverage is lost. - **CHANGELOG placement:** the entry sits in Unreleased's existing `### Changed` section rather than a second heading. It also mentions the new `comfy_sdk.exceptions.RateLimited` re-export. ## Residual - `tests/test_head_refusal.py` and the `HEAD /assets/by-hash` 429 synthesis belong to open PR #208 and weren't touched or run here (that file doesn't exist on `main`). When #208 lands, its `type(exc) is RateLimited` assertion now resolves through `_BY_CODE` instead of the fall-through. Same class, so no change is expected. - Open PRs #205 and #151 also add `_BY_CODE` lines. Expect a trivial merge conflict on adjacent dict lines, nothing semantic. - No behavior change, so no mutation test can tell this PR apart from `main`: removing the new `_BY_CODE` entry still passes because the fall-through gives the same class. The proof that behavior didn't change is that `tests/test_job_metadata.py` (which already pins v2 `rate_limited` → `RateLimited`) passes unmodified. - `tests/integration/test_gateway_e2e.py` skips without live credentials, so it wasn't exercised. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: full suite green on the current head: 1266 passed, 9 skipped (the skips include the credential-gated gateway e2e) - Lint/format/types: `ruff check`, `ruff format --check`, `mypy src` all clean - Public-repo hygiene script: clean - README carve-out checked against `comfy_low.errors._CODE_BY_STATUS` (401/402/403 map to the shared codes; 429 maps to `queue_full`, not `rate_limited`) - Not run locally: the 3.10/3.11/3.13 matrix legs, `build-check`, codegen drift (no generated or spec file touched) - **Deviations:** none from acceptance. The root-export and test-reshuffle choices are listed under Judgment calls. --- <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:21 +00:00

bsakaguchi referenced this issue

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

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