feat(deploy): add the deployment control plane - #803
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 |
7efdfed to
344fbba
Compare
344fbba to
b7b3944
Compare
b7b3944 to
dd318d7
Compare
`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.
dd318d7 to
7f9a3b4
Compare
skishore23
left a comment
There was a problem hiding this comment.
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.
Stack of 7 — review bottom-up (this is 803)
feat(build): packaging, sync and Builder client primitives — basemainfeat(build)!: replace the legacy build and distribution surface (breaking)feat(deploy): the deployment control plane ← this PRfeat(deploy): the asset and job data-plane clientscomfy 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:
comfy deployas one coherent lifecycle surface —up,status,ls,show,logs,events,scale,stop,start,deleteandrefs 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_readpoints atupandstatus,deploy_statusatstopandup,deploy_upatstopandscale. 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
buildVersionIdanddistributionVersionIdfrom the request type outright, and #7425 dropped the response mirror, so both spellings are now dead on the wire in both directions:releaseId. SendingbuildVersionIdbinds nothing and the service answers 400releaseId is required— no deployment could ever have been created.distributionVersionId(ls, resolve,status, andup's reconcile, supersede and soft-delete-generation passes) readreleaseId. Three used.get(), so they failed silently:upnever matched the live deployment and created a duplicate instead of reconciling it, and the idempotency generation never advanced.deploy_ls.jsonanddeploy_show.jsondeclarereleaseIdand no longer declare either retired key.minandmaxare 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 fromscale,statusandup's reconcile branch. The same optionality reachesdeploy_scale.json, anddeploy_logs.jsonstops requiringcapturedAt, 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 themomitemptyand omits the key. The schemas here follow the handler, since that is what a client actually receives.Worker bounds on create.
upsends both bounds when it creates, so the API'smin <= maxcheck has neither to skip. An omitted--maxtherefore defaults tomax(1, --min)rather than a flat1— otherwisecomfy deploy up --min 3on a fresh release is refused against a ceiling the caller never set. An explicit--min 3 --max 2is still refused, as it should be. Covered bytest_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_unknownis 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_jsongains aheadersparameter the deploy client needs to send its own headers alongside the auth ones.runand its data-plane clients are absent by design;deploy_missing_inputis worded forupalone untilrunexists.Python 3.10 compatibility:
deploy_runtime.pyimportsassert_neverfromtyping_extensions, anddeploy_resolve.pyusestimezone.utcrather than the 3.11-onlydatetime.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--helpoutput. Rendered help cannot be matched reliably — rich colorizes flag names whenever it detects a CI terminal, which splits--gpuacross ANSI escapes and fails on GitHub Actions while passing locally.build_tree_supportreads the build tree the same way.Validation
buildjob, 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/distributionVersionIdrequest and response spellings, which are already dead on the service side.