diff --git a/CLAUDE.md b/CLAUDE.md index 26c7053c..21611686 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -83,6 +83,11 @@ common` by `GET /api/assets`, and is written to only when a call says so Reserved names: `workflows`, `prompts`, `assets`, `outputs`, `exports`, `common`. +The web UI has a page for it: `ui/src/lib/pages/AssetsPage.svelte` (#165) +reads `GET /api/assets` and shows the library the way the gallery shows +outputs, tagged by `origin` so a shadowed or read-only entry is visible +before a 403 explains it. + ### Workflow sources `dw/workflow_sources.py` is the server's workflow search path: the writable @@ -190,6 +195,29 @@ The same conventions, written for an agent composing a workflow over MCP, are the `Authoring a workflow from an agent` section of docs/WORKFLOW_GUIDE.md; change both when one changes. +### LTX-2.5 IC-LoRAs + +`templates/ltx2/generative-upscale` was the only IC-LoRA use in the catalog; +three more conditioning templates join it (#151, #152), all through +`LTX2InContextPipeline` + `LTX2ReferenceCondition`, all at +`reference_downscale_factor: 1` (the upscaler's is 2). `reference-sheet` +drives Ingredients — the family's only identity route, and the first two +templates here whose reference is a file the workflow did not make; the sheet +is a still, so a `loop_frames` step (`dw/tasks/video_utils.py`, the video +analogue of `loop_audio`) laps it into the static video the LoRA reads +through its 121-frame bucket. `restore-deblur` and `restore-decompression` +each invert one defect and no other. Every number in the three is the vendor +card's and is pinned by `tests/test_ltx2_ic_loras.py`; the trained caption +form is a *different* genre from a T2V shot caption, so those stored prompts +are tagged `ic-lora` and `tests/test_ltx_prompt_library.py` checks them +against their own convention rather than the 150-220-word paragraph rule. +The weights are `gated: auto` on Hugging Face — per repo, so a box that pulls +one can still 403 on another. A `loras` entry counts toward +`plan.downloads_required` (`_collect_sources`, `dw/plan.py`): it names its repo +under `model_name` directly rather than through `from_pretrained_arguments`, so +the walk used to miss it and a box holding every base weight but not the +IC-LoRA answered `[]` and then pulled it mid-run. + ### Quantization Support Quantization configs are defined per-component in workflow JSON and instantiated in `config_objects.py`. Supported frameworks: BitsAndBytes, TorchAO, GGUF, SDNQ, optimum-quanto. The `config_type` field is a free-form string — new quantization backends work automatically via dynamic import. @@ -266,6 +294,23 @@ same reason - default setup cannot load a pack. take `name=value` strings, and a string handed to a list variable is comma-split - so `shots` can only be supplied over the API/MCP (a JSON body); `python -m dw.run` runs the templates' default list +- **A reference name is checked for its shape before the queue, and `@` is + part of it** — a `for_each` member is `@` and the files it + writes carry that `@` in their base name, which `OUTPUT_REFERENCE_PATTERN` + and `ASSET_REFERENCE_PATTERN` refused: a whole class of files the server + itself named could not be named back to it, so `output:` on a shot a + list-driven template produced forced a re-render or an `upload_asset` round + trip (#162). `@` is safe in a path — not a separator, not `..`, and + containment is still `validate_path`'s — and a name still may not *start* + with one. The other half is that the refusal arrived at run time, after the + queue, from a message that described a *valid* name and never said which + character it objected to: `reference_name_errors` (`dw/reference_names.py`) + now checks the shape of every `asset:`/`prompt:`/`output:` reference in the + definition in `validation_errors`, and `_name_fault` (`dw/security.py`) + names the offending character and position. Shape only — *existence* + depends on the workspace and on what pruning has taken, so it stays where it + was: the validate route resolves the caller's `arguments` against the + workspace, and the definition's own references resolve at run time - **Cartesian product explosion** — multiple `previous_result` references multiply: 4 images × 3 masks = 12 iterations - **Component sharing requires exact key matching** between `shared_components` and `reused_components` - **Built-in workflows** need explicit argument mapping: `"prompt": "variable:prompt"` @@ -368,6 +413,118 @@ same reason - default setup cannot load a pack. one entry in the table; `tests/test_task_domains.py` pins every entry to a real parameter of a real command so a rename cannot leave one checking nothing +- **`cost` is curated, `observed` is derived, and they are different fields** — + `dw/workflow_schema.json` defines `cost` as *"Never derived"*, so nothing + writes one; `dw/server/observed_cost.py` reports a sibling built from this + box's own `jobs.sqlite` rows (#93). Four rules, each a way the naive median + would lie: runs are bucketed by the workflow's declared `cost_drivers` (a + list driver on its *length*, so two four-shot runs are comparable however + different their prompts) and the bucket reported is the one the *defaults* + give, keeping it comparable to a curated figure; `cold_minutes` and + `warm_minutes` are separate, each with its own run count, and only the cold + one is comparable to `cost` (wall clock including model load); a run whose + every manifest entry is `reused` wrote nothing and is excluded; and a run + whose persisted events hit `MAX_PERSISTED_EVENTS` without a `loading` phase + is `unclassified_runs` rather than assumed warm. Everything comes off the + job row in one query, so a figure survives a pruned run directory, and the + aggregate caches against `JobHistory.watermark()` rather than a file mtime — + a job landing changes every figure and changes no file. The compact listing + carries only `observed_minutes`/`observed_runs` (#101 budget); the full + block is in the full listing and `GET /api/workflows/{name}/variables`. The + raw `GET /api/workflows/{name}` is left verbatim, since the editor saves + what it reads back. A `cost_drivers` entry naming no declared variable is + dropped, and `tests/test_observed_cost.py` sweeps the catalog for one. + `plan.estimate` quotes the observed figure ahead of the curated one + (`basis: "observed"`, with `runs`) — `basis: "unknown"` has to mean nobody + has a number, not nobody curated one (#154). Only the *cold* median, only + when the history is this backend's, and only for the bucket the caller's + own arguments fall in (`ObservedCosts.observed(name, definition, + arguments)`); a resized list finds no bucket and falls back to the curated + figure. Nothing is added for a composed child, since an observed run + already ran it. An inline definition has no catalog name, so no history +- **An H3 adapter is checked against the partition its step denoises on** — + `ref2va` loads `transformer_ref` alone, so diffusers puts whatever + `lora_weight_name` names straight onto it: an FL2VA turbo LoRA on a + reference step runs, succeeds, and only retains identity worse (#149, + #155). `dw/adapter_compatibility.py` refuses the mispairing in + `validation_errors` (so `POST /api/validate` and the pre-queue check both + catch it, at `arguments.` when the caller supplied it) and *warns* + on a file name carrying neither `ref2v` nor `fl2v` — the name of a future + reference-trained checkpoint cannot be predicted, so the escape hatch + stays open while the one documented mistake is closed. The workflow names + and the partition each denoises against are diffusers' + (`MiniMaxH3Blocks._workflow_map`, pinned by `tests/test_h3_adapters.py`); + the file-name convention is MiniMax's and is swept against the catalog's + own defaults +- **An elided step says whether anyone decided it** — `warn_elided` used to + tell every caller their reference was probably misspelled, including the + one who deliberately passed `singer_reference` and so bought the elision + `music-video` advertises (#146, #157). `overriding_variables` + (`dw/elision.py`) compares the definition as *written* against the + substituted steps: a step reached only through a variable whose value no + longer names it was replaced on purpose, and its record carries + `overridden_by` and drops the diagnosis. A variable no step reads is not + how the step was reached, so that case keeps the old wording +- **A deliverable with no audio headroom warns** — a track at or above + −0.5 dBFS is written anyway and said out loud (`warn_without_headroom`, + `dw/result.py`, kind `audio_no_headroom`), for both a saved audio file and + a muxed video: a clipped file succeeds, and a consumer that cannot listen + had `peak_dbfs` with no rule to read it against — `get_gallery_metadata`'s + hint taught the near-silent end of the range only (#158). A warning, not a + gain change: what level a deliverable sits at is the workflow's to decide, + and `normalize_audio` is the step that decides it. The two Music 3 + templates decide it now (#159) - `music` and `music-video` peak-normalize + to -1 dBFS, the level `assemble-and-score` has always used, because the + warning was firing on their own defaults every run. `music-video` + normalizes only the track going into the mux, not the slices that condition + the shots, so the picture is unchanged; `music`'s deliverable moves to the + new `balanced` step, which renames the file an `output:` reference names +- **A deliverable is measured as written, not as handed to the writer** — + `warn_without_headroom` reads the waveform, and the encoder sits downstream + of it: a song normalized to exactly -1.0 dBFS came back out of + `music-video`'s AAC mux at **+0.94**, so a clean default run shipped a + clipped file and nothing warned (#161). `warn_if_written_above_full_scale` + (`dw/result.py`, kind `audio_clipped`) probes the file it just wrote and + warns when it decodes at or above 0 dBFS — whatever the encoder did, that + is the number a consumer's decoder sees. Only for a file that can carry a + soundtrack, and silent when `warn_without_headroom` already spoke for that + file, since two warnings would be two answers to one mistake. The encode's + overshoot is material-dependent — about 0.1 dB on an mp3 and about 1.9 dB + on the AAC mux of the same song — so no target chosen up front can be + *known* to be enough, which is why reading the file back is the half that + stops the next instance. The half that fixes this one: every template + whose deliverable ends in a `pair_audio` mux (`music-video`, + `assemble-and-score`, `dissolve-between-shots`) normalizes to **-3 dBFS**; + `music`, an mp3, keeps -1 +- **A variable's bound is declared by the author, checked three times** — a + model's own rule about a value (H3's `num_frames` is `17 * n + 5` from 124 + to 345) is a property of the model, so it lives in the workflow rather than + in engine code, as a `variable_constraints` entry (`dw/variable_constraints.py`). + One shape, not two: it takes a chain step's `frame_snap` field names, and a + chain writes `"frame_snap": "constraint:num_frames"` rather than repeating + the numbers. `snap: "up"` rounds an off-grid value to the next legal one + and warns (at validation *and* through `emit_warning`, so it reaches the + job's `warnings`); without `snap` an off-grid value is refused. The bounds + hold for the value the run will use, matching diffusers' own + `align_num_frames`, which snaps before it range-checks — so 108 is accepted + (it becomes 124) and 346 refused (it would become 362). LTX-2.5's templates + declare the `8 * n + 1` grid with *no* `snap`, because those pipelines floor + an off-grid count rather than raising: rounding up here would be a second + silent change to the length. Checked in `validation_errors` (so + `POST /api/validate`, `validate_workflow` and the pre-queue check all + refuse it at `arguments.` / `variables.`), at run time in + `apply_constraints` before anything loads, and reported beside the default + by `list_workflows` (terse) / `get_workflow(variables_only=true)` — that + last part is what stops the next consumer picking 61 (#96). + `tests/test_variable_constraints.py` sweeps the whole catalog and pins every + declared number to the diffusers symbol it derives from. A constraint key is + a plain variable name and is matched wherever a value by that name sits - + top-level variable *or* a field of a `for_each` entry (#145), the latter only + where a step consumes that field as `item:` (`entry_constraint_fields`), + so the bound follows the value into the pipeline argument rather than the + name into the JSON. An entry violation is reported at + `arguments.shots[0].num_frames`, and the rule is reported beside the field in + the catalog's `lists` block as well as in `constraints` - **Step cache**: a process-wide singleton (`dw/step_cache.py`) consulted by every `Workflow.run`, including server jobs; entries are keyed by `(workflow id, step name)` and validated against the output *root*, never the per-run directory - a run directory is new every execution and would defeat the cache; disabled entirely when the workflow sets no `seed`; a hit reports the earlier run's files with `reused: true` and writes nothing new; `memory clear` drops it. This is why "Run again" on a seeded workflow finishes instantly and generates nothing - the job page says so when every step was reused, and `POST /api/jobs/{id}/rerun` with `{"new_seed": true}` (MCP `rerun_job(new_seed=True)`) draws a fresh seed into the workflow's seed variable, which is the way to get a different image diff --git a/docs/MCP.md b/docs/MCP.md index 6fbf1d87..6994890c 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -226,7 +226,7 @@ when no single workflow covers it. | `get_health()` | — | Check that the server is alive, and which machine answered: `version`, `device`, whether the worker process is up, the job running now and the queue depth | | `get_server_info()` | — | What this installation can do and where it keeps things: `device` (the accelerator a run will use), `version`, the `workspace` this session is working in and the workflow/asset/output/prompt `directories` of *that* workspace, the bind address and port, whether a token is required, and whether MCP is mounted. Check the device before authoring - a CUDA-only choice (bitsandbytes, `torch.compile`, flash attention) is not available on an `mps` or `cpu` server | | `list_jobs(limit=20, status=None, workspace=None)` | optional `limit` (newest N), `status` (one state or a comma-separated set of `queued`, `running`, `succeeded`, `failed`, `cancelled`), `workspace` | List queued, running and recent jobs, **newest first**. Bounded by default: the unbounded listing was over a client's tool-result limit on a server with a few months of history, which made it a tool that could not be called at all. `total` says how many matched and `truncated`/`next` say so when the answer was cut - raise `limit` or narrow with `status`. Without `workspace`, a named workspace lists its own jobs and the default one lists every job the server holds | -| `list_gallery(limit=50, subfolder=None, workspace=None)` | `limit`, `subfolder`, `workspace` | List generated output files, newest first. A name is `//`, where `` may sit in the subfolder the step chose (`final/episode.mp4`); each entry carries `folder` (the workflow) and `subfolder` (by convention `final` or `intermediate`, `''` when the step chose none, any path the workflow wrote otherwise), and `subfolder="final"` lists only deliverables. Each entry also carries a ready-made `url`, already scoped to the workspace that made it - a hand-built `/outputs/` URL 404s for anything but the default workspace. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | +| `list_gallery(limit=50, subfolder=None, only_orphans=False, workspace=None)` | `limit`, `subfolder`, `only_orphans`, `workspace` | List generated output files, newest first. A name is `//`, where `` may sit in the subfolder the step chose (`final/episode.mp4`); each entry carries `folder` (the workflow) and `subfolder` (by convention `final` or `intermediate`, `''` when the step chose none, any path the workflow wrote otherwise), and `subfolder="final"` lists only deliverables. Each entry also carries a ready-made `url`, already scoped to the workspace that made it - a hand-built `/outputs/` URL 404s for anything but the default workspace. `only_orphans=True` inverts the call: instead of files, it returns run directories with no media anywhere under them (`runs`, each `{name, mtime}`) - a run whose output was deleted before `delete_output` could remove it by name, or one that failed before writing anything; `subfolder` does not apply in this mode, and `name` is exactly what `delete_output` accepts (#170). `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | | `get_gallery_metadata(name, envelope=False, workspace=None)` | `name`, `workspace` | Get the metadata embedded in a generated file — or, when `name` is an `asset:` reference, what an *input* asset holds (`source` says which; `job` is null for an asset). Reading an input's duration, frame count, fps and sample rate before a run is how a caller learns the `total_frames`, `fps` and `sample_rate` a workflow expects it to supply: the exact workflow and arguments that produced it, and, for audio/video, a `media` block (duration, rate, channels, fps, size, peak/mean dBFS). `envelope=true` adds `media.envelope` — `rms_dbfs` and `peak_dbfs` one entry per second — which is what locates something in a track rather than measuring the whole of it. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | ### Media @@ -249,12 +249,12 @@ The session starts in `default` and stays there unless it is told otherwise. | Tool | Arguments | Purpose | | --- | --- | --- | -| `validate_workflow(workflow=None, name=None, workspace=None, arguments=None)` | exactly one of `workflow` (inline definition) or `name` (a stored workflow, as `list_workflows` reports it), optional `workspace`, optional `arguments` | Check a workflow against the schema and against real pipeline signatures. Free and instant. Validating by name uses the workflow file's own directory as the base directory, so it sees what a run would. Returns every schema violation in `errors`, each with the JSON path it sits at, so a draft is fixed in one pass, and a `previous_result:` that names no earlier step is one of them. `warnings` covers what still runs but is probably wrong - a signature mismatch, and, for a list-driven variable, an entry key no step reads, at the entry's path. `workspace` names the workspace for this one call without switching the session to it - use it to pin a job whose `output:` or `asset:` references live in a workspace other than the session's. Pass the same `arguments` you will pass to `run_workflow` and they are checked too - an undeclared or renamed variable name, a value that will not coerce to the declared type, and an `asset:`, `prompt:` or `output:` reference that names nothing this workspace can reach, each reported at `arguments.`. `checked_arguments` lists what was covered, so a `valid: true` about the stored defaults cannot be mistaken for one about your values. A reference set the model would refuse - too many images, videos or audio clips, or, for MiniMax-H3, audio as the only reference - is an error here too, rather than a failure minutes into a run you acknowledged. `run_workflow` makes the same check and refuses a bad argument rather than queuing a job that fails on its first step. A valid answer carries `plan` - the fingerprint, step count, list lengths, `downloads_required` and `estimate` (with `basis`) for the arguments given; quote from it | +| `validate_workflow(workflow=None, name=None, workspace=None, arguments=None)` | exactly one of `workflow` (inline definition) or `name` (a stored workflow, as `list_workflows` reports it), optional `workspace`, optional `arguments` | Check a workflow against the schema and against real pipeline signatures. Free and instant. Validating by name uses the workflow file's own directory as the base directory, so it sees what a run would. Returns every schema violation in `errors`, each with the JSON path it sits at, so a draft is fixed in one pass, and a `previous_result:` that names no earlier step is one of them. `warnings` covers what still runs but is probably wrong - a signature mismatch, and, for a list-driven variable, an entry key no step reads, at the entry's path. `workspace` names the workspace for this one call without switching the session to it - use it to pin a job whose `output:` or `asset:` references live in a workspace other than the session's. Pass the same `arguments` you will pass to `run_workflow` and they are checked too - an undeclared or renamed variable name, a value that will not coerce to the declared type, and an `asset:`, `prompt:` or `output:` reference that names nothing this workspace can reach, each reported at `arguments.`. `checked_arguments` lists what was covered, so a `valid: true` about the stored defaults cannot be mistaken for one about your values. A reference set the model would refuse - too many images, videos or audio clips, or, for MiniMax-H3, audio as the only reference - is an error here too, rather than a failure minutes into a run you acknowledged. `run_workflow` makes the same check and refuses a bad argument rather than queuing a job that fails on its first step. A valid answer carries `plan` - the fingerprint, step count, list lengths, `downloads_required`, `estimate` (with `basis`) and `elided_steps` for the arguments given; quote from it. `steps` counts what will run: a step nothing reads and which saves no file does not run, and is named in `elided_steps` instead | | `list_workspaces()` | — | The server's workspaces and which one this session is using. Each has its own workflows, assets and outputs; the prompt library is shared by all of them | | `use_workspace(name)` | `name` | Work in that workspace for the rest of the session - every later call reads and writes there. This is how to keep your work out of another agent's namespace rather than sharing the default one. Checked against the server, so a typo fails here rather than scoping every later call to nothing | | `create_workspace(name, use=False)` | `name`, `use` | Create a workspace. Pass use=true to switch this session to it as well; otherwise the session stays where it was and the result says so | | `delete_workspace(name, acknowledged_cost=False)` | `name`, `acknowledged_cost` | Permanently delete a workspace and everything in it. Refuses without the acknowledgement, reporting what it would remove | -| `list_assets()` | — | The input media on the server, each with the `asset:` reference a workflow argument carries. Look here before asking for a file - what a workflow needs may already be there | +| `list_assets()` | — | The input media on the server, each with the `asset:` reference a workflow argument carries. Look here before asking for a file - what a workflow needs may already be there. `libraries` names the roots searched and which are writable; `shadowed` lists names a nearer library hides | | `keep_output(name, asset_name=None, overwrite=False, shared=False, workspace=None)` | `name`, optional `asset_name`, `overwrite`, `shared`, `workspace` | Keep a generated file as an input asset under a stable `asset:` name, so a later workflow can rely on it. The copy happens on the server: nothing is downloaded or re-uploaded. `asset_name` may name a folder and takes the kept file's extension when it has none; `shared=true` keeps it in the library every workspace shares, which is where a recurring cast belongs. `workspace` names the workspace for this one call without switching the session to it - the same pin `run_workflow` takes, so a job run into another workspace stays reachable from the session that queued it | | `upload_asset(file_path, asset_name=None, shared=False)` | `file_path` | Push a local image, video or audio file into the server's asset library and get back its `asset:` reference. The file is read from the machine the MCP server runs on, so this is how an input reaches a dw.serve running somewhere else. `asset_name` stores it under a readable name (`cast/priya-voice.wav`) instead of a random one; `shared=true` puts it in the library every workspace shares | | `delete_asset(name)` | `name` | Permanently remove one file from the asset library, by the name `list_assets` reports. Deletes from whichever library holds it - this workspace's own before the shared one; one from a read-only examples library is refused. Any workflow still carrying that `asset:` reference stops loading | diff --git a/docs/RECIPES_24GB.md b/docs/RECIPES_24GB.md index cdb4df1f..1318695b 100644 --- a/docs/RECIPES_24GB.md +++ b/docs/RECIPES_24GB.md @@ -146,9 +146,20 @@ Two things about the checkpoint are worth knowing before tuning anything: constructor argument, so diffusers logs "not expected ... will be ignored" and decodes with the convolutional VAE. Reaching it means `LTX2VideoDiffusionDecodePipeline` on a step run with `output_type: "{latent}"`, and two things get in the way. Its - first three stages run on the full volume by design - only stage 4 and the diffusion - blocks tile - and the attention mask they build is quadratic in the output grid: - 70GiB at 1536x896x121, which no tile size reduces. Base resolution fits comfortably. + neighborhood attention has two processors, and the one you get by default is the + portable FlexAttention fallback: it densifies a `seq_len x seq_len` block mask and then + runs uncompiled `flex_attention`, which falls to the eager reference path. Both + allocations are quadratic in the output grid, and neither is reduced by tiling (stages + 1-3 always run on the full volume) or by shrinking the clip (the stage-4 grid is near + output resolution either way). Measured on an otherwise empty 3090 (#153): 10.05GiB + inside stages 1-3 at 960x544x121, 69.77GiB for one stage-4 attention at 512x288x25 + (17.44GiB just to densify that stage's mask, whichever it reaches first), and ~25.5GiB + at 224x224x25, which is the smallest canvas its 7x7 kernel accepts at all. Nothing + fits - not base resolution, not the smallest clip the decoder will take. The path that does is NATTEN's `na3d` kernel, named per component as + `"attn_processor_type": "diffusers.models.autoencoders.ltx2_diffusion_decoder.LTX2VideoVaeNeighborhoodNattenProcessor"`, + which builds no mask at all - but it is fetched from the Hub by the `kernels` package + and needs a `shi-labs/natten` build matching the installed torch, which as of + torch 2.14 does not exist. And a step that returns latents returns *audio* latents too, which nothing outside a pipeline call can vocode - the two-stage template below feeds them back into one, which is the only way they become sound. Nothing here ships the diffusion decoder. diff --git a/docs/SERVER.md b/docs/SERVER.md index ec95c501..099d4187 100644 --- a/docs/SERVER.md +++ b/docs/SERVER.md @@ -177,7 +177,7 @@ Every event in the stream carries a `seq` and an `event` name: | `pipeline_step` | each denoise step | `step`, `total_steps`. Emitted for a pipeline that takes a `callback_on_step_end`, and for a `ModularPipeline` (H3, LTX-2, Qwen-Image), which takes none - there the denoise block's own progress bar is what reports | | `phase` | the step changes what it is doing | `phase`, `detail` | | `pipeline_released` | a step with `release_pipeline` drops its pipeline | `step`, `index`, `gpu_memory_allocated_mb` and `gpu_memory_allocated_before_mb` (both `null` where the backend cannot say). Emitted between the step's generation and its files being written, which is where the release happens - so the ordering is readable off the event stream rather than by trying to poll memory through a sub-second write | -| `warning` | a step finds something wrong with what it is about to write | `message`, plus a `kind` and the figures behind it (`level_spread`: `spread_db`, `measure`, `command`; `fps_mismatch`: `declared_fps`, `source_fps`). Also appended to the job's `warnings`, prefixed with the step it fired in - the event keeps the moment, `warnings` keeps it where a caller polling the finished job will look, since a warning about the artifact outlives the run that noticed it | +| `warning` | a step finds something wrong with what it is about to write | `message`, plus a `kind` and the figures behind it (`level_spread`: `spread_db`, `measure`, `command`; `fps_mismatch`: `declared_fps`, `source_fps`; `audio_no_headroom`: `file`, `peak_dbfs` - a deliverable at or above -0.5 dBFS, which an mp3 or AAC encode decodes over full scale; `step_elided`: `step`, `overridden_by` when a supplied argument is what made it unreferenced). Also appended to the job's `warnings`, prefixed with the step it fired in - the event keeps the moment, `warnings` keeps it where a caller polling the finished job will look, since a warning about the artifact outlives the run that noticed it | | `workflow_end` | the run finishes | `manifest` | A step spends most of its wall clock outside the denoise loop, and @@ -282,7 +282,14 @@ The editor's forms come from these; they are just as usable from scripts: MiniMax-H3, audio as the only reference - because the pipeline enforces those only once its checkpoint is loaded, minutes into an acknowledged run (`dw/reference_limits.py`, which reads each limit off the diffusers - block that enforces it rather than restating it). + block that enforces it rather than restating it). A LoRA loaded onto the + wrong checkpoint partition is an error for the opposite reason - the + pipeline accepts it: MiniMax-H3's `ref2va` holds `transformer_ref` alone, + so an FL2VA-trained adapter loads onto it, the run succeeds and only the + identity retention is worse (`dw/adapter_compatibility.py`, #155). A + `weight_name` carrying neither `ref2v` nor `fl2v` cannot be placed, so it + is a `warnings` entry naming the rule rather than a refusal - a + reference-trained checkpoint nobody has named yet still gets through. A valid answer also carries `plan`, what the run will execute for those arguments: `fingerprint` (`sha256:…` over the realized, expanded @@ -299,8 +306,12 @@ The editor's forms come from these; they are just as usable from scripts: `{repo, gb}` (`gb` from the hub, `null` when it could not be asked - `?sizes=false` skips the hub) and each `from_single_file` URL as `{repo: null, url, gb: null}`; and `estimate`, `{minutes, basis, - device, measured_on, partial}` from the workflow's own `cost` block - - `basis` is `catalog` (the stored total, for a run whose lists are the + device, measured_on, partial, runs}` from this box's own history when it + has one and otherwise from the workflow's `cost` block - + `basis` is `observed` (the cold median of this server's own finished runs + of this shape, with `runs` saying how many; preferred over a curated + figure, and quoted only for the bucket the caller's arguments fall in), + `catalog` (the stored total, for a run whose lists are the ones it was measured with), `per_entry` (re-priced from a measured per-entry rate, when the entry carries `per_entry`), `derived` (the stored total extrapolated linearly over a list whose length the caller @@ -308,7 +319,9 @@ The editor's forms come from these; they are just as usable from scripts: the serving backend; the first entry's figure, which is a warning rather than a quote) or `unknown` (no cost block, or more than one list changed so there is nothing honest to extrapolate along); a composed child's - cost is added and `partial` is true when a child has none. `plan` is `null` when + cost is added to a curated figure and `partial` is true when a child has + none - an `observed` figure already measured the whole run, children + included, so nothing is added to it and `partial` is false. `plan` is `null` when it could not be built; an invalid answer carries no `plan` key. ## Files and models @@ -333,7 +346,25 @@ The editor's forms come from these; they are just as usable from scripts: `cost_basis` says what that is - `curated`: figures a maintainer measured once and wrote into the workflow, never derived from this server's own job history, so `null` means nobody wrote one down rather than "this box has - never run it". A `models/` entry + never run it". Beside it, `observed` is the derived figure the same + listing is allowed to carry (#93): what *this* box's own finished runs of + that workflow took, as `cold_minutes`/`cold_runs` (model load included, + so comparable to a curated `cost`) and `warm_minutes`/`warm_runs` (model + already resident), with the `drivers` the figure is for, `since`, and + `unclassified_runs` when a run's persisted events were trimmed past its + `loading` phase. Runs are bucketed by the workflow's declared + `cost_drivers` - the variables that move its cost - so a 345-frame run + never informs a 124-frame figure; a list driver buckets on its length. A + workflow declaring no drivers falls back to runs that overrode nothing at + all, and a run whose every step was a step-cache hit is excluded. The + compact view carries only `observed_minutes` (cold) and `observed_runs`; + `GET /api/workflows/{name}/variables` carries the whole block beside the + defaults. Derived from the job rows in one query - so the figures outlive + a pruned run directory - and cached against the jobs table's high-water + mark rather than a file mtime, because a job landing changes every figure + and changes no file. `observed` never replaces `cost`: a maintainer's + claim on a named card and this machine's last week are different things. + A `models/` entry takes its `shape` and `traits` from the template it configures and keeps its own `cost`. A list-driven workflow (one with a `for_each` step) also carries `lists`: per list variable, the fields an entry takes, the steps @@ -404,7 +435,12 @@ The editor's forms come from these; they are just as usable from scripts: - `GET /api/assets` — the asset library: input media, each with the `asset:` reference a workflow carries rather than a path, since a path only means something on the server's own machine. Empty rather than an - error when no library is configured + error when no library is configured. `libraries` lists the roots searched, + in order, each `{origin, dir, writable}` — what `asset_dirs` names without + saying which of them an upload or delete can actually reach. `shadowed` + lists the entries a nearer library hides: same shape as an `assets` entry + but without `url` (that URL would serve the shadowing file, not this one), + plus `shadowed_by` naming the origin that won - `POST /api/assets/keep` (`{"name": ..., "asset_name": ..., "overwrite": false, "shared": false}`) — keep a generated file as an input asset under a stable name, returning its `asset:` reference. A run's files are named by the run that made them, @@ -424,6 +460,22 @@ The editor's forms come from these; they are just as usable from scripts: workspace's own before the shared one, the order `asset:` resolves in). An asset from a read-only examples library answers 403, the same as a read-only prompt or workflow; a name nothing holds answers 404 +- `POST /api/assets/archive` — `{"names": [...]}` (1-1000) bundles a + multi-file asset selection into one zip, named by each file's + library-relative path, which is the name its `asset:` reference carries. + The gallery archive's counterpart on the input side; it resolves down the + same search path a run does, so a selection spanning this workspace's + library, the shared one and an examples tree downloads as one archive, and + an unknown or out-of-library name 404s the whole request rather than + yielding a partial one. A duplicate name (repeated in the selection, or + differing only by leading/trailing whitespace) collapses onto the one zip + entry. Media (image/video/audio) stores rather than deflates, unless it's + a raw format that still compresses (`.bmp`, `.wav`) - everything else the + libraries hold is an already-compressed container, and the response does + not start until the archive is complete, so deflating it is latency the + caller waits through for nothing. Everything else - `.json`, `.md`, + `.txt`, an unrecognized extension - deflates; so does the export zip's + text files (`workflow.json`, `manifest.json`, `job.json`, the README) - `POST /api/uploads?filename=...` — the raw bytes of one image, video or audio file (200MB ceiling, checked from `Content-Length` before a byte is read, and again on the body; extension held to the allowed image/video list), saved diff --git a/docs/WORKFLOW_GUIDE.md b/docs/WORKFLOW_GUIDE.md index 0a11e852..f600a0c7 100644 --- a/docs/WORKFLOW_GUIDE.md +++ b/docs/WORKFLOW_GUIDE.md @@ -336,6 +336,55 @@ in braces keeps it a plain string — `"{nf4}"` is the string `nf4`. Getting thi wrong fails at load time, after validation has already passed, so a value that is meant as text under one of those keys must be braced. +### What a variable is allowed to be + +A model's own rule about a value belongs in the workflow, not in engine code +(CLAUDE.md) and not in a consumer's head. `variable_constraints` declares it +per variable, in the same field names a chain step's `frame_snap` uses: + +```json +"variable_constraints": { + "num_frames": { + "modulus": 17, + "remainder": 5, + "min_frames": 124, + "max_frames": 345, + "snap": "up", + "reason": "the video VAE encodes 17 * n + 5 frames, and MiniMax-H3 generates between 5 and 15 seconds at 24 fps" + } +} +``` + +The value has to be `modulus * n + remainder` within `min_frames` to +`max_frames`. With `snap: "up"` an off-grid value is rounded to the next one +the rule accepts and the run *says so* - `130` becomes `141`, reported as a +warning at validation time and again in the job's `warnings`; without `snap` +it is refused. The bounds are checked against the value the run will use, so +they hold for the rounded number: on the rule above `108` is accepted (it +becomes `124`) and `346` is refused (it would become `362`). + +Checked three times, for the reasons the task-argument domains are: in +`validation_errors`, so `POST /api/validate`, `validate_workflow` and the +pre-queue check all refuse a bad value at `arguments.` or +`variables.` for free; at run time before anything loads, which is the +backstop for a value the static pass cannot see (an inline workflow, a value +a parent passed down); and in the catalog, where `list_workflows` and +`get_workflow(variables_only=true)` report the rule beside the default - the +half that stops the next caller picking a number the model refuses. + +State the rule once. Where a template both declares a constraint and snaps a +chain, the chain's `frame_snap` names it rather than repeating the numbers: + +```json +"frame_snap": "constraint:num_frames" +``` + +Two limits, both accepted. A constraint cannot express a bound that depends +on another variable (a maximum that is `fps * seconds` where a template +exposes `fps`), and it reaches a top-level variable only - not a field inside +a list entry, so a `for_each` template whose entries each carry their own +`num_frames` is unconstrained and relies on the run-time check. + ### A workflow takes only the keys the engine reads The workflow object itself, `step`, `task`, `workflow`, @@ -484,8 +533,10 @@ with your `arguments` answers with a `plan` whose `estimate` already does that arithmetic (`basis: per_entry`); without `per_entry` it extrapolates the stored total linearly over your list (`basis: derived` - an estimate rather than a measurement) and reports the stored total unchanged only -when your list is the one it was measured with (`basis: catalog`) - quote -the plan's figure and say which basis it has. An +when your list is the one it was measured with (`basis: catalog`). Ahead of +all of those it quotes this box's own finished runs of the shape you are +about to run when it has any (`basis: observed`, with `runs` saying how +many) - quote the plan's figure and say which basis it has. An entry key no step reads is a validation warning at the entry's path, so a misspelt field is caught before the run. Then `validate_workflow` with the @@ -616,6 +667,20 @@ than prefixing it, so `"file_base_name": "episode"` in a `final` subfolder writes `final/episode-0.0.mp4` - name each step that sets one differently, or the second collides and picks up a `-2`. +A step that saves nothing and which no later step reads does not run at +all: the engine drops it before the first step executes and warns once per +dropped step. That is how a template whose portraits can be supplied as +`asset:` files stops paying for the steps that would have drawn them. It +follows from what the definition says, never from a value produced during +the run, so it is decided at validate time too - the `plan` a validate call +answers with counts only the steps that will run and lists the rest under +`elided_steps`. Four things keep a step: a `result` with a `content_type` +and `save` not `false`, being the last step, being read by a later step +(`previous_result:`, `gather:`, a `pipeline_reference`, a shared component), +or being read by a step that is itself kept - elision is transitive. If a +step you meant to run is named in the warnings, a reference to it is +misspelled somewhere later or it needs a `result`. + ### Composing a stored workflow A step with a `workflow` block runs another workflow as one step of this one, @@ -656,6 +721,19 @@ gets it wrong; a declaration that merely repeats the derivation is noise that rots when the rules change, and the repo's catalog tests refuse it. `cost` is never derived — leave it absent until a run has been measured. +`cost_drivers` is the other half of saying what a workflow costs, and it *is* +for derivation: the variables that move the wall clock — a frame count, a +step count, a segment count, the list a `for_each` runs over — never a prompt +or a seed. The server buckets its own finished runs by those values and +reports the result as `observed` beside the curated `cost`, so a 345-frame +run never informs a 124-frame figure and a list driver buckets on its length. +Declaring none is not neutral: the figure then falls back to runs that +overrode nothing at all, which most real runs do, so a measured workflow with +no drivers keeps answering "unknown". Each name must be a variable the +workflow declares — `tests/test_observed_cost.py` sweeps the catalog for one +that is not, since a driver bucketing on nothing looks exactly like a driver +that works. + ## Result Configuration ```json @@ -784,6 +862,11 @@ For components the pipeline loads itself — which is all of a modular pipeline' `diffusion_decoder`, for example). - `attention_backend` — a persistent `set_attention_backend` on one component, which a compiled component needs (the pipeline-level `attention_backend` applies per call). +- `attn_processor_type` — the attention processor the component runs, constructed with no + arguments and handed to `set_attn_processor`. The `unet` and `transformer` blocks cover + those two; this covers any other component that carries attention (LTX-2.5's + `diffusion_decoder`, whose default processor is a portable fallback rather than the + NATTEN path the decoder was built around). - `compile`, `truncate_layers`, `remove_modules` — see [ACCELERATION.md](ACCELERATION.md). - A dotted key reaches a module inside a component, for a component that holds the model @@ -912,6 +995,37 @@ later step needing one of them reloads it. **Example:** [enhance-prompt.json](../workflows/templates/minimax/enhance-prompt.json) +#### A step nothing reads does not run + +Before the first step executes, the engine drops any step whose result no later step +reads and which writes no file, and warns once per dropped step saying which and why. +`dialogue-short` cast from portraits that already exist used to run its two Z-Image +steps anyway and throw the pictures away - about a minute of GPU per episode on +something nothing looked at (#122). + +Four things keep a step: + +- **it saves** - a `result` with a `content_type`, and `save` not `false`. A workflow + whose whole point is writing three images references nothing, so this is the rule that + keeps elision from being destructive. `"save": false` is how a step says it is + scaffolding. +- **it is the last step** - it is the run's answer, whatever it declares. +- **something reads it** - `previous_result:`/`from_previous_result` (including + `previous_result:step.property`), a `gather:` (which is a list of those by the time + this runs), a `pipeline_reference` naming it, or a `reused_components` entry naming a + component it shares. +- Elision is transitive, so dropping a step can drop the step it read in turn. + +`release_pipeline` on an elided step moves onto the last surviving step before it when +that step loaded the same pipeline, and `release_models` moves unconditionally - a +release that vanished with its step would leak the memory it existed to free. The plan a +validate call answers with is computed after elision, so `steps`, `downloads_required` +and the cost it quotes are the work that will actually happen, and it lists what was +dropped under `elided_steps`; the run manifest records the same list. + +If a step you expected to run is named in the warnings, the usual cause is a reference +to it spelled wrong somewhere later, or a step that was meant to declare a `result`. + ### VAE Options ```json @@ -1226,7 +1340,9 @@ joined into a single file: - `frame_snap` — the constraint the pipeline puts on `num_frames`, used to snap the final `match_audio` segment to a valid length. MiniMax H3 accepts `17n+5` frames between 124 and 345: `{ "modulus": 17, "remainder": 5, "min_frames": 124, - "max_frames": 345 }`. + "max_frames": 345 }`. Where the workflow already declares that rule as a + `variable_constraints` entry, write `"frame_snap": "constraint:num_frames"` + instead, so the numbers live in one place (*What a variable is allowed to be*). - `prompts` — optional per-segment prompt list for narrative progression; segment `i` uses `prompts[min(i, len - 1)]`. - `save_segments` — write each completed segment to the output directory as a @@ -1677,6 +1793,16 @@ rate of their own. A mono track needs no preparation: an mp4 audio stream takes stereo and nothing else, so saving duplicates the single channel into two and emits a warning saying it did. +The track and the frames are two lengths a workflow used to have to keep equal by +hand. `"fit": "video"` derives one from the other instead: the track is cut to +exactly the frames it is laid over, or padded with silence and warned about when it +is shorter than they are. That is what a soundtrack over a cut whose length is an +argument needs - nothing in a workflow can multiply a list's length by a frame +count, so `music-video.json` sliced a fixed 496 frames of song while its cut +followed a `shots` list, and a two-shot run wrote 10.3 s of picture into a 20.7 s +container and reported `succeeded` with no warnings (#142). Left unset the track is +used as it is and a disagreement is warned about rather than passing in silence. + Which shape a pipeline argument wants is the pipeline's business, and the two LTX-2 paths differ: a keyframe condition is mapped from 0-255, so it takes the `video_frames` array, while an IC-LoRA reference goes through the video processor, which expects the diff --git a/docs/proposals/h3-video-mux-headroom-warning.md b/docs/proposals/h3-video-mux-headroom-warning.md new file mode 100644 index 00000000..cf04a4be --- /dev/null +++ b/docs/proposals/h3-video-mux-headroom-warning.md @@ -0,0 +1,207 @@ +# H3 video templates warn `audio_no_headroom` on their own defaults (#174) + +Written by model `sonnet` via provider `anthropic`. + +## The report + +M-F008 (`templates/minimax/video-with-audio-768p`, stock defaults) asserts +`warnings: []` and got one: `audio_no_headroom` at `peak_dbfs: -0.14`, from +H3's own generated soundtrack, muxed with no `normalize_audio` step anywhere +in the template. `get_gallery_metadata` on the finished file reads +`peak_dbfs: -1.1158` — the AAC mux moved the level *down* 1.0 dB from the +source, not up. That is the third run in a row where this template family's +AAC mux overshoot went negative on this box (`M-F012.jsonl`: -0.92 dB and +-1.11 dB under target), against the +1.94 dB *over* that #161 measured +(on `music-video`'s mux, a different song, a different day). #174 asks +which of two fixes is right; this doc is that decision, plus one thing +neither #174 nor #159/#161 had: the current code already runs the +ground-truth check that would have caught this, and throws its answer away. + +## Root cause, part one: scope + +#159 gave `normalize_audio` to `templates/minimax/music` and `music-video` +only, because those were the templates the -0.94 dBFS-over-full-scale case +(#158) was filed against. Every H3 *video* template — `video-with-audio`, +`video-with-audio-768p`, `dialogue-short`, `storyboard`, +`chained-segments`, `chain-matched-and-aligned`, `chain-matched-to-audio`, +`chain-video-continuity`, `composable-references`, `first-and-last-frame`, +`generated-subject-reference`, `image-to-video`, `last-frame-only`, +`reference-to-video`, `voice-timbre-reference` — generates its own +soundtrack (`t2va`/`fl2va`/`ref2va`) and mixes it straight into the mux with +no gain stage. `#159` never claimed to cover them; it just happened to +close the two tickets that existed. All 15 templates carry the same +exposure #174 found in one of them. + +## Root cause, part two: the warning suppresses its own correction + +`dw/result.py`'s two checks are meant to compose: `warn_without_headroom` +(dw/result.py:77) reads the pre-encode waveform and predicts a clip; +`warn_if_written_above_full_scale` (dw/result.py:113) reads the encoded +file back and reports the truth, `already_warned=True` (the pre-encode +warning fired) suppressing the post-encode check outright +(dw/result.py:122-126, dw/result.py:132-133) — +because when #161 wrote it, the only encoder overshoot on record was +positive (mp3 +0.1 dB, AAC +1.9 dB): if the source was already over the +line, the encoded file surely would be too, so probing it again looked +redundant. Both call sites already run the same way for a video mux +(dw/result.py:762, dw/result.py:800) and for audio-only saves +(dw/result.py:641), and the post-encode call always runs, unconditionally, +right after the save (dw/result.py:677-682) — it just returns immediately +when told to. + +M-F008 breaks that assumption: the AAC mux moved this soundtrack *down* a +full dB. The post-encode probe would have said so — it runs the same +`probe_media` call `get_gallery_metadata` used to read -1.1158 — but never +got the chance, because `already_warned` was `True` before it started. +The consumer got the wrong answer even though the code to get the right one +executes on every single run. + +## Two fixes, not either/or + +#174 poses these as alternatives. They're not — the second is a bug fix +underneath the first regardless of which way the loudness question goes. + +**1. Stop suppressing the ground-truth check for a mux.** Drop +`already_warned` from the `warn_if_written_above_full_scale` call at +dw/result.py:680 for `content_type.startswith("video")` (keep the +suppression for a plain audio save, where #159's mp3 measurement — +0.1 dB, +consistently positive — still supports "the source being over the line +means the file will be too"). A video mux now always reports what it +actually wrote. If `audio_no_headroom` fired and the file came back clean, +nothing further is said — `warn_if_written_above_full_scale` only speaks +when the decoded peak is itself at or above 0 dBFS (dw/result.py:142). If +the mux is over, the caller gets `audio_clipped` with the real number +instead of (or, currently, never, since it can't fire when +`audio_no_headroom` already claimed the slot) a stale prediction. This +closes the immediate defect on its own: a clean mux stops being reported as +suspect, on any template, without adding a gain stage anywhere. + +**2. Whether to also add `normalize_audio` to the H3 video templates** is +the part that's genuinely a product call, and where I think the case for +"yes" is weaker than #159's was. #159 added the step because a measured +run had *already* clipped end-to-end (#158's +0.94 dBFS case). Nothing here +has: every H3 video-template measurement on record (M-F008, and the two +`M-F012.jsonl` entries) landed *under* full scale after the mux. Baking a +fixed `peak_dbfs: -3` gain reduction into fifteen templates to guard +against a failure mode that fix (1) already reports accurately, and that +hasn't reproduced once on this family, trades a small amount of loudness on +every default run for a warning that (post fix 1) no longer misfires. I'd +hold off unless a future run actually shows an H3 video mux clipping — at +which point fix (1) is exactly the mechanism that will catch it and say so +correctly, with a real number to size the gain from instead of a guess. + +## What I'm asking for + +Approval to ship fix (1) (the suppression bug) now — it's a pure +correctness fix to a warning that currently lies when the encoder happens +to undershoot instead of overshoot, no template changes, no gain applied to +any deliverable. Fix (2) (gain stage on the fifteen H3 video templates) I'd +rather leave undone until there's a clipped-in-practice case to size it +against, the way #158 gave #159 one; happy to be overruled if the +preference is defense-in-depth over waiting for evidence. + +## M-F008's `warnings: []` assertion + +Once fix (1) ships, M-F008 as currently worded should pass on a fresh run +of `video-with-audio-768p`'s stock defaults (H3's soundtrack is +under-scale after the mux, same as the other two measurements), without +touching the case text — #173 (parked, owner:don) is where any wording +change for the case itself belongs; this issue only concerns the engine +behavior. + +## Regression case proposed (for the tester to add once verified) + +- Suite: `regression-suite-model-specific.md` (H3-specific mux behavior). +- Call: `run_workflow("templates/minimax/video-with-audio-768p")`, stock + defaults. +- Expected once fix (1) ships: `status: "succeeded"`, and no + `audio_no_headroom` warning unless `get_gallery_metadata`'s `peak_dbfs` + on the written file is itself at or above 0 dBFS (in which case + `audio_clipped` should appear instead, with a `peak_dbfs` matching what + `get_gallery_metadata` reports for the same file). + +## Amendment (2026-09-16): fix (1) as approved and shipped is insufficient + +Written by model `sonnet` via provider `anthropic`. + +Fix (1) was approved and shipped as `ce82f06` (merged `aab4ef5`, deployed to +`lem`): `warn_if_written_above_full_scale`'s `already_warned` suppression is +now passed as the real prior state only for `content_type.startswith("audio")`; +for `"video"` it is always `False`, so the post-encode ground-truth probe +always runs and always speaks for a video mux. + +That is correct as far as it goes, but the tester's verification run against +`lem` (job `46f4d0f2faf1`, run `20260916-092932-7e47a764`, +`video-with-audio-768p` stock defaults) shows it doesn't reach the acceptance +criterion: + +- Written file measured `peak_dbfs: -1.1158844…` via `get_gallery_metadata` — + well under 0 dBFS, a clean mux. +- `job.warnings` still contains `audio_no_headroom` at `peak_dbfs: -0.14`. +- `get_job_events` shows why: that warning is emitted at seq 32, `at: 571.7`, + *before* the write (seq 33, `at: 574.1`). It is the **pre-encode** + `warn_without_headroom` check (dw/result.py:77), which fix (1) never + touched. No warning event of any kind follows the write in this run — the + post-encode probe ran (per the code path) but had nothing to report, + because the file came back clean, exactly as fix (1) predicts. It just + doesn't cancel or replace the pre-encode warning that already fired. + +The approval's acceptance criterion was "no `audio_no_headroom` … unless the +written file is ≥0 dBFS, in which case `audio_clipped` should appear +**instead**." What ships today instead produces *both* checks running +independently: the pre-encode one always fires on this family's stock +defaults (H3's own soundtrack sits close enough to the line that +`warn_without_headroom`'s threshold catches it before any encoding happens), +and the post-encode one only *adds* a second warning when the mux is +genuinely bad — it never suppresses the first one when the mux turns out +fine. So the stock M-F008 run still asserts `warnings: []` and still fails +that assertion, unchanged from the original report. + +### The undershoot record now has four points, all negative + +| Run | Template / mux | Predicted (pre-encode) | Written (post-encode, `get_gallery_metadata`) | Delta | +|---|---|---|---|---| +| #161's `music-video` case | AAC, different song | — | — | **+1.94 dB** (the one positive measurement on record, a different template family) | +| `M-F012.jsonl` #1 | H3 video mux | target | measured | **-0.92 dB** | +| `M-F012.jsonl` #2 | H3 video mux | target | measured | **-1.11 dB** | +| M-F008 original report | `video-with-audio-768p` | -0.14 dBFS | -1.1158 dBFS | **-0.98 dB** | +| Tester's 2026-09-16 re-verify | `video-with-audio-768p` | -0.14 dBFS | -1.1158844… dBFS | **-0.97 dB** | + +Four consecutive H3-family AAC muxes, on two different templates and two +different sessions, all undershoot by roughly 1 dB. The one overshoot on +record is a different template (`music-video`) on a different day. This +doesn't prove H3's mux never overshoots — it's still a small sample — but it +directly contradicts treating "encoder overshoot is always positive" as safe +to assume for this family, which is the flag Don raised in the approval +comment for the audio-only suppression. The same caution now applies with +actual evidence behind it on the video side. + +### What changing would require + +To satisfy the approved criterion literally — `audio_no_headroom` never +reaching the caller for a video mux that measures clean — the pre-encode +`warn_without_headroom` result would have to be held back rather than +emitted immediately, and either dropped or upgraded to `audio_clipped` +once the post-encode probe runs ~2.4 s later. That's a real behavior change +beyond fix (1) as approved: it changes *when* a warning reaches the caller +(after the write completes, not at the point the risk is detected) and +*which* checks a video mux can ever surface (post-encode ground truth only, +never the prediction) — for every content type that already goes through +`warn_without_headroom`, not just H3's video family, since the suppression +logic is generic to `dw/result.py` and not H3-specific. + +I'm not implementing this. Per the triage note that queued this issue, the +approval rested on the proposal's premise that fix (1) alone would clear +M-F008 on its own — the 2026-09-16 verification run disproves that premise, +so this needs a fresh decision rather than a unilateral extension of what +was already approved. + +### Asking + +Approve or decline holding the pre-encode `audio_no_headroom` warning for a +video mux until the post-encode probe has run, replacing it with +`audio_clipped` (measured value) when the file is genuinely over, and +emitting nothing when it's clean — scoped to `content_type.startswith("video")` +only, audio-only saves unaffected. Fix (2) (a `normalize_audio` gain step on +the fifteen H3 video templates) remains held, unchanged from the original +decision — no run on record has an H3 video mux actually clipping. diff --git a/docs/proposals/list-entry-constraints.md b/docs/proposals/list-entry-constraints.md new file mode 100644 index 00000000..5eff7964 --- /dev/null +++ b/docs/proposals/list-entry-constraints.md @@ -0,0 +1,150 @@ +# A declared bound that reaches a list entry + +Status: proposed, 2026-09-14. Raised by #145, which revisits limit 2 of +`preflight-argument-bounds.md` (accepted, dkackman, 2026-09-14). Written by +the implementer agent, model `opus` via provider `anthropic`. + +## The problem + +`variable_constraints` (#96) refuses `num_frames: 61` on +`templates/minimax/music-video`, where the frame count is a top-level +variable. The same 61 is accepted, quoted at 8.4 minutes and queued on +`templates/minimax/dialogue-short`, where the frame count is a field of a +`shots` entry: + +``` +validate_workflow("templates/minimax/dialogue-short", + arguments={"shots": [{"name": "probe", "num_frames": 61, ...}]}) +-> {"valid": true, "errors": [], "warnings": [], + "plan": {"estimate": {"minutes": 8.4, "basis": "derived"}}} +``` + +61 is the value `validate_workflow`'s own tool description cites as the thing +#96 stopped happening ("61 used to validate and then fail 138 s into the run, +after the weights were loaded"). It still does - it just has to be spelled as +a list entry. Three consequences, all in the ticket: + +1. The refusal is missing. The run loads H3, enters the text encoder and + fails on the pipeline's own check, which is the two minutes of GPU the + feature exists to not spend. +2. The **snap warning** is missing. `shots[1].num_frames: 130` validates + `warnings: []` and then generates 141 frames. As a top-level variable + that comes back as a notice naming 141. +3. The **rule is unreadable**. `get_workflow(..., variables_only=true)` and + the compact `list_workflows` entry report no `constraints` block for + `dialogue-short` at all, while `lists` tells the caller that a `shots` + entry carries `num_frames`. So the discovery surface names the field and + says nothing about what it may hold - the exact gap #96 named as the half + that stops the next consumer picking 61. + +This was accepted knowingly: limit 2 records that `dialogue-short` "cannot +declare the rule per entry, and relies on the run-time check as before." +What the limit did not weigh is that `dialogue-short` is not a corner - it is +the only H3 template a caller composes a multi-shot deliverable with, the one +place per-shot length is *meant* to vary, and therefore the place a frame +count is most likely to be typed by hand. Today it is also the only +list-driven workflow in the catalog whose entries carry a constrained field +(`music-video`'s entries carry `prompt` and `start_frame` only), so the whole +exposure is one field of one template - and so is the whole cost of closing it. + +## Why this is a decision and not an edit + +Whatever closes this changes what a `variable_constraints` key *means*, which +is the part a consumer reads and a template author writes. It also reopens a +limit you accepted yesterday. Picking the shape is yours; the mechanics are +small either way. + +## Option A - the key stays a name, matched wherever that name is declared (recommended) + +A constraint is keyed by a plain variable name today. Keep that, and match the +name in both places a value by that name can sit: a top-level variable, and a +key of the same name in an entry of a list-valued variable. + +```json +"variable_constraints": { + "num_frames": {"modulus": 17, "remainder": 5, "min_frames": 124, + "max_frames": 345, "snap": "up", "reason": "..."} +} +``` + +`dialogue-short` declares exactly that block - the same six lines +`music-video` already carries, no new syntax at all - and its per-entry +counts are checked. + +Mechanics, all in `dw/variable_constraints.py` plus reporting: + +- `constraint_errors` / `constraint_warnings` additionally walk each + list-valued variable whose entries are objects, and check any entry key + that matches a declared constraint name. The path is where the caller + wrote it: `arguments.shots[0].num_frames`, or + `variables.shots[0].num_frames` for a stored default. Every existing + caller - `validation_errors`, `POST /api/validate`, the pre-queue check in + `POST /api/jobs`, `validate_workflow` - gets it with no change. +- `apply_constraints` already runs on the run's `variables` dict *before* + substitution (`_prepare_definition`), so a `shots` list is sitting right + there: it rounds an entry field in place and `emit_warning`s the same + notice, and raises for one no rule can reach. The rounded value then + propagates through `item:` like any other. +- The catalog reports it twice: the existing `constraints` block (which is + keyed by name, so it needs nothing) plus the field's entry in `lists`, so + a caller reading what a `shots` entry carries reads the rule beside the + field name rather than having to cross-reference. + +What it costs: a name means one rule for the whole workflow. A template that +wanted `num_frames` bounded one way as a variable and another way inside an +entry cannot say so. No template wants that - a bound is a property of the +model the value is handed to, and both spellings are handed to the same +pipeline - and a template that did want it should use two names. + +The honest risk: an entry field that happens to share a constrained name but +feeds something else would be checked against a rule that does not apply to +it. In a file the same author writes end to end, with the only live case +being `num_frames -> num_frames`, that is a naming mistake the constraint +would usefully surface rather than a trap. `tests/test_variable_constraints.py` +already sweeps the catalog; the sweep would gain "every entry field a +constraint matches feeds the pipeline argument that constraint is about." + +## Option B - a path-shaped key + +```json +"variable_constraints": {"shots[].num_frames": {...}} +``` + +Explicit, and it answers the collision A accepts. It is also the second +dialect you ruled out: a key that is sometimes a name and sometimes a path, +a `constraints` block a consumer can no longer index by variable name, and a +grammar (`[]`, nesting, how deep) to define and validate. For one field of one +template. + +## Option C - report the limit instead of closing it + +Leave enforcement alone and make the discovery surface say the bound does not +reach entry fields - e.g. `dialogue-short` declares the constraint for +documentation and the catalog marks it advisory. This makes the rule readable +(consequence 3) and leaves 1 and 2 exactly as they are: a documented bound +nothing checks is the drift `preflight-argument-bounds.md` rejected Option C +for. Worth naming only to reject. + +## Option D - hoist `num_frames` to a top-level variable + +`dialogue-short` drops the per-entry field and bounds one workflow-wide count. +No engine change at all. It also deletes the feature the template is written +around - the tag entry runs 141 frames where the others run 124, because +length is per-shot in a cut sequence - so the bound would be enforced by +removing the thing being bounded. + +## Recommendation + +A. It declares nothing new, it is one block added to one template, and it +makes the constraint follow the value rather than the spelling - which is what +a caller reading `lists` already assumes. If the collision risk reads worse to +you than the dialect does, B does the same job for more grammar. + +## Not in scope + +- A constraint on a field of a *nested* structure that is not a list entry + (`references[].something`). Nothing in the catalog has one. +- Limits 1 and 3 of `preflight-argument-bounds.md` (a constraint that depends + on another variable; the run-time check as backstop) stand unchanged. + +*Implementer agent, model `opus` via provider `anthropic`.* diff --git a/docs/proposals/orphaned-run-directories.md b/docs/proposals/orphaned-run-directories.md new file mode 100644 index 00000000..3c6dbbc7 --- /dev/null +++ b/docs/proposals/orphaned-run-directories.md @@ -0,0 +1,145 @@ +# Design: making media-less run directories visible + +Status: **proposal**. Written for #170 (filed by the regression agent, model +`opus` via provider `anthropic`), triaged and drafted by the implementer +agent, model `sonnet` via provider `anthropic`, 2026-09-15. + +## The ask, as filed + +`list_workspaces().usage` reports files/bytes a consumer cannot otherwise +enumerate: `list_gallery`, `list_assets` and `list_workflows` all report the +workspace empty while `usage` counts 44 files / 36,276 bytes in it. #170's own +investigation (repeated here, verified against the code) traces this to run +directories that hold a `manifest.json` and `workflow.json` but no media - a +run whose output was deleted before #134 shipped the "delete by run directory +name" form of `delete_output`, or a run that failed before writing anything +and was never swept. There is no MCP call that names one of these directories, +so there is also none that can delete it. #170 is explicit that #134 itself is +not at fault: per-job residue measured at zero across four fresh jobs in the +same session, including a failed one cleaned up by its `/` +name. + +## The cheap half, checked first + +The triage comment on #170 asked whether `workspace_usage` counting +`manifest.json`/`workflow.json` at all is intended, before treating the gap as +a design problem. It is. `workspace_usage` (`dw/workspace.py`) walks every +file under a workspace's own folders with `_tree_usage` - no extension filter, +no media check - because its stated purpose is "how much disk a workspace +occupies," the number a client offers alongside "delete this workspace." The +gallery, by contrast, deliberately filters to `MEDIA_KINDS` +(`_iter_gallery_files`, `dw/server/app.py`) because it exists to show a +consumer their generated files, not their bookkeeping. Two different +questions, two different counts, both working as designed. `usage` is not the +bug; the bug is that nothing else can account for the gap it reveals. + +## Why this needs a decision and not an edit + +Every existing way to inspect a workspace's outputs - `list_gallery`, +`GET /api/gallery`, the job page's manifest - is keyed to *media files*: a +`manifest.json`/`workflow.json`-only directory has none, by construction +(`_iter_gallery_files` only yields names whose extension is in +`MEDIA_KINDS`). Making an orphan visible means adding a concept a consumer +has to learn - "a run directory can exist with nothing to show you" - on a +surface that has never needed it before. That is squarely the kind of change +guardrail 7 asks to be proposed rather than edited in: new MCP surface, +however small. + +There is also a real design question underneath it, not just a listing +gap: **should an orphan be swept automatically, surfaced for a human/agent +decision, or both?** A media-less run directory is not always junk - a run +that failed mid-step and left partial intermediate files with no +`content_type` result would look identical to one that failed before writing +anything, and a workflow that legitimately writes no media (a pure `utility` +shape - e.g. a text-analysis step whose result is JSON, not a saved file) is +indistinguishable from an orphan by this test alone. Deciding that is worth +getting right once rather than shipping a listing tool whose shape has to +change later. + +## Options + +### A. `list_gallery` gains an "orphans" mode + +Add a boolean, e.g. `list_gallery(only_orphans=True)`, that walks the output +tree the same way `_iter_gallery_files` does but inverts the filter: instead +of yielding media files, it yields run directories (identity + run id, via +`split_run_path`/`is_run_id`) that hold `manifest.json` or `workflow.json` +and **no** file whose extension is in `MEDIA_KINDS` anywhere under them. Each +entry would carry `name` (the `/` `delete_output` already +accepts), `mtime` (from the manifest, for sorting/staleness), and `reason` +(`"no-media"` vs `"failed"`, read off the manifest's own status field if it +has one - worth checking before committing to the field name). This reuses +the existing filter/pagination shape consumers already know, and pairs +directly with the `delete_output` form #134 added - no new tool, one new +parameter, one documented reading. + +Cost: `list_gallery`'s docstring and the tool description both grow a +second meaning ("files, or absence of files"), and the REST mirror +(`GET /api/gallery?only_orphans=true`) needs the same walk done a second way +(gallery's normal walk is `os.walk` + extension filter; this one is +`os.walk` + *directory* filter) - not free, but contained to one function +`_iter_orphan_runs` beside `_iter_gallery_files`. + +### B. A dedicated tool, e.g. `list_orphaned_runs` + +Same underlying walk, its own MCP tool and REST route +(`GET /api/gallery/orphans` or `GET /api/outputs/orphans`) rather than a mode +of an existing one. Clearer name, no double meaning on `list_gallery`, but +one more tool for a consumer to discover and one more entry in every list of +"the gallery-adjacent tools" (`get_guide`, the plugin skills, `dw/CLAUDE.md` +if it ever enumerates them). Given how rare this call is expected to be - +once per regression sweep, essentially never for a normal workflow session - +the discoverability cost of a whole tool seems disproportionate next to +option A's one parameter. + +### C. Sweep automatically, no listing at all + +Fold orphan cleanup into something that already runs unattended: a +`forget_workspace_usage`-adjacent pass at server startup, or opportunistically +whenever a job finishes in a workspace (walk that workspace's outputs, +`shutil.rmtree` any run directory with no media, same "climb and rmdir while +empty" logic `delete_output`/`_sweep_run_directory` already have). This closes +the backlog with no new MCP surface at all, but removes the option to inspect +before deleting - the same objection the `wontfix` reasoning below would raise +if a consumer's own retained-but-empty run (a deliberate `utility`-shape +workflow, see above) got swept as if it were junk. Also weakens the +regression sweep's own assertion further: "the workspace is empty" would +become true by a mechanism the sweep did not invoke and cannot verify was +*this run's* doing. + +## Recommendation + +**A**, with the "no-media" test scoped to a run directory holding *only* +`manifest.json`/`workflow.json` (or either alone) and nothing else - not +"no `MEDIA_KINDS` file," which would misclassify a legitimate no-media +utility run as an orphan. That distinction needs one query against the +manifest: does this step list contain any `result.content_type`. +Concretely: + +- `list_gallery(only_orphans=False)` (default unchanged), and + `list_gallery(only_orphans=True)` returns run directories matching the + refined test above, each `{name, mtime}` (`name` is exactly what + `delete_output` already accepts - the pairing is the point). No new + `delete_output` shape is needed; #134 already covers removal once a + consumer has the name. +- `GET /api/gallery?only_orphans=true` mirrors it. +- No automatic sweep (rejects C) - orphan cleanup stays an explicit, + attributable action, consistent with how `delete_output` already works for + everything else in a workspace. +- The regression agent's own sweep step gains a call it did not have before: + `list_gallery(only_orphans=True)` then `delete_output` per name, closing + the loop #170 opened. + +## Open questions for Don + +1. Does the "no `content_type` anywhere in the manifest" test correctly + distinguish an orphan from a legitimate no-media run, or is there a + workflow shape it still misclassifies? +2. Is `only_orphans` the right name, or should this be a `subfolder`-style + third value (`list_gallery(mode="orphans")`) to leave room for another + listing mode later without stacking booleans? +3. Should the backlog already on `lem` (44/48/72/12/4 files across the + workspaces #170 measured) be cleared by hand once this ships, or left for + whoever next runs the regression sweep against that workspace to clear via + the new call - i.e. is a manual `lem` cleanup in scope for this ticket or + a separate housekeeping pass? diff --git a/docs/proposals/preflight-argument-bounds.md b/docs/proposals/preflight-argument-bounds.md index e3b6d839..f6f9fd13 100644 --- a/docs/proposals/preflight-argument-bounds.md +++ b/docs/proposals/preflight-argument-bounds.md @@ -129,3 +129,50 @@ Checking a value against a *loaded* pipeline's signature - that is what the existing signature warnings do. This is only about bounds that are constants of the model, which is the class of error that costs two minutes of GPU before it is reported. + +## Accepted limits (recorded at implementation) + +Approved as Option A with C's reporting folded in (dkackman, 2026-09-14) and +implemented on that basis. Three limits were accepted rather than designed +around: + +1. **A constraint cannot depend on another variable.** A bound that is + `fps * seconds` where a template exposes `fps` has no expression here. + H3's fps is fixed and LTX-2.5's templates carry `frame_rate` as a + variable but bound only the VAE grid, which is rate-independent, so + nothing in the catalog needs it today. +2. ~~**A constraint reaches a top-level variable only.**~~ **Superseded by + #145** (`docs/proposals/list-entry-constraints.md`, Option A, approved + 2026-09-14). The limit as accepted read: *a `for_each` template whose + entries each carry their own `num_frames` - + `templates/minimax/dialogue-short` is the live case - cannot declare the + rule per entry, and relies on the run-time check as before. Reaching + inside a list entry would need a path-shaped constraint key, which is a + second dialect and was ruled out.* + + What the original decision did not weigh is that `dialogue-short` is the + one H3 template where per-shot length is *meant* to vary, so it is where + a frame count is most likely typed by hand and simultaneously the only + place the rule was unreadable. The key is still a plain variable name - + no path dialect - and is now matched wherever a value by that name sits. + It reaches a list entry only where a step consumes that field as + `item:`, so the bound follows the value into the pipeline argument + rather than the name into the JSON (`entry_constraint_fields`, + `dw/variable_constraints.py`). +3. **The run-time check stays.** It is the backstop for an inline workflow an + agent just wrote, which no declared constraint covers, and for a value a + parent workflow passed down. + +One thing the implementation resolved that the proposal did not anticipate: +the two families round in *opposite* directions. H3's `align_num_frames` +snaps a frame count **up** and then range-checks the aligned value, so the +H3 templates declare `snap: "up"` and the engine's bounds are checked against +the rounded number (108 is accepted, 346 refused). LTX-2.5's pipelines floor +an off-grid count instead - `(num_frames - 1) // 8 * 8 + 1` - so those +templates declare the `8 * n + 1` grid with **no** `snap`: an off-grid count +is refused with the rule stated, rather than rounded up (which would hand +back a longer clip than either the caller or the pipeline chose) or silently +floored (which is the behaviour being fixed). `snap: "down"` was deliberately +not added - one shape, not two. + +*Implementer agent, model `opus` via provider `anthropic`.* diff --git a/docs/proposals/step-callback-lead-in-instrumentation.md b/docs/proposals/step-callback-lead-in-instrumentation.md new file mode 100644 index 00000000..52d974a7 --- /dev/null +++ b/docs/proposals/step-callback-lead-in-instrumentation.md @@ -0,0 +1,135 @@ +# Design: naming the pre-denoise lead-in on step-callback pipelines + +Status: **proposal**. Written for #171 (filed by the regression agent, model +`opus` via provider `anthropic`, quoting a `triage: work` comment from +dkackman), investigated and drafted by the implementer agent, model `sonnet` +via provider `anthropic`, 2026-09-16. + +## The ask, as filed + +`templates/ltx2/text-to-video` ran 213.2s against a curated 1.8min (108s), +with 98.6s of it a silent "generating" lead-in before the first +`pipeline_step` event - unlike #95's block narration, this template emits no +block logs at all during that lead-in. The filer named two candidate causes: +the curated cost being stale, or the lead-in itself having grown as a +regression. A `triage: work` comment said this needs a measured run on `lem` +to tell the two apart. + +## What the measured run on `lem` shows + +I pulled the `events` JSON for three `templates/ltx2/text-to-video` jobs +directly from `jobs.sqlite` and read the phase timeline off `phase` / +`pipeline_step` / `step_end` entries (`at` is seconds since job start): + +**`bd2f45b50862`, 2026-09-13, 108.1s total (closest to curated 1.8min):** +loading 6.8→69.8 (63.0s: transformer 21.0s, text_encoder 17.4s, pipeline +24.6s) → generating phase starts 69.8, first denoise step at 77.7 (**7.9s +lead-in**) → 8 denoise steps 77.7→101.2 (23.5s) → decode 4.4s → save 2.5s. + +**`58430c14f254`, 2026-09-16, 213.2s total (today's #171 report):** +loading 1.8→84.0 (82.2s - already ~19s slower than the baseline run above) +→ generating phase starts 84.0, first denoise step at 182.6 (**98.6s +lead-in**, matching the issue exactly) → 8 denoise steps 182.6→206.0 (23.4s, +same pace as the baseline run) → decode 4.7s → save 1.9s. + +**`83444a2ff3f1`, 2026-09-13, 249.0s total (the C-F005 job #171 cites for +"no block logs during the lead-in"):** loading 2.3→69.8 (67.5s, normal) → +generating phase starts 69.8, first denoise step at 79.6 (**9.8s lead-in**, +*not* anomalous) → 8 denoise steps 23.4s (normal) → decode 12.1s (a little +high but not the outlier) → save phase starts 115.1, but `step_end` does not +land until 248.2 - **a 133.1s stall inside "saving"**, with none of the +`writing .../wrote ... in N.Ns` log pair the other two runs both have at this +point. + +Two conclusions: + +1. **The curated `cost.minutes: 1.8` is not stale.** `bd2f45b50862` lands at + 108.1s against a curated 108s almost exactly. Candidate cause (1) from the + issue is ruled out by this measurement. +2. **Two different intermittent stalls are being conflated as one symptom.** + Today's regression run stalls in the pre-denoise lead-in (encode_prompt / + connectors, per the architecture read below). The C-F005 job #171 cites as + corroboration has a *normal* lead-in and instead stalls in the save phase + (file write/mux). Denoise-step pacing (23.4-23.5s for 8 steps) and loading + time (63-82s) are the same order of magnitude across all three runs; + nothing in `git log -- dw/pipeline_processors/pipeline.py` between + 2026-09-13 and 2026-09-16 (`cd71fc5`, `20e59d3`) touches group offload, + LTX2, or the save/mux path, so there is no code change to pin either stall + to. Both look like host-side variance (GPU/CPU contention or I/O) + surfacing in two different uninstrumented places, not a single regressing + code path. + +## Why this needs a decision and not an edit + +`LTX2Pipeline` is a classic `DiffusionPipeline` subclass whose `__call__` +signature genuinely names `callback_on_step_end`, so dw takes the +step-callback instrumentation route (`_takes_step_callback` / +`_with_step_callback` in `dw/pipeline_processors/pipeline.py`), not the +block-narration route #95 added for true `ModularPipeline` instances. That +route only wraps the denoise loop itself - everything `__call__` does before +entering it (`check_inputs`, `self.encode_prompt(...)` against the +group-offloaded Gemma text encoder, `self.connectors(...)` against the +group-offloaded connectors component, latent prep) runs with no event of any +kind. That is architectural, not a bug in this template: any step-callback +pipeline with expensive pre-loop work has the same blind spot, and #95's +narration approach (patching tqdm / walking `SequentialPipelineBlocks`) does +not apply here since there is no blocks tree to walk. + +Adding visibility here is a new instrumentation concept for a whole class of +pipelines (patching or wrapping specific pre-loop calls - `encode_prompt`, +`connectors`, or a generic "time between phase-start and first callback" +watchdog), not a one-line fix, so it falls under guardrail 7. + +## Options + +### A. A generic "still working" watchdog log, no pipeline-specific hooks + +While waiting for the first `callback_on_step_end` invocation after a +`generating` phase event, emit a `log` event every N seconds (e.g. "still +generating, Ns since phase start, no denoise step yet"). Cheap, applies to +every step-callback pipeline uniformly, needs no per-pipeline knowledge of +`encode_prompt`/`connectors`. Downside: it says *that* something is slow, not +*what* - a consumer still can't tell text encoding from connector loading +from a genuine denoise-loop hang. + +### B. Named sub-phase events for LTX2's specific lead-in calls + +Wrap `encode_prompt` and `connectors` (or monkeypatch them the way +`_with_step_callback` wraps the pipeline's `__call__`) to emit `phase`/`log` +events naming each, mirroring #95's spirit but per-pipeline rather than +generic. Gives the precise breakdown this investigation needed to do by hand +against `jobs.sqlite`. Downside: bespoke per pipeline family - LTX2 today, +whichever pipeline is next tomorrow - and a maintenance burden if +`LTX2Pipeline.__call__`'s internals change upstream (this is exactly the kind +of coupling the ModularPipeline block-narration route was designed to avoid +by walking a declared tree instead of hardcoding call names). + +### C. Do nothing beyond noting the finding + +The curated cost is confirmed accurate; the two stalls found are each +one-off measurements, not a reproducible regression tied to a code change. +Recommend closing #171 as informational/wontfix and letting a future +instance of either stall (now that its shape is known) motivate whichever of +A/B is worth the maintenance cost. + +## Recommendation + +**A**, generic watchdog logging, as the first increment: it is the cheap half +that would have made both stalls visible in real time without per-pipeline +coupling, and it generalizes past LTX2 to any future step-callback pipeline +with expensive pre-loop work. B can follow later, scoped to LTX2 specifically, +if the watchdog shows this lead-in stalling often enough to be worth naming +precisely rather than just flagging. + +## Open questions for Don + +1. Is a periodic "still working, no denoise step yet" log acceptable, or does + it need to be structured (a distinct event type a client can key off, like + `phase`/`pipeline_step`) rather than a plain `log` message? +2. The C-F005 job's 133s save-phase stall (no `writing`/`wrote` log pair + appearing at all) looks like a separate, possibly more concerning gap - + worth its own issue, or fold into this one's watchdog scope since it's the + same "phase started, nothing else logged for a long time" shape? +3. Given the curated cost is confirmed accurate, should #171 stay open only + for the instrumentation question, or would you rather close it now (cost + figure vindicated) and let the watchdog work track separately if approved? diff --git a/docs/proposals/workspace-folders.md b/docs/proposals/workspace-folders.md new file mode 100644 index 00000000..36f6723f --- /dev/null +++ b/docs/proposals/workspace-folders.md @@ -0,0 +1,173 @@ +# Design: hierarchical workspace names + +Status: **proposal, not implemented**. Written for issue #167. Researched +2026-09-16 (feasibility comment on the issue, model claude-sonnet-5 via +provider anthropic); this document is the triaged follow-up (model +claude-sonnet-5, provider anthropic). + +## The ask, as filed + +Don's own QA workflow makes one workspace per test episode - +`QA`, `QA-EP1`, `QA-EP2` - and they pile up flat at the workspace root with +no relationship visible between them. The ask is for a workspace name to +carry structure - something like `QA/EP1` - so related workspaces group +under a common prefix the way subfolders already group files inside one +workspace's outputs (`docs/proposals/output-folders.md`) or entries inside +the workflow and prompt libraries. + +There is no reproduction here - nothing is broken. This is an ergonomics +request: the catalog and every workflow in it work today with workspaces +exactly as flat as they are. + +## Why this needs a design and not an edit + +A workspace name is load-bearing in three places that all assume it is +exactly one path segment: + +- **The security boundary.** `WORKSPACE_NAME_PATTERN = r"^[\w][\w.-]*\Z"` + (`dw/security.py:694`) refuses any separator on purpose - the comment + above it says a workspace is "one folder under the workspace root", so + `..`, a hidden name, and anything with `/` in it are excluded before the + name is ever joined onto the root. Allowing a separator is not a regex + tweak; it is loosening the exact rule that keeps `named_workspace` safe to + call with untrusted input. +- **The listing.** `workspace_names` (`dw/workspace.py:439`) is a single + non-recursive `os.listdir(workspace.root)`, keeping only entries that + `_holds_a_workspace` (all three of `NAMED_SUBDIRS` present). A workspace + called `QA/EP1` would have to be discovered by walking - and walking + raises the question of how deep, and whether an intermediate node like + `QA` is itself a listable, usable workspace or just a grouping label. +- **Deletion's safety check.** `delete_workspace` refuses to `rmtree` a + directory holding anything besides `NAMED_SUBDIRS` + `exports` + (`_foreign_entries`) - which is exactly what a directory holding *child* + workspaces would look like: `QA/` would contain `EP1/`, not + `workflows/`/`assets`/`outputs`, and the current check would call that + foreign and refuse to delete it (correctly, today, since nothing makes + `QA/` itself a workspace) or would need to learn a new legitimate shape. + +This is the same shape of change #69 (output/job folders) was held for: +a position-addressed name (there: `//`; +here: `/`) breaks every reader that assumes a fixed number of +segments once a segment can be inserted. Four readers assume one segment +today: `workspace_names`, `named_workspace`, the two MCP/REST workspace +routes, and the UI. That is worth deciding on paper before code moves. + +### What a child inherits, and from whom + +`named_workspace` already answers one version of this question for a +*flat* name: a named workspace points its `prompts_root` and +`common_root` at the true root's `prompts/` and `common/`, not at itself - +a stored prompt is shared by reference, so `prompt:scenic` must resolve to +the same text regardless of which workspace asked (`dw/workspace.py:88-90`). +Nesting reopens that question one level down: does `QA/EP1` share `QA`'s +own prompts and common assets, or only the true root's? The ticket doesn't +ask for scoped sharing, and inventing it now would be solving a problem +nobody has filed - but the proposal has to say so explicitly, because +silence here is where scope creeps in during implementation. + +### True nesting vs. a shallow naming convention + +Two designs satisfy "workspaces group under a prefix": + +1. **True nesting.** `QA` becomes a real, listable, usable workspace in its + own right, and `EP1` is a child of it with its own three folders. + `workspace_names` returns a tree or a set of `/`-joined paths; + `delete_workspace("QA")` has to decide whether it cascades to `EP1` or + refuses while children exist. +2. **Shallow sub-workspace convention.** A workspace name may contain + exactly one `/`, so `QA/EP1` is one flat workspace (one set of three + folders, one entry in `workspace_names`) whose *name* happens to have a + `/` in it and whose folder on disk is nested one level + (`/QA/EP1/{workflows,assets,outputs}`). `QA` alone is never a + workspace and is never listed, created, or deleted - it is purely a + grouping label baked into sibling names, the same precedent the + one-level `subfolder` field settled on for run outputs rather than + arbitrary output-tree nesting. + +**Recommendation: option 2.** It is the smaller change - one more +character class in `WORKSPACE_NAME_PATTERN`, one join in `named_workspace` +- and it answers "what does `QA` inherit from its parent" by making the +question not arise: there is no parent workspace, only a naming +convention. It also matches what the issue actually asked for: grouping in +a listing/UI, not a new addressable resource. True nesting (option 1) is a +bigger, genuinely different feature - listable intermediate nodes, cascade- +or-refuse delete semantics, inheritance rules for prompts/common assets at +each level - and nothing in the ask requires it. If Don wants real nesting +later, that is its own proposal built on top of this one, not a reason to +build it now. + +## The design (option 2) + +- **Name grammar.** `WORKSPACE_NAME_PATTERN` becomes + `r"^[\w][\w.-]*(/[\w][\w.-]*)*\Z"` - one or more segments, each obeying + today's per-segment rule, joined by a single `/`. This is the exact + pattern `output-folders.md` already uses for a `subfolder` segment + (`OUTPUT_REFERENCE_PATTERN`'s per-segment rule), so it is a precedent + already reviewed and shipped, not a new security surface. `..` stays + refused because no segment may start with `.` doubled as a whole segment + under `[\w][\w.-]*`; a leading, trailing, or doubled `/` is refused by + requiring each segment to start with `[\w]`. A depth cap of 2 (one `/`, + matching the ask's own example) rather than unbounded depth - unbounded + nesting is exactly the option-1 territory this proposal is declining. +- **Reserved names** apply per full name, not per segment - `QA/outputs` is + refused the same way `outputs` is today, since a segment named after one + of a workspace's own subfolders is exactly the collision + `RESERVED_WORKSPACE_NAMES` exists to prevent. +- **On disk**, `named_workspace(workspace, "QA/EP1")` joins the segments + the way it joins one today: `os.path.join(workspace.root, "QA", "EP1")`. + `os.path.join` with a validated, separator-free-per-segment name is safe + the same way it is safe for one segment - `validate_workspace_name` + is the barrier, exactly as `dw/security.py`'s CodeQL modeling already + expects for a workspace name. +- **`workspace_names`.** Still one call, but `os.walk` bounded to depth 2 + under `workspace.root` instead of `os.listdir`: a workspace is any + directory at depth 1 that `_holds_a_workspace`, or any directory at depth + 2 under a depth-1 grouping directory that does. A depth-1 directory that + itself holds `NAMED_SUBDIRS` is a workspace named by its own segment (no + change from today); a depth-1 directory that does *not* hold + `NAMED_SUBDIRS` but has depth-2 children that do is purely a grouping + prefix and contributes no entry of its own - `QA/` never appears in the + listing, only `QA/EP1` and `QA/EP2` do. This is what keeps `QA` from + becoming an addressable resource by accident. +- **Delete.** `delete_workspace(workspace, "QA/EP1")` behaves exactly as it + does today, `rmtree`-ing `/QA/EP1` after the same foreign-entries + check. If that leaves `/QA/` empty, it is removed too (nothing + should keep an empty grouping directory around); if `QA` still holds + other children, it is left alone. There is no `delete_workspace(root, + "QA")` - `QA` alone was never a workspace name in `workspace_names`, so + the existing `if name not in workspace_names(workspace): raise + FileNotFoundError` already refuses it with no new code. +- **MCP/REST surface: no shape change.** `create_workspace`, + `delete_workspace`, `list_workspaces` (REST: `POST/DELETE/GET + /api/workspaces`, MCP: `create_workspace`/`delete_workspace`/ + `list_workspaces`) already take a bare `name: str` - a name with a `/` in + it is still a string, so no request/response schema changes, no + `breaking-change` label. The only behavior change is that `name` may now + validate where it previously raised 400. +- **UI.** `ui/src/lib/workspace.svelte.ts` and the workspace switcher on + `ServerPage.svelte` read `workspace_names`/`list_workspaces` and today + render one flat list; grouping `QA/EP1`, `QA/EP2` under a `QA` heading in + that list (split the name on `/`, group by the prefix) is a presentation + change with no new API. Creating a child (`ServerPage.svelte`'s add- + workspace flow) needs its text field to accept a `/`، which is a one-line + relaxation of whatever client-side check mirrors + `WORKSPACE_NAME_PATTERN`, if any exists there today - worth confirming + during implementation rather than assuming. + +## What this proposal is not deciding + +- **True nesting** (option 1 above) - an addressable, listable, deletable + `QA` workspace with its own folders and children. Out of scope; a + separate proposal if ever wanted. +- **Per-level inheritance** of prompts/common assets - not needed under + option 2, since there is no parent workspace to inherit from. +- **A depth greater than 2.** If a future ask wants `QA/EP1/take3`, that is + a new proposal with its own reason, not an extension assumed here. + +## Rollout + +No migration: every existing workspace name is a valid one-segment name +under the new pattern, `workspace_names`'s depth-2 walk returns exactly what +`os.listdir` returned before for a root with no grouped names, and no +existing `output:`/`asset:`/`prompt:` reference or job record encodes a +workspace name, so nothing stored needs rewriting. diff --git a/docs/superpowers/plans/2026-09-14-assets-page-libraries.md b/docs/superpowers/plans/2026-09-14-assets-page-libraries.md new file mode 100644 index 00000000..242da196 --- /dev/null +++ b/docs/superpowers/plans/2026-09-14-assets-page-libraries.md @@ -0,0 +1,244 @@ +# Assets page: libraries as sections, review fixes, and cleanups + +Branch: `assets-page` (over `develop`). Spec: the code review findings in this +session plus the section-by-library design accepted by the user. There is no +separate spec file; the Global Constraints below are the binding text. + +## Global Constraints + +- Python: never `eval`/`exec`/`shell=True`; every disk read of a request-named + path goes through `validate_path(path, base)` with a non-None base (see + CLAUDE.md *Security Rules* - CodeQL models the validators as barriers). +- Tests: Python `python -m pytest tests/test_server.py tests/test_server_downloads.py tests/test_mcp_assets.py -q` + (run the focused file while iterating, the three once before committing); + UI `cd ui && npm test` and `npm run check` must both pass before a commit + that touches `ui/`. +- Every UI string a test asserts is quoted in the task; use it verbatim. +- Existing response fields of `GET /api/assets` (`asset_dir`, `asset_dirs`, + `assets`, `folders`, and every field of an `assets` entry) keep their names + and meaning - MCP `list_assets` returns this body verbatim. +- Commit per task, message in the repo's voice (imperative subject, a body + saying *why*), ending with `Co-Authored-By: Claude Fable 5.1 `. +- Comments explain *why*, in the style of the surrounding code; no narration + of what the line does. +- Do not touch `ui/src/lib/pages/GalleryPage.svelte` beyond what a task names. + +## Task 1: Archive routes - one search probe, one compression policy, one tail + +Files: `dw/server/app.py`, `tests/test_server_downloads.py`, `tests/test_server.py`, +`docs/SERVER.md`, `dw_mcp/CLAUDE.md`. + +1. **`_asset_in(name, roots)`** (app.py ~2262): stop calling + `resolve_asset_reference` per root (each call already searches the pinned + fallbacks, so the loop re-walks them - measured 107 syscalls per hit vs + 29). New body: `validate_asset_reference(name)` once (import from + `dw.assets` if not already; it raises `SecurityError` subclasses), then for + each root `candidate = os.path.join(root, name)`; if + `os.path.isfile(candidate)` return `validate_path(candidate, root)`. On a + total miss raise `HTTPException(404, detail=f"Unknown asset {name!r}: not found in {', '.join(roots)}")`; + when `roots` is empty the detail is + `f"Unknown asset {name!r}: this workspace has no asset library"`. A + `SecurityError` from validation becomes a 404 with `str(e)` as today. + Keep the docstring's *why* (bulk callers resolve many names against one + path). Fix the comment in `archive_assets` (~3075) so it claims only what + the code does. +2. **`_asset_file(reference, ws)`** stays a one-line wrapper; add a module-level + helper inside `create_app` named `_resolution_roots(ws)` returning + `_asset_roots(ws) or [ws.assets]` with a docstring saying why a missing + library is still named (so the 404 points at the caller's own directory), + and use it at the three sites (validate route ~1498, `_asset_file`, + `archive_assets`). +3. **`archive_assets`**: strip each name and dedupe preserving order before + resolving (`names = list(dict.fromkeys(n.strip() for n in body.names))`); + the zip entry name is the stripped name. Test: a body + `["iris.png", "iris.png "]` yields one entry named `iris.png`. +4. **Compression policy**: replace `COMPRESSIBLE_EXTENSIONS` with + `RAW_MEDIA_EXTENSIONS = {".bmp", ".wav"}` defined directly beneath + `MEDIA_KINDS` (~2237) with a comment: the allowlist members that are not + already-compressed containers. In `_zip_download`, an entry is + `ZIP_STORED` only when `extension in MEDIA_KINDS and extension not in RAW_MEDIA_EXTENSIONS`; + everything else (`.json`, `.md`, `.txt`, `.bmp`, `.wav`, unknown) is + `ZIP_DEFLATED`. Tests: (a) `assert RAW_MEDIA_EXTENSIONS <= set(MEDIA_KINDS)` + - expose both via `app.state` or import; pick whatever the existing test + for `MEDIA_KINDS` (if any) does, else attach them to `app.state` -; (b) the + export zip's `workflow.json` entry has `compress_type == zipfile.ZIP_DEFLATED` + (extend an existing export test in `tests/test_server_exports.py` or + `test_server_downloads.py`); (c) the existing + `test_archive_stores_already_compressed_media_rather_than_deflating_it` + still passes. +5. **One tail**: add `_archive_selection(entries, kind)` beside + `_zip_download`: it builds the zip via `_zip_download(entries, f"dw-{kind}s-{stamp}.zip")` + with `stamp = datetime.now().strftime("%Y%m%d-%H%M%S")`, and logs + `f"Archived {len(entries)} {kind} files"` *after* the archive is written. + Both archive routes call it (`kind` = `"output"` / `"asset"`), and the + duplicated three-line tails go. Check the download filename the UI/tests + expect - keep whatever `dw-outputs-*.zip` / `dw-assets-*.zip` names exist + today. +6. **Export route** (~3424): restore a plain two-line loop naming `path` and + `entry` (`entry = os.path.relpath(path, current).replace(os.sep, "/")`, + append `(f"{job_id}/{entry}", path)`), and restore the dropped "not a + second permanent copy" comment if git shows it (`git show develop:dw/server/app.py` + around the old export route). +7. **Docs**: `docs/SERVER.md` `POST /api/assets/archive` paragraph - replace + the "stored rather than deflated unless `.bmp`, `.wav`" sentence with the + new rule (media stored, everything else deflated; the export zip's text + files deflate). `dw_mcp/CLAUDE.md` line 10: "the gallery's bulk zip" -> + "the two bulk zips (gallery and assets)". + +## Task 2: `GET /api/assets` names its libraries and what they shadow + +Files: `dw/server/app.py` (`list_assets` ~2906), `tests/test_server.py`, +`docs/SERVER.md`, `docs/MCP.md` (the `list_assets` row), `dw_mcp/assets.py` +docstring, `ui/src/lib/types.ts`. + +1. Response gains `libraries`: a list in search-path order, one per root + `_asset_roots(ws)` returns, `{"origin": <"workspace"|"common"|"examples">, "dir": , "writable": }`. + `asset_dirs` stays (it is `[lib["dir"] for lib in libraries]`). +2. Response gains `shadowed`: entries the loop currently skips at + `if relative in seen: continue`. Each has the same fields as an `assets` + entry **except `url`** (the URL would serve the shadowing file) plus + `"shadowed_by": `. + `assets` is unchanged - exactly the files `asset:` resolves to. + Record the shadowing origin as you go (a dict `name -> origin` filled when + a name is first seen). +3. Tests in `tests/test_server.py` beside `test_the_asset_library_lists_what_it_holds`: + (a) `libraries` lists the workspace root first with `writable: true`; an + examples root (see `test_gallery_metadata_finds_an_asset_an_examples_tree_brought` + for how one is configured) is `writable: false`; (b) a name present in + both the workspace and an examples library appears once in `assets` + (origin `workspace`) and once in `shadowed` with `shadowed_by: "workspace"` + and no `url` key; (c) with no library configured the body has + `libraries: []` and `shadowed: []`. +4. `ui/src/lib/types.ts`: extend `AssetListing`'s type (find where + `listAssets` return type is declared - `api.ts` or `types.ts`) with + `libraries: AssetLibrary[]` and `shadowed: ShadowedAsset[]`; + `AssetLibrary = { origin: AssetFile['origin']; dir: string; writable: boolean }`, + `ShadowedAsset = Omit & { shadowed_by: AssetFile['origin'] }`. + Update `AssetsPage.test.ts`'s `listing` fixture to include + `libraries: [{ origin: 'workspace', dir: '/ws/assets', writable: true }]` + and `shadowed: []` so `npm run check` passes; do not change the page. +5. Docs: `docs/SERVER.md` `GET /api/assets` bullet gains one sentence each + for `libraries` and `shadowed` (why: a client that only sees the + resolved list cannot show which of its names hide a shared one). + `docs/MCP.md` `list_assets` row and `dw_mcp/assets.py` docstring mention + that `shadowed` lists names a nearer library hides. + +## Task 3: A bulk bar counts what its actions will touch + +Files: `ui/src/lib/picks.svelte.ts`, `ui/src/lib/picks.test.ts`, +`ui/src/lib/BulkBar.svelte`, `ui/src/lib/pages/AssetsPage.svelte`, +`ui/src/lib/pages/GalleryPage.svelte`, their tests, `ui/CLAUDE.md`. + +1. `Picks.size` returns `this.names.length` (ticks the filter hides are + inert: not counted, not acted on, kept until the filter lifts). Delete + `get hidden()` and its test. Update the doc comment on `size`. +2. `keepFailed(attempted: string[], failed: string[])`: delete every name in + `attempted` that is not in `failed`; touch nothing else (a hidden tick + survives). Update both callers (`removePicked` in each page passes + `names, failed`). +3. `picks.test.ts`: replace the `hidden` test with (a) "size counts only what + the order includes" - tick two, shrink the order to one, `size === 1`, + restore the order, `size === 2`; (b) "keepFailed keeps ticks the action + never attempted". +4. `AssetsPage.svelte` / `GalleryPage.svelte`: `downloadPicked` / + `removePicked` early-return when `picks.names.length === 0` (the bar is + hidden then anyway, but a keyboard path could still reach them). +5. `ui/CLAUDE.md` lines ~113-116: rewrite the `Picks.hidden` / open-question + sentences to state the rule now chosen: the count is the visible + selection, a hidden tick waits. +6. Run `npm test` and `npm run check`; fix any test that asserted the old + count. + +## Task 4: The assets page sections by library + +Files: `ui/src/lib/pages/AssetsPage.svelte`, `ui/src/lib/pages/AssetsPage.test.ts`, +`ui/CLAUDE.md`, `ui/src/lib/api.ts` only if `listAssets` needs its type. + +Depends on Task 2's `libraries`/`shadowed` fields and Task 3's `Picks`. + +Structure - the search path is the top level, folders inside: + +1. Derive `sections` from `libraries` in order: for each library, + `{ origin, dir, writable, label, assets: visible.filter(a => a.origin === origin), shadowed: listing.shadowed.filter(s => s.origin === origin && matches filter) }`. + Labels (verbatim): `workspace` -> `This workspace`, `common` -> + `Shared library`, `examples` -> `Examples`. A library with no assets and + no shadowed entries still renders its header (so an empty workspace shows + where an upload would land) unless a filter is active, in which case an + empty section is skipped. +2. Each section: a header row with the label, `(N)` count, the `dir` in + `.path` mono, a collapse chevron (state in `storageGet/storageSet` under + key `collapsed-asset-libraries`, a `Record`; while the + filter is active every section is open, as `FolderGroups` does), and: + - `writable` and origin `workspace`: an `Upload` button (the one filled + button on the page). + - `writable` and origin `common`: a `.quiet` `Upload to shared` button with + `title="lands in the shared library - visible from every workspace under this root and cannot be moved afterwards"`. + - not writable: a muted `read-only` span. + One hidden ``; a `let uploadTarget: 'workspace' | 'shared'` + set by whichever button was clicked before `fileInput.click()`. The + `window.prompt` text keeps its shape: `Upload — name in the ${uploadTarget} asset library:`. +3. Inside an open section: `FolderGroups` with `collapseKey="collapsed-asset-folders-${origin}"`, + the existing card snippet. Then, if the section has shadowed entries, one + more grid titled with a muted `shadowed/` heading rendered the way + `FolderGroups` renders a folder name (reuse its markup style, not the + component): tiles with class `cell shadowed` (opacity 0.45, no checkbox, + not a button - a `div`), caption = leaf name, `title="shadowed by this workspace's {name} - asset:{name} resolves to that file"` + where the shadowing origin's label is used in place of "this workspace's" + when `shadowed_by !== 'workspace'` (`shadowed by the shared library's ...`). +4. Remove: the `origin` state and `