Skip to content

feat(deploy): add the deployment control plane - #803

Open
guill wants to merge 1 commit into
stack/05-build-surfacefrom
stack/06-deploy-control-plane
Open

feat(deploy): add the deployment control plane#803
guill wants to merge 1 commit into
stack/05-build-surfacefrom
stack/06-deploy-control-plane

Conversation

@guill

@guill guill commented Aug 27, 2026

Copy link
Copy Markdown
Member
Stack of 7 — review bottom-up (this is 803)
  1. feat(build): add packaging, sync and Builder client primitives #801feat(build): packaging, sync and Builder client primitives — base main
  2. feat(build)!: replace the legacy build and distribution surface #802feat(build)!: replace the legacy build and distribution surface (breaking)
  3. feat(deploy): add the deployment control plane #803feat(deploy): the deployment control plane   ← this PR
  4. feat(deploy): add the asset and job data-plane clients #804feat(deploy): the asset and job data-plane clients
  5. feat(deploy): add comfy deploy run #805feat(deploy): comfy deploy run
  6. fix(build): a pull no longer deletes definition data silently #810fix(build): a pull no longer deletes definition data silently
  7. fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10 #824fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10

Each 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: comfy deploy as one coherent lifecycle surface — up, status, ls, show, logs, events, scale, stop, start, delete and refs compute. A deployment serves a Build release, so this sits on the build spec and Builder client already in place.

Stacked on #802. Review from the bottom up.

Why one change and not one verb at a time: the verbs cross-reference each other in prose and the help-string guard resolves every mention against the live Typer tree — deploy_read points at up and status, deploy_status at stop and up, deploy_up at stop and scale. Splitting further would mean inventing transitional wording or registering placeholder commands, neither of which a reviewer benefits from.

What changed

This speaks the service's current release vocabulary, which is not the one the surface was first written against. comfy-deploy #7419 removed buildVersionId and distributionVersionId from the request type outright, and #7425 dropped the response mirror, so both spellings are now dead on the wire in both directions:

  • The create body sends releaseId. Sending buildVersionId binds nothing and the service answers 400 releaseId is required — no deployment could ever have been created.
  • The five reads that keyed off distributionVersionId (ls, resolve, status, and up's reconcile, supersede and soft-delete-generation passes) read releaseId. Three used .get(), so they failed silently: up never matched the live deployment and created a duplicate instead of reconciling it, and the idempotency generation never advanced.
  • deploy_ls.json and deploy_show.json declare releaseId and no longer declare either retired key.

min and max are optional throughout, matching the service: its create handler writes them only when the caller sent them, so a deployment made by the web UI or a direct API call legitimately carries neither. Requiring them refused that deployment outright from scale, status and up's reconcile branch. The same optionality reaches deploy_scale.json, and deploy_logs.json stops requiring capturedAt, which is absent until a health probe has run — precisely the window in which someone reads the logs.

Those four keys are a genuine spec-vs-handler disagreement on the service side: the OpenAPI marks them nullable: true, implying a null on the wire, while the generated Go tags them omitempty and omits the key. The schemas here follow the handler, since that is what a client actually receives.

Worker bounds on create. up sends both bounds when it creates, so the API's min <= max check has neither to skip. An omitted --max therefore defaults to max(1, --min) rather than a flat 1 — otherwise comfy deploy up --min 3 on a fresh release is refused against a ceiling the caller never set. An explicit --min 3 --max 2 is still refused, as it should be. Covered by test_a_created_deployment_lifts_the_default_ceiling_to_the_requested_floor; the no-bounds default of {min: 0, max: 1} is unchanged.

Also: registers the 16 codes these paths raise — deploy_endpoint_unknown is deliberately not among them despite sitting in the same registry section, because its only call site is the asset download path, which lands with the data plane. request_json gains a headers parameter the deploy client needs to send its own headers alongside the auth ones. run and its data-plane clients are absent by design; deploy_missing_input is worded for up alone until run exists.

Python 3.10 compatibility: deploy_runtime.py imports assert_never from typing_extensions, and deploy_resolve.py uses timezone.utc rather than the 3.11-only datetime.UTC. The declared floor stays >=3.10.

Test conventions: the command-registration tests read the option surface from the live Typer tree (deploy_up_support.option_names) rather than substring-matching --help output. Rendered help cannot be matched reliably — rich colorizes flag names whenever it detects a CI terminal, which splits --gpu across ANSI escapes and fails on GitHub Actions while passing locally. build_tree_support reads the build tree the same way.

Validation

  • CI (build job, Python 3.10) at this commit: 6669 passed, 38 skipped, 0 failed.
  • ruff check . / ruff format --diff . clean at CI's pin (0.15.15).

Contradicts: the retired buildVersionId / distributionVersionId request and response spellings, which are already dead on the service side.

@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Aug 27, 2026
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9248179a-4b0a-4ca9-92a8-11ebc90e0cd0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

`comfy deploy` as one coherent lifecycle surface: up, status, ls, show, logs,
events, scale, stop, start, delete and refs compute. A deployment serves a
Build release, so this sits on the build spec and Builder client already in
place.

Lands as a single change rather than one verb at a time because the verbs
cross-reference each other in prose and the help-string guard resolves every
mention against the live Typer tree -- deploy_read points at up and status,
deploy_status at stop and up, deploy_up at stop and scale. Splitting further
would mean either inventing transitional wording or registering placeholder
commands, neither of which a reviewer benefits from.

Speaks the service's current release vocabulary, which is not the one this
surface was first written against. comfy-deploy #7419 removed
`buildVersionId` and `distributionVersionId` from the request type outright
and #7425 dropped the response mirror, so both spellings are now dead on the
wire in both directions:

- the create body sends `releaseId`; sending `buildVersionId` binds nothing
  and the service answers 400 `releaseId is required`, so no deployment could
  ever have been created
- the five reads that keyed off `distributionVersionId` (ls, resolve, status,
  and up's reconcile, supersede and soft-delete-generation passes) read
  `releaseId`. Three of those used `.get()`, so they failed *silently*: `up`
  never matched the live deployment and created a duplicate instead of
  reconciling it, and the idempotency generation never advanced
- deploy_ls.json and deploy_show.json declare `releaseId` and no longer
  declare either retired key

`min` and `max` are optional throughout, matching the service: its create
handler writes them only when the caller sent them, so a deployment made by
the web UI or a direct API call legitimately carries neither. Requiring them
refused that deployment outright from `scale`, `status` and `up`'s reconcile
branch. The same optionality reaches deploy_scale.json, and deploy_logs.json
stops requiring `capturedAt`, which is absent until a health probe has run --
that is precisely the window in which someone reads the logs.

Those four keys are a genuine spec-vs-handler disagreement on the service
side: openapi.yaml marks computeConfig, deletedAt, deletedBy and endpointUrl
`nullable: true`, implying a null on the wire, while the generated Go tags
them `omitempty` and omits the key. The schemas here follow the handler,
since that is what a client actually receives.

Registers the 16 codes these paths raise. deploy_endpoint_unknown is NOT
among them despite sitting in the same section of the registry: its only call
site is the asset download path, which lands with the data plane.

request_json gains a headers parameter, which the deploy API client needs to
send its own headers alongside the auth ones.

`run` and its data-plane clients are deliberately absent; deploy_missing_input
is worded for `up` alone until `run` exists.

@skishore23 skishore23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wire contract verified against the comfy-deploy checkout containing #7419/#7425: create sends releaseId + computeConfig, every read keys off releaseId, min/max/capturedAt are omitempty, the idempotency key is namespaced and the server 409s on key reuse with a different body, up never touches a deployment for another release, and http.py applies auth after caller headers so Idempotency-Key can't override Authorization. Three small contract-level defects, two of which contradict the PR body:

1. schemas/deploy_status.json:30 and schemas/deploy_up.json:32 still "required": ["gpuClass", "region", "min", "max"], while deploy_types.compute_config omits min/max when the server omitted them. comfy deploy status --json against a UI-created deployment emits computeConfig: {gpuClass, region} and fails its own published schema. deploy_scale.json got the optionality; these two didn't.

2. The edit paths send min without max for a bounds-free deployment (deploy_up.py:169-175 reconcile, deploy_lifecycle.py:86-91 scale, validator at deploy_api.py:69-74 skips the min <= max check). The server does not skip it: update.go:346-355 applies effective defaults (min→0, max→1) and 400s computeConfig.min must not exceed computeConfig.max. So comfy deploy up --min 3 / deploy scale --min 3 on a UI-created deployment is refused against a ceiling the user never set — the exact failure the PR fixed for create via max(1, --min). The tests can't see it because the fake server doesn't validate.

3. Hints name verbs the tree doesn't accept. deploy_up.py:187,199 and deploy_status.py:204 print comfy deploy stop <id> / comfy deploy scale <id>, but both take --deployment; the positional is a build path, so an agent obeying the hint gets build_spec_not_found. deploy_ambiguous_deployment's hint says pass --deployment <id> but is raised from up and status, which have no such option. In this repo hints are the docs, so these should be right before merge.

Non-blocking: poll_deployment (deploy_runtime.py:65-73) is while True with no ceiling and unhealthy/stopping/queued/provisioning are all non-terminal, so --watch on a stuck-provisioning deployment spins forever and Ctrl-C yields click's Aborted! with no envelope in --json mode — a --timeout would close it. deploy_api.py:107-110 maps only HTTPError/TimeoutError/URLError, so a mid-body IncompleteRead/ConnectionResetError is a traceback (same pattern as #802/#804). The diff registers 17 codes, not 16.

Tests on the branch with the mix_stderr shim: 124 + 456 passed; ruff clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants