feat(jobs): expose the timestamp, progress and queue-position properties on Job/AsyncJob - #171
mattmillerai wants to merge 3 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.
mattmillerai
added
agent-coded
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 20 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 20 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 24 minutes for your next included review. Limit details: You’ve used the included review currently available. Your 148 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 (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: Comfy-Org/comfy-python-sdk/.coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (8)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review 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: ➖ Normal 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: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 44.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 6 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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)
mattmillerai
added
the
full-autonomy
…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
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.
2 participants
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