feat(deploy): add the asset and job data-plane clients - #804
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
6dc9cfc to
03aae12
Compare
d02f65b to
cdee048
Compare
cdee048 to
34dd70f
Compare
The clients a workflow submission needs, with no command wired to them yet: - hashing: blake3 content hashing (new blake3 dependency, with the lockfiles bumped in the same change so no layer imports an undeclared package). - deploy_assets: dedupe local assets by digest, upload what is missing. - deploy_jobs: submit a job, poll it, cancel it. - deploy_events: the job event stream. - deploy_download: fetch completed outputs. transfer._stream_http_one takes an injectable opener so the download path can reuse the shared streaming logic under its own HTTP-only opener. Registers the 9 codes these clients raise. deploy_endpoint_unknown moves here from the control-plane group in the registry -- its only call site is deploy_download -- while deploy_job_canceled and deploy_workflow_format_ui stay out, because only `deploy run` and the workflow reader raise them. A 422 `missing_asset` maps to deploy_asset_missing rather than falling through to the generic 422 row. The API lists it beside invalid_workflow, and without its own entry a run whose workflow is perfectly valid — but which names an asset id this account cannot mint — was reported as an invalid workflow, sending the reader to edit a graph that has nothing wrong with it. deploy_asset_missing's registry text widens to name both of its causes. None of these modules emits a `command=` envelope label, so none of them depends on a Typer verb that does not exist yet.
34dd70f to
18c4c2b
Compare
skishore23
left a comment
There was a problem hiding this comment.
Approving. This is the layer that had to be right, and it is — verified against the platform-gateway checkout (2026-08-25) and by adversarial testing on the branch: digest is blake3:<64 hex> matching the gateway's canonical regex; uploads stream in 1 MiB chunks with Content-Length equal to the streamed bytes, multipart field order the gateway requires, filename quotes/CR/LF neutralised, Bearer only to the endpoint; download origin checks are exact set membership (34 hostile URLs — subdomain suffixes, userinfo, \\@, %2E, punycode, ports, http, file:, private IPv6 — all rejected), the redirect handler refuses non-https/untrusted origins and strips auth on every hop, and the atomic part-file write, 10 GB cap, no-overwrite suffixing and symlink refusal are reused from transfer.py. The CodeQL hit is on the test's in str(...) assertion, not production. SSE parser is bounded (3 MiB lines, no terminator, bad UTF-8, deep nesting, comment floods) and the 30 s idle timeout is sound against the gateway's 15 s pings.
Three non-blocking notes:
- Connection-level failures escape the error mapping —
deploy_jobs.py:164,deploy_assets.py:200,223,250,deploy_events.py:210,267catch(TimeoutError, URLError), but urllib only wrapsrequest(); a socket closed before the status line raiseshttp.client.RemoteDisconnectedand a garbled one raisesBadStatusLine(reproduced on loopback). Onsubmit_jobthat's the LB-drain case the registry reserves fordeploy_job_submit_unknown("do not resubmit"), but #805's catch-all reports it asdeploy_server_error, andBadStatusLineis caught by nothing. Same pattern in #802/#803 — one shared except tuple forhttp.client.HTTPException/ConnectionErrorwould close all of them. uv.lock/pylock.tomlcarry cp311–cp314 blake3 wheels only, no cp310 (PyPI has them), while CI's matrix is["3.10"]andrequires-python >= 3.10.uv sync --lockedon 3.10 builds blake3 from the Rust sdist (confirmed with--no-build-package blake3 --dry-run); CI passed on a warm cache.uv lock --upgrade-package blake3+ regenerate pylock.deploy_jobs._retry_afteraccepts any non-negativeRetry-Afterand_sleepis uncapped, whiledeploy_events._retry_aftercaps at 10 s — aRetry-After: 86400on a 429 parks the CLI for a day. Dormant today (gateway sends none on job 429s) but worth matching.
Minor: no cancel client lands here (it's in #805) despite the body's "submit/poll/cancel". Tests on the branch: 214 passed, 1 skipped; ruff clean.
Two defects from the #802 review, and a third of the same shape found while fixing them. `--from-workflow` shipped the user's workflow path to analytics verbatim. Redaction is keyed on the Typer parameter name and `_scrub_value` returns a bare local path unchanged by design, so a path is redacted by being named in `_SENSITIVE_EXACT` or not at all. `from_snapshot` was named; its interchangeable sibling was not. `update --from-snapshot` discarded the base image the importer resolved. `merge_definition` carried every stored key outside the scanned set forward *over* the incoming value, so a snapshot resolving `cuda12-py312` kept building on the stored `cuda12-py311` -- and the diff reported no change at all, so nothing said so, while the advisory reported that image's Python as satisfied. The carry now tests the incoming definition rather than the key set alone: a local scan states no `baseImage`, so the stored one still survives a plain `update`, but an importer that resolves one now wins and the change appears in the diff the user confirms. `--base-image` is applied after the merge and still outranks both. `_merge_entry` carried entries the same way, and a snapshot resolves each pack's `commit`, which no scan reports. The stored SHA outranked the imported one and the entry then read as unchanged, so an import could never refresh a pinned commit. Same fix, same property: the scan path is byte-identical because a scan states only scanner-owned keys. Not addressed here, all called non-blocking in review: connection-level failures escaping the error mapping (one shared except tuple spanning this PR, #803 and #804 -- better as one focused change than three partial ones); the unguarded `CliRunner(mix_stderr=False)` call sites, which CI's locked resolve pins away from; and `_unrenderable` pointing at a `--json` payload that carries only rendered advisories. The CodeQL chmod alerts on these tests are false positives: every flagged line is a `finally` restoring owner-only bits, not a world-readable mask.
Two non-blocking notes from the #804 review. `uv.lock` carried cp311-cp314 blake3 wheels and no cp310, while `requires-python` is >=3.10 and every CI job pins 3.10 -- so `uv sync --locked` had to build blake3 from the Rust sdist on the only Python the suite actually runs on. PyPI publishes 13 cp310 wheels for 1.0.9; the lock simply lacked them. Confirmed before and after with `uv sync --locked --python 3.10 --no-build-package blake3 --dry-run`, which refused with "has no binary distribution" and now resolves. No package version moves; the only other edit is uv narrowing `exceptiongroup`'s `typing-extensions` marker to `python_full_version < '3.11'`, a no-op given both referents are already gated below 3.11. `pylock.toml` is regenerated with the command its own header records. That diff is larger than the blake3 rows because the committed file was already stale against the committed `uv.lock` -- five versions behind and missing four packages -- so it moves onto `uv.lock` rather than resolving anything independently. `deploy_jobs._retry_after` accepted any non-negative `Retry-After` and slept it unbounded, so a `Retry-After: 86400` on a 429 would park a foreground `submit_job` for a day with no output. It is now clamped to the same ceiling `deploy_events` already applied to its own. Deferred to known-issues.md as items 15-16: the shared except tuple for connection-level failures, which spans this PR, #802 and #803 and belongs in one focused change; and the duplicated clamp literal.
Stack of 7 — review bottom-up (this is 804)
feat(build): packaging, sync and Builder client primitives — basemainfeat(build)!: replace the legacy build and distribution surface (breaking)feat(deploy): the deployment control planefeat(deploy): the asset and job data-plane clients ← this PRcomfy deploy run#805 —feat(deploy):comfy deploy runfix(build): a pull no longer deletes definition data silentlyfix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10Each PR is based on the branch below it, so its own diff is only its commit. Merge in order.
Rebased onto
main@c1fa1f4. The original bottom PR (#800, raising the Python floor to 3.11) was closed — see that thread for why; the stack is 3.10-compatible instead.TL;DR: The clients a workflow submission needs — asset dedupe/upload, job submit/poll/cancel, the event stream, and output download — with no command wired to them yet.
Stacked on #803. Review from the bottom up; this PR's diff is the 15 files below.
What changed
blake3dependency, with the lockfiles bumped in the same change so no layer imports an undeclared package._stream_http_onetakes an injectable opener so the download path can reuse the shared streaming logic under its own HTTP-only opener.Error registry: registers the 9 codes these clients raise.
deploy_endpoint_unknownmoves here from the control-plane group — its only call site isdeploy_download— whiledeploy_job_canceledanddeploy_workflow_format_uistay out, because onlydeploy runand the workflow reader raise them.A 422
missing_assetmaps todeploy_asset_missingrather than falling through to the generic 422 row. The API lists it besideinvalid_workflow, and without its own entry a run whose workflow is perfectly valid — but which names an asset id this account cannot mint — was reported as an invalid workflow, sending the reader off to edit a graph that has nothing wrong with it.deploy_asset_missing's registry text widens to name both of its causes.None of these modules emits a
command=envelope label, so none depends on a Typer verb that does not exist yet.Python 3.10 compatibility:
deploy_events.py's private_WatchStateuses(str, Enum)— the existing house pattern — rather than the 3.11-onlyenum.StrEnum. It is compared only by identity in thematchstatement and never stringified, so thestr()difference between the two does not apply.assert_nevercomes fromtyping_extensionshere and intest_deploy_assets.py. The declared floor stays>=3.10.Validation
buildjob, Python 3.10) at this commit: 6731 passed, 38 skipped, 0 failed.ruff check ./ruff format --diff .clean at CI's pin (0.15.15).Contradicts: nothing.