fix(run): resolve the bundled default checkpoint at runtime (BE-2994)#519
fix(run): resolve the bundled default checkpoint at runtime (BE-2994)#519mattmillerai wants to merge 2 commits into
Conversation
`comfy run --prompt` (zero-config) hard-pinned one checkpoint and failed
wherever it was absent — Comfy Cloud and fresh local installs.
- Fix the stale pin: node "4" ckpt_name is now
v1-5-pruned-emaonly-fp16.safetensors (the gallery/tutorial SD1.5 file).
- Add a pure, offline-testable `resolve_default_checkpoint(workflow,
object_info)` in cql/default_workflow.py: if the pinned checkpoint is in the
target's CheckpointLoaderSimple enum → no change; else if the enum is
non-empty → substitute the first available checkpoint + emit a note; else
(positively-empty enum) → flag no_checkpoint; enum absent / object_info {} →
fail open (unchanged behavior).
- Wire it into both run paths (local + cloud) after object_info is fetched and
before preflight, guarded to the bundled default graph and skipped when the
user pinned the checkpoint via --set (honored verbatim). Thread a
checkpoint_user_set flag through the `preloaded` tuple.
- Emit a `checkpoint_substituted` event (--json) / pretty notice on
substitution; raise a new `no_checkpoint_available` error with an actionable
download/cloud hint when the target has zero checkpoints.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
mattmillerai
added
agent-coded
dosubot
Bot
added
the
size:L
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 4 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
|
|
||
| if res.note: | ||
| # Event fires in NDJSON/stream mode only; the pretty line covers humans. | ||
| renderer.event("checkpoint_substituted", message=res.note, checkpoint=res.substituted_to, where=where) |
There was a problem hiding this comment.
🟢 Low — The substitution notice is surfaced only via renderer.event (NDJSON/stream only) and pprint (pretty only). In single-envelope JSON mode (is_json() true, is_stream() false — reachable via COMFY_OUTPUT=json or agent auto-detection without the local --json that calls force_stream()), both branches are no-ops and the final envelope omits it too, so an agent's requested checkpoint is silently swapped with no indication anywhere in the output. Raised by 1 of 8 reviewers (claude-opus-4-8-thinking-xhigh edge-case).
There was a problem hiding this comment.
Deferring this one as out-of-scope for this PR (leaving unresolved for visibility). renderer.event() is a no-op whenever is_stream() is false, so single-envelope JSON drops every event, not just checkpoint_substituted — surfacing events in the final envelope is a general renderer capability, not something to special-case for this one note. The substitution is fully surfaced today in NDJSON stream mode (--json-stream) and pretty mode. Threading per-event notes into the final JSON envelope is a broader renderer enhancement worth its own change rather than bolting a bespoke path onto the checkpoint resolver here.
| if pinned in enum: | ||
| return workflow, CheckpointResolution() | ||
|
|
||
| replacement = enum[0] |
There was a problem hiding this comment.
🟢 Low — When the pinned checkpoint is absent, resolution silently picks enum[0] — the target's first-listed checkpoint — which may be architecturally incompatible with the bundled SD1.5 KSampler/latent settings (e.g. an SDXL model) and, on paid Comfy Cloud, launches an arbitrary model the user never selected and pays compute for. Consider preferring a same-family match rather than blindly taking the first entry. Raised by 1 of 8 reviewers (claude-opus-4-8-thinking-xhigh adversarial).
There was a problem hiding this comment.
Acknowledged as a real tradeoff, but declining the family-match heuristic here (leaving unresolved for visibility). Reliably preferring a same-architecture checkpoint needs metadata the ckpt_name enum doesn't expose — filenames alone don't distinguish SD1.5 / SDXL / Flux, so any name-based heuristic would be brittle and could itself select a wrong model. The substitution already emits a clear checkpoint_substituted note and points at --set checkpoint=<name>, and the new basename fallback (sibling thread) eliminates the most common false substitution. First-available-with-a-visible-note is the safer default; a metadata-driven family match belongs in a separate enhancement.
…2994) Address cursor-review findings on the runtime checkpoint resolver: - Cloud fails open on an empty checkpoint enum instead of hard-erroring no_checkpoint_available: Comfy Cloud provisions models per-job, so a zero-length cached enum can't prove the run would fail. Only the local path (enum reflects what's installed) treats empty as a hard stop. - Resolve the bundled default's checkpoint BEFORE emitting the prompt_preview event in the submit flow so the streamed audit trail advertises the graph we actually submit. --print-prompt stays a no-server-hit dry-run (documented) and prints the graph as-is. - Guard _checkpoint_enum against non-dict object_info (a hostile server returning `[]`) so it fails open rather than crashing with AttributeError. - Match a pinned bare filename against enum entries by basename so a checkpoint living in a subfolder (SD1.5/…safetensors) is found instead of triggering a needless substitution. - Escape the target-provided checkpoint name in the pretty substitution notice to prevent Rich markup injection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
ELI-5
comfy run --prompt "a fox"(no--workflow) runs a tiny built-in text2imgworkflow. That workflow had one specific checkpoint filename baked in. If the
machine (or Comfy Cloud, or a fresh install) didn't have that exact file, the
run failed — often with a cryptic server-side error. This PR makes the CLI look
at what checkpoints the target actually has and use one, so a zero-config run
produces an image wherever any checkpoint exists — and gives a clear,
actionable error when there are truly none.
What changed
Fixed the stale pin.
cql/data/default_text2img.jsonnode"4"ckpt_nameis nowv1-5-pruned-emaonly-fp16.safetensors(the file thegallery
default.jsonand the text-to-image tutorial use), replacing thestale
v1-5-pruned-emaonly.ckpt.Runtime checkpoint resolution. New pure, offline-testable helper
resolve_default_checkpoint(workflow, object_info)incql/default_workflow.py:available and emit a note;
object_infounfetchable ({}) → fail open (unchangedbehavior — submit and let the server answer).
Wired into both run paths (local
execute+execute_cloud) afterobject_infois fetched and before_preflight_validate. It only acts onthe bundled default graph (
workflow_name == "default_text2img") and isskipped entirely when the user pinned the checkpoint via
--set checkpoint=…/
--set 4.ckpt_name=…(honored verbatim; acheckpoint_user_setflag isthreaded through the
preloadedtuple).Actionable no-checkpoint error. When the target positively has zero
checkpoints, a new registered
no_checkpoint_availableerror (exit 1) with adownload hint (local) / gallery-template guidance (cloud) replaces the cryptic
server reject. Fail-open behavior is preserved when
object_infocan't befetched at all.
Substitution is surfaced as a
checkpoint_substitutedNDJSON event (--json)and a one-line pretty notice otherwise.
Acceptance criteria
default_text2img.jsonusesv1-5-pruned-emaonly-fp16.safetensors; fixtures/tests updated.--set checkpoint=<name>always honored verbatim.no_checkpoint_available+ exit 1;object_info{}→ fail-open submit (unchanged).--where localand--where cloud.resolve_default_checkpoint(present / substituted / empty-enum / user-pinned) + wiring tests;ruff/pytestgreen.Testing
uv run --extra dev pytest tests/comfy_cli/cql tests/comfy_cli/command/test_run_prompt.py tests/comfy_cli/command/test_run.py tests/comfy_cli/command/test_run_json.py tests/comfy_cli/command/test_run_cli.py tests/comfy_cli/output→ 488 passed.ruff format --checkclean on all touched files.Judgment calls / notes
resolve_default_checkpoint(...) -> (workflow, note|None), but a barenotecan't distinguish "pinned present (fine)" from "enum positively empty (error)" — both would beNone. I returned a smallCheckpointResolutiondataclass (note/substituted_to/no_checkpoint) so the caller can drive step 3 precisely and only hard-error when it positively knows the enum is empty.--print-promptis unchanged. The dry-run returns before the server round-trip, so it prints the bundled pin, not the runtime-resolved checkpoint (consistent with--print-prompt's existing "no server hit" contract). Theprompt_previewevent (emitted before resolution) likewise shows the pre-substitution graph; the explicitcheckpoint_substitutedevent documents the swap in the stream.checkpoint_substitutedis not added toschemas/run_event.json'stypeenum — that enum is already non-exhaustive (prompt_preview/convertedaren't in it either;additionalProperties: true), so I matched the established pattern rather than partially expanding it.no_checkpoint_availabledeny path is capability-enabling on net: runtime resolution makes--promptsucceed wherever any checkpoint exists (the opposite of a dead-end). The error fires only in the provably-empty-enum case — a state where the workflow genuinely cannot run (no checkpoint to load) — and its hint redirects to the real fix (install a model / gallery template /--set). Fail-open is preserved for every ambiguous case.Out of scope
Restructuring the bundled default to the gallery's newer Z-Image-Turbo graph (multi-file diffusion) is a larger, separate change and a product call on the blessed default model. Runtime resolution deliberately sidesteps that choice.