Repository navigation
feat(jobs): expose the timestamp, progress and queue-position properties on Job/AsyncJob - #171
mattmillerai wants to merge 5 commits into
Conversation
…ies on Job/AsyncJob Job and AsyncJob both hold the full v2 job model and surfaced four fields off it (id, status, outputs, error). created_at, started_at, completed_at, expires_at, progress, queue_position, metrics and urls were reachable only through the private model attribute, so a caller could not read a job's duration without one. All eight land as read-only properties on both classes. Timestamps come back as the timezone-aware datetimes the model already parses, so job.completed_at - job.started_at is the run duration; the nullable wire fields reach the caller as None. Like the existing properties these are views onto the state the handle holds — nothing re-fetches. progress is lifted into the SDK's own Progress dataclass rather than the generated model, so a snapshot read off a handle is the same type as the progress frames job.events() yields and one 'case Progress()' matches either. That lift needed current_node_class, which the event decoder was dropping; it is appended to the dataclass so the published positional order is unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. 8 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughThe SDK adds cached lifecycle, progress, queue, metrics, and URL properties to synchronous and asynchronous job handles. Progress conversion now preserves ChangesJob state and progress accessors
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Client
participant Job
participant Server
participant progress_from_model
Client->>Job: access properties or call refresh()
Job->>Server: request current job state
Server-->>Job: return timestamps, queue, metrics, URLs, and progress
Job->>progress_from_model: convert progress snapshot
progress_from_model-->>Job: return Progress
Job-->>Client: expose cached properties
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The lifecycle properties appear ready to merge after normal checks; no concrete issue requiring a fix was identified. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 46.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 6 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly identifies the primary change: exposing timestamp, progress, and queue-position properties on both Job and AsyncJob. It is concise and directly related to the changeset, although… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 46.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 6 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
- Commit to this branch
- Create a new PR
Comment @coderabbitai help to get the list of available commands.
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)
…ecycle-properties # Conflicts: # CHANGELOG.md
The main-branch spec sync (86d8f2c) declared X-Comfy-Credits-Used on runRouterModel's 200 after credits_used was already lifted from it, tripping the reconciliation tripwire on merge. Move credits_used into _CONTRACT_HEADER_LIFTS per the tripwire's own instructions, and give its presence-check a valid decimal (the generic "x" sentinel parses to None same as absent, since credits_used goes through Decimal parsing).
robinjhuang
left a comment
There was a problem hiding this comment.
Auto-approved under the full-autonomy policy.
Gates verified at 16a5c8eb0cd0059b7d89acece861fb285fd1a9af:
full-autonomylabel present- assigned to, or review requested from, @robinjhuang
- not a draft
- 8 required check(s) green — none failing, none pending
Issued by full-autonomy-approve.yml (run). This approval attests
that the machine gates above passed at this commit. It does not attest that a
human read the diff.
guill
left a comment
There was a problem hiding this comment.
This needs to be designed along with Developer Platform.
…ecycle-properties # Conflicts: # CHANGELOG.md # src/comfy_sdk/jobs.py # tests/conftest.py # tests/test_router_spec_contract.py
Editing the dict or JobUrls a read returned rewrote the handle's own model, whose urls refresh(), cancel() and events() follow. Copy them, as metadata already does and as the TypeScript SDK's accessors do.
|
@guill on your review (designed along with Developer Platform): I'm on Developer Platform and reviewed this together with its TypeScript twin, Comfy-Org/comfy-typescript-sdk#162, which merged today. The two now ship the same surface: the same eight properties ( |
|
Approving at 78d49d1c0e. Merged main in (dd6be49): conflicts in jobs.py, CHANGELOG, conftest.py and test_router_spec_contract.py. Kept both the job metadata properties from #204 and the lifecycle properties here, made the stub job body take both the metadata and the lifecycle knobs, and took main's version of the credits_used contract test, which already reconciles the same header. Fixed in 78d49d1:
Clean: the nullable vs required split against the Checked: pytest (1248 passed on the merge), ruff, ruff format, mypy, codegen drift and the hygiene script locally; the copy tests fail with the fix reverted. |
ELI-5
A job handle already knows when the job was created, when it started, when it finished, how far along it is and where it sits in the queue — it just never told you. It handed back four things (
id,status,outputs,error) and kept the rest behind a private attribute, so "how long did this run take?" meant reaching intojob._model. This adds the other eight as ordinary read-only properties on bothJobandAsyncJob.What changed
created_at,started_at,completed_at,expires_at,progress,queue_position,metricsandurlsare now properties onJoband onAsyncJob.datetimes the model already parses, not as strings — which is the whole point, sincejob.completed_at - job.started_atis what a caller actually wants and is the one thing they cannot do for themselves against a raw string.started_at,completed_at,progress,queue_position,metrics) reach the caller asNone.created_at,expires_atandurlsare non-nullable in the contract and are always set.statusandoutputs: nothing re-fetches implicitly, andrefresh()/wait()/result()are what move them. There is a test for each half of that — reads do not poll, and arefresh()does move them.urlsalso makes an existing README claim true: the authentication section already told readers aboutjob.urls.self/cancel/events, which until now did not exist on the public surface. The one private-attribute access left in the test suite (job._model.urls.events, intest_sse_idle.py) now goes through the property.Two judgment calls worth a reviewer's eye
progressreturns the SDK'sProgressdataclass, not the generated model. The SDK already has a publicProgress— it is whatjob.events()yields and what the README'scase Progress() as p:matches. Returningcomfy_low.models.Progresshere would have put two different classes calledProgresson one surface, andisinstance(job.progress, comfy_sdk.Progress)would have beenFalse. Lifting it means a snapshot read off a handle and one received live are the same type, and onecase Progress()matches either.That lift needed
current_node_class, the one field of the contract's progress schema the SSE decoder was silently dropping — without it the lift would have been lossy. It is now decoded fromprogressframes too, and is appended to the dataclass rather than slotted in besidecurrent_node, so the published positional order is unchanged for anyone constructing aProgresspositionally. A test pins the two paths together: for the same payload, the model lift and the stream decoder must produce an equalProgress, so a field added to one and not the other fails.urlspasses the generatedcomfy_low.models.JobUrlsthrough unwrapped, the same wayerroralready passesJobErrorthrough. It is deliberately not re-exported at thecomfy_sdktop level — that surface is guarded by a test and widening it was not part of this change — so a caller who wants to name the type imports it fromcomfy_low.models.How this was tested
The stub server grew knobs for the nullable lifecycle fields (
job_started_at,job_completed_at,job_progress,job_queue_position,job_metrics), so the populated case and the null case both come off a real response body rather than a hand-built model — which is what puts the timestamp parsing itself under test. Defaults are unchanged from the previously hardcoded values, so no existing test's fixture moved.tests/test_job_lifecycle_properties.pycovers every property in the populated and the null shape, on the sync and the async class, plus the does-not-re-fetch and the does-move-on-refresh halves of the contract.The repo's existing
tests/test_sync_async_parity.pyis what guarantees the sync and async surfaces stay in step here, and it is not taken on faith: deletingAsyncJob.queue_positionlocally fails it withJob/AsyncJob public surface diverges: sync-only=['queue_position'], and it was restored before committing.Provenance
ruff check .: all checks passed;ruff format --check .: 58 files already formatted;mypy src: no issues in 21 source files;pytest: 987 passed, 9 skipped;scripts/check_public_repo_hygiene.py: no internal-only referencesResidual
Not fixed here, and actionable on its own:
JobagainstAsyncJobonly. So the names added here match the ones the request specified, but nobody has yet observed them matching the TypeScript side. Someone should run that check once both halves are merged; a name mismatch would be a rename on one side, not a redesign.progresshas never been observed populated by a real server, only by the stub. The request notes that Comfy Cloud returnsnullfor it on a poll today (a server-side gap tracked separately); that is taken on the request's word, not verified — this environment has no live credentials, andtests/integration/test_gateway_e2e.pyskips withoutCOMFY_BASE_URLandCOMFY_API_KEY. For that reason no named-surface claim went into the README or the docstrings: they say "not every surface fills this in on a poll" rather than naming one. If the server gap is closed later, the wording is worth revisiting to say so positively.None, on both classes" criterion is met literally for the five nullable ones only.created_at,expires_atandurlsare non-nullable in the contract; the tests assert them in both scenarios and say why there is no third one. If the contract ever makes one of them nullable, that test is where it has to be noticed.Summary by CodeRabbit
New Features
Documentation
refresh(),wait(), orresult(), and that absent progress is represented asNone.