Repository navigation
fix: surface two silently-dropped configs - #145
Merged
Merged
Conversation
Both bugs share a failure mode: the config looks accepted, nothing errors, and the run is quietly not what was asked for. `seeds:` under a workflow's `defaults:` was silently discarded. `StepDefaults` has `seed` (singular) and no `deny_unknown_fields`, so serde dropped the key without complaint — a 3-seed batch produced 1 image per step and looked fine. Unknown keys are now captured via #[serde(flatten)] and warned at parse time, with a specific hint for `seeds` since it is step-level only. This catches every future typo under `defaults:`, not just this one. `try_submit_via_socket` swallowed the worker connect error and fell back to one-shot mode, so without a running daemon every job reloads the model from scratch, forever, with no indication. Now hints once per process pointing at `modl worker start`. Docs: `--skip-existing` does not exist. Skipping already-generated sub-jobs is the default; `--force` opts out. Corrected in skills/modl/SKILL.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pedropaf
added a commit
that referenced
this pull request
Jul 22, 2026
`modl run` skips sub-jobs whose work is already on disk, but the completion key was only (prompt, seed, model). Re-running the same prompt/seed at a different guidance or step count matched the old key, so the run skipped generation and handed back the STALE image while printing `✓ complete` and exiting 0. Any guidance/steps sweep was silently corrupted — the same failure class as the silently-dropped configs (#145): it looks accepted, nothing errors, and the run is quietly not what was asked for. The completion key is now (prompt, seed, base_model, steps, guidance). The generate spec is built before the skip check so the resolved steps/guidance (after fast/lightning/default precedence) go into the key, then reused for execution — no double build. Guidance is an f32 (neither Eq nor Hash), so it is quantized to an integer (×1000, rounded) that round-trips stably through the YAML sidecar. Edit steps have no skip path, so no change there. Tests: completion_index_keys_on_steps_and_guidance (distinct settings are distinct keys; unseen settings miss) and quantize_guidance_is_stable_and_distinct. Verified end-to-end with SD-1.5: identical guidance skips, changed guidance regenerates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs found while running a 36-image prompt experiment today. They share a failure mode: the config looks accepted, nothing errors, and the run is quietly not what was asked for. That's the worst kind of bug for batch work — you only find out by auditing output counts.
1.
seeds:underdefaults:was silently discardedStepDefaultshasseed(singular) and nodeny_unknown_fields, so serde droppedseeds:without complaint.I hit this designing a seed-replication run. It only surfaced because I happened to
--dry-runfirst and noticedcount=1. Had I not, I'd have drawn conclusions from a third of the intended data.Unknown keys are now captured via
#[serde(flatten)]and warned at parse time, with a specific hint forseedssince it's step-level only:Warn rather than
deny_unknown_fieldsso existing workflows carrying stray keys keep running. This catches every future typo underdefaults:, not just this one.2. Missing persistent worker was invisible
try_submit_via_socketswallowed the connect error and fell back to one-shot mode. Without a running daemon every job reloads the model from scratch — indefinitely, with no indication. Every run I did today paid this, and I only noticed by readingexecutor.rs.Now hints once per process:
Not auto-started: that's a bigger decision about VRAM ownership and belongs in its own change.
3. Docs:
--skip-existingdoesn't existskills/modl/SKILL.mddocumented a flag that was never implemented. Skipping already-generated sub-jobs is the default;--forceopts out. (The same error is in the org-level CLAUDE.md, which isn't in this repo — fixed separately.)Testing
seedsbehaviour)cargo fmt+clippyclean via pre-commit🤖 Generated with Claude Code