Repository navigation
fix(deploy): status shows why a deployment failed and its worker counts (BE-16742) - #927
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe status command retrieves full deployment details and displays deployment errors and serving sample age. The logs command explains when a deployment has no log and points to deployment events. Tests cover both commands, and the changelog records the changes. ChangesDeployment CLI fixes
Merge Risk: ⚪ Minimal · up to The deployment status and logs changes are ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Swarmhost agentic reviewThe detailed evaluation is available to employees in the internal Slack review thread. Evaluation budget remaining for this pull request: 96 automatic and 99 manual. Updated by Swarmhost's agentic review process. |
james00012
left a comment
james00012
left a comment
There was a problem hiding this comment.
Holding on the failure reason in terminal output. Reads as: status reads the chosen deployment in full, and logs points at events when the server has no log. Head 8c07582, judged as comfy-cli's owner.
Must
- Correctness: a failed deployment still shows no reason in the terminal
comfy_cli/command/deploy_status.py:212. Only JSON output carries the error, so a person still sees the problem the ticket names.
Optional
- Correctness: worker counts show a timestamp, not the sample's age
comfy_cli/command/deploy_status.py:178. The ticket's criterion asks for the age. - Correctness: logs tells a booting deployment that no container has started
comfy_cli/command/deploy_read.py:100. The server records no capture until the health check ends. - Tests: the new test gives a failed deployment worker counts, which the server never sends
tests/comfy_cli/command/test_deploy_status.py:202. Add a ready case and a terminal-output failed case.
Clean: Modularity, Scope, Verbosity, Resilience, Access, Reachability, Contract, Rollout.
Native review: 8 findings, folded into the list above.
Checked: The single-deployment read on comfy-deploy main returns the error and the sample, scoped to the workspace. Both new tests fail on main. The red GPU job fails on main too.
Bots: CodeRabbit and Swarmhost raised nothing; cursor-review did not run.
The deployment list is a summary without error or serving, so status always printed no failure reason and "not sampled yet". Without --watch the chosen deployment is now read again from the single-deployment endpoint; --watch already polled it.
A deployment that failed while staging its models never booted a container, so it has no log; the empty output now says so and names the command that does show what happened.
…E-16742) Reading the deployment in full put `error` and `serving` in the payload, but only `--json` showed them, so the terminal still answered "why did my deploy fail?" with nothing. Print the reason beside the status line, and print how old the worker-count sample is beside the counts, which is what the criterion asked for: the counts are a snapshot refreshed on the control plane's own schedule and a deployment whose endpoint stopped answering keeps the last one, so a bare timestamp does not say whether they still describe now. An unparseable `sampledAt` degrades to "age unknown" rather than raising out of the renderer, where nothing catches ValueError. `deploy logs` no longer tells a booting deployment that no container has started. The log is written when the health check finishes, so an empty one is equally the state of a deployment still coming up; the line now names both and points at `comfy deploy events` to tell them apart. Tests: the failed-deployment fixture no longer carries worker counts, which the server never sends for a status that has released its compute; the full read is covered once through a failed deployment's error and once through a ready one's counts, and the reason is asserted through the terminal, not the JSON.
james00012
left a comment
james00012
left a comment
There was a problem hiding this comment.
Approving. Reads as: status reads the chosen deployment in full and prints its failure reason and the sample's age; logs explains an empty log and points at events. Head 3ceb13e, judged as comfy-cli's owner.
Optional
- Contract: the deploy skill still tells agents status cannot say why a deployment failed
comfy_cli/skills/comfy-deploy/SKILL.md:219. Agents read that doc, and status now prints the reason. - Tests: one assertion guards against text that never shipped
tests/comfy_cli/command/test_deploy_read.py:195. Only the round 1 commit printed that line, so the assertion cannot fail.
Clean: Modularity, Correctness, Scope, Verbosity, Resilience, Access, Reachability, Rollout.
Native review: 9 findings, folded into the list above.
Checked: All four round 1 items are fixed in the pushed code. The reason prints for failed, stop failed and under watch. Seven new tests fail on main, and all 50 pass on the head.
Bots: CodeRabbit had not finished this head and raised nothing on the last; Swarmhost raised nothing.
3ceb13e to
1cc4c4f
Compare
3 participants
comfy deploy statusalways printed no failure reason and "Serving: not sampled yet", because it read the deployment list, whose rows leave outerrorandserving. Without--watchit now reads the chosen deployment from the single-deployment endpoint, as--watchanddeploy showalready do.comfy deploy logswith nothing captured now says no container has started and points atcomfy deploy events.Tests: a new test whose list rows omit
errorandservingchecks that status reports both; the logs test checks the new line.BE-16742