[PR #171] feat(jobs): expose the timestamp, progress and queue-position properties on Job/AsyncJob #169

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15899-job-lifecycle-properties

📝 Commits (5)

8f92101 feat(jobs): expose the timestamp, progress and queue-position properties on Job/AsyncJob
2ef0de0 Merge remote-tracking branch 'origin/main' into matt/be-15899-job-lifecycle-properties
16a5c8e fix(tests): reconcile credits_used against the now-declared spec header
dd6be49 Merge remote-tracking branch 'origin/main' into matt/be-15899-job-lifecycle-properties
78d49d1 fix(jobs): hand back copies of metrics and urls from Job/AsyncJob

📊 Changes

8 files changed (+515 additions, -11 deletions)

View changed files

📝 CHANGELOG.md (+13 -0)
📝 README.md (+32 -0)
📝 src/comfy_sdk/events.py (+27 -0)
📝 src/comfy_sdk/jobs.py (+125 -1)
📝 tests/conftest.py (+27 -8)
📝 tests/test_event_types.py (+42 -1)
➕ tests/test_job_lifecycle_properties.py (+248 -0)
📝 tests/test_sse_idle.py (+1 -1)

📄 Description

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 into job._model. This adds the other eight as ordinary read-only properties on both Job and AsyncJob.

What changed

created_at, started_at, completed_at, expires_at, progress, queue_position, metrics and urls are now properties on Job and on AsyncJob.

The timestamps come back as the timezone-aware datetimes the model already parses, not as strings — which is the whole point, since job.completed_at - job.started_at is what a caller actually wants and is the one thing they cannot do for themselves against a raw string.
The nullable wire fields (started_at, completed_at, progress, queue_position, metrics) reach the caller as None. created_at, expires_at and urls are non-nullable in the contract and are always set.
All eight are views onto the state the handle currently holds, exactly like status and outputs: nothing re-fetches implicitly, and refresh() / wait() / result() are what move them. There is a test for each half of that — reads do not poll, and a refresh() does move them.
urls also makes an existing README claim true: the authentication section already told readers about job.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, in test_sse_idle.py) now goes through the property.

Two judgment calls worth a reviewer's eye

progress returns the SDK's Progress dataclass, not the generated model. The SDK already has a public Progress — it is what job.events() yields and what the README's case Progress() as p: matches. Returning comfy_low.models.Progress here would have put two different classes called Progress on one surface, and isinstance(job.progress, comfy_sdk.Progress) would have been False. Lifting it means a snapshot read off a handle and one received live are the same type, and one case 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 from progress frames too, and is appended to the dataclass rather than slotted in beside current_node, so the published positional order is unchanged for anyone constructing a Progress positionally. A test pins the two paths together: for the same payload, the model lift and the stream decoder must produce an equal Progress, so a field added to one and not the other fails.

urls passes the generated comfy_low.models.JobUrls through unwrapped, the same way error already passes JobError through. It is deliberately not re-exported at the comfy_sdk top 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 from comfy_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.py covers 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.py is what guarantees the sync and async surfaces stay in step here, and it is not taken on faith: deleting AsyncJob.queue_position locally fails it with Job/AsyncJob public surface diverges: sync-only=['queue_position'], and it was restored before committing.

Provenance

Authored by: agent-work loop
Verified: 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 references
Deviations: none against the plan; the acceptance gaps I could not close are under Residual below

Residual

Not fixed here, and actionable on its own:

The cross-SDK surface-parity check between this SDK and the TypeScript one was never run. It is the acceptance criterion that spans two repositories, and it lives outside this one — nothing in this repo can execute it, and the TypeScript side was not reachable from where this was built. What is verified is the in-repo parity test, which compares Job against AsyncJob only. 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.
The TypeScript half is not in this PR and this change does not depend on it — the two are expected to land together, and a reviewer merging only this one leaves the parity check failing in the other direction until the counterpart lands.
progress has never been observed populated by a real server, only by the stub. The request notes that Comfy Cloud returns null for 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, and tests/integration/test_gateway_e2e.py skips without COMFY_BASE_URL and COMFY_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.
Three of the eight properties have no null case to test, so the "populated and None, on both classes" criterion is met literally for the five nullable ones only. created_at, expires_at and urls are 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
- Job handles now expose creation, start, completion, and expiration timestamps.
- View current progress, queue position, metrics, and related job URLs.
- Synchronous and asynchronous jobs provide the same lifecycle information.
- Progress events include the currently executing node class.
Documentation
- Added guidance describing job handle properties, cached state, refresh behavior, and unavailable progress values.
- Clarified that job state updates through refresh(), wait(), or result(), and that absent progress is represented as None.

_{🔄 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/171 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-15899-job-lifecycle-properties` --- ### 📝 Commits (5) - [`8f92101`](https://github.com/Comfy-Org/comfy-python-sdk/commit/8f9210156783439071240ebeebdf04925e310b70) feat(jobs): expose the timestamp, progress and queue-position properties on Job/AsyncJob - [`2ef0de0`](https://github.com/Comfy-Org/comfy-python-sdk/commit/2ef0de0222c64cee1d432507c10ad6316079e1ec) Merge remote-tracking branch 'origin/main' into matt/be-15899-job-lifecycle-properties - [`16a5c8e`](https://github.com/Comfy-Org/comfy-python-sdk/commit/16a5c8eb0cd0059b7d89acece861fb285fd1a9af) fix(tests): reconcile credits_used against the now-declared spec header - [`dd6be49`](https://github.com/Comfy-Org/comfy-python-sdk/commit/dd6be4996ebb1df7ac1b5d78684121395894899a) Merge remote-tracking branch 'origin/main' into matt/be-15899-job-lifecycle-properties - [`78d49d1`](https://github.com/Comfy-Org/comfy-python-sdk/commit/78d49d18afe4fe7001d85b21078b561c476e7a4d) fix(jobs): hand back copies of metrics and urls from Job/AsyncJob ### 📊 Changes **8 files changed** (+515 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+13 -0) 📝 `README.md` (+32 -0) 📝 `src/comfy_sdk/events.py` (+27 -0) 📝 `src/comfy_sdk/jobs.py` (+125 -1) 📝 `tests/conftest.py` (+27 -8) 📝 `tests/test_event_types.py` (+42 -1) ➕ `tests/test_job_lifecycle_properties.py` (+248 -0) 📝 `tests/test_sse_idle.py` (+1 -1) </details> ### 📄 Description ## 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 into `job._model`. This adds the other eight as ordinary read-only properties on both `Job` and `AsyncJob`. ## What changed `created_at`, `started_at`, `completed_at`, `expires_at`, `progress`, `queue_position`, `metrics` and `urls` are now properties on `Job` and on `AsyncJob`. - The timestamps come back as the timezone-aware `datetime`s the model already parses, not as strings — which is the whole point, since `job.completed_at - job.started_at` is what a caller actually wants and is the one thing they cannot do for themselves against a raw string. - The nullable wire fields (`started_at`, `completed_at`, `progress`, `queue_position`, `metrics`) reach the caller as `None`. `created_at`, `expires_at` and `urls` are non-nullable in the contract and are always set. - All eight are views onto the state the handle currently holds, exactly like `status` and `outputs`: nothing re-fetches implicitly, and `refresh()` / `wait()` / `result()` are what move them. There is a test for each half of that — reads do not poll, and a `refresh()` does move them. - `urls` also makes an existing README claim true: the authentication section already told readers about `job.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`, in `test_sse_idle.py`) now goes through the property. ### Two judgment calls worth a reviewer's eye **`progress` returns the SDK's `Progress` dataclass, not the generated model.** The SDK already has a public `Progress` — it is what `job.events()` yields and what the README's `case Progress() as p:` matches. Returning `comfy_low.models.Progress` here would have put two different classes called `Progress` on one surface, and `isinstance(job.progress, comfy_sdk.Progress)` would have been `False`. Lifting it means a snapshot read off a handle and one received live are the same type, and one `case 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 from `progress` frames too, and is **appended** to the dataclass rather than slotted in beside `current_node`, so the published positional order is unchanged for anyone constructing a `Progress` positionally. A test pins the two paths together: for the same payload, the model lift and the stream decoder must produce an equal `Progress`, so a field added to one and not the other fails. `urls` passes the generated `comfy_low.models.JobUrls` through unwrapped, the same way `error` already passes `JobError` through. It is deliberately *not* re-exported at the `comfy_sdk` top 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 from `comfy_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.py` covers 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.py` is what guarantees the sync and async surfaces stay in step here, and it is not taken on faith: deleting `AsyncJob.queue_position` locally fails it with `Job/AsyncJob public surface diverges: sync-only=['queue_position']`, and it was restored before committing. ## Provenance - **Authored by:** agent-work loop - **Verified:** `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 references - **Deviations:** none against the plan; the acceptance gaps I could not close are under Residual below ## Residual Not fixed here, and actionable on its own: - **The cross-SDK surface-parity check between this SDK and the TypeScript one was never run.** It is the acceptance criterion that spans two repositories, and it lives outside this one — nothing in this repo can execute it, and the TypeScript side was not reachable from where this was built. What *is* verified is the in-repo parity test, which compares `Job` against `AsyncJob` only. 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. - **The TypeScript half is not in this PR** and this change does not depend on it — the two are expected to land together, and a reviewer merging only this one leaves the parity check failing in the other direction until the counterpart lands. - **`progress` has never been observed populated by a real server**, only by the stub. The request notes that Comfy Cloud returns `null` for 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, and `tests/integration/test_gateway_e2e.py` skips without `COMFY_BASE_URL` and `COMFY_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. - **Three of the eight properties have no null case to test**, so the "populated and `None`, on both classes" criterion is met literally for the five nullable ones only. `created_at`, `expires_at` and `urls` are 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** - Job handles now expose creation, start, completion, and expiration timestamps. - View current progress, queue position, metrics, and related job URLs. - Synchronous and asynchronous jobs provide the same lifecycle information. - Progress events include the currently executing node class. - **Documentation** - Added guidance describing job handle properties, cached state, refresh behavior, and unavailable progress values. - Clarified that job state updates through `refresh()`, `wait()`, or `result()`, and that absent progress is represented as `None`.  --- <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:00 +00:00

bsakaguchi referenced this issue

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

[PR #169] chore: sync vendored Comfy API v2 spec (cloud@d9624758) #170