diff --git a/AUDIT.md b/AUDIT.md index e5a78b1..dd62ad8 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -5,6 +5,19 @@ Date: 2026-08-11 Scope: workpc workers plus the homesrv orchestrator. This audit excludes FFmpeg changes and does not recommend Magi or its embeddings. No GPU workloads were run during the audit. +## Status + +Phase 1 landed on 2026-08-11. This file is the reasoning, not the state. + +| where the finding went | file | +| --- | --- | +| implemented, with evidence and a runnable check | `decisions/audit-phase1.md` | +| deliberately not done, with a revisit trigger | `caveats/audit-open.md` | +| still ahead, in order | `ROADMAP.md` | +| what to do next | `NEXT.md` | + +Every P0 below is closed. The sections that name one now point at its decision instead of repeating it. + ## Outcome The strongest performance gains are available without replacing the current models: @@ -21,18 +34,18 @@ contaminate the permanent character registry. ## Highest-priority findings -### P0: tracklet gender gate uses the wrong enum +### P0: tracklet gender gate uses the wrong enum — CLOSED, decisions/audit-phase1.md#tracklet-hard-constraints `orchestrator/tracklets.py` checks `male|female`, while vision emits `m|f|unknown`. The tracklet hard gender gate therefore never activates on normal pipeline data. -### P0: tracklets lack co-presence constraints +### P0: tracklets lack co-presence constraints — CLOSED, decisions/audit-phase1.md#tracklet-hard-constraints The linker has no same-panel exclusion and uses transitive union-find. Two similar-looking people in the same panel can be joined, and weak links such as A~B and B~C can merge A and C even when the endpoints are incompatible. -### P0: face-to-character pairing is unconditional +### P0: face-to-character pairing is unconditional — CLOSED, decisions/audit-phase1.md#gated-face-pairing `worker_vision._pair_faces_to_present()` assigns every detected face to the nearest vision character as long as an unused character exists. Despite the docstring, there is no distance or overlap threshold. @@ -44,18 +57,18 @@ The roster is described as hints-only, but those names are fed into detection. A causes `worker_identity` to persist it immediately. A coarse appearance-to-roster guess can therefore contaminate a permanent character gallery. -### P0: script repair feedback is discarded +### P0: script repair feedback is discarded — CLOSED, decisions/audit-phase1.md#verifier-false-positives The orchestrator sends `beat` and `verifier_feedback` on a failed-script retry, but `worker_script.ScriptInput` does not define either field and the prompt builder does not consume them. The second call is another stochastic attempt rather than a targeted correction. -### P0: action evidence is missing from the verifier +### P0: action evidence is missing from the verifier — CLOSED, decisions/audit-phase1.md#verifier-false-positives The beat builder reads plural `actions`, while the scene worker emits singular `action`. Script validation therefore receives little or no action evidence and cannot reliably detect invented or omitted actions. -### P0: GPU leases are unsafe for concurrent jobs +### P0: GPU leases are unsafe for concurrent jobs — CLOSED, decisions/audit-phase1.md#no-lease-stealing When `/session/open` returns 409, the homesrv proxy assumes the active lease is stale and closes it. A second legitimate job can terminate the first job's model. `heartbeat_session()` exists but is not used by the @@ -508,60 +521,29 @@ dependency-fingerprinted artifacts, and uncertainty-driven review for recurring/ ## Implementation sequence for approval -### Phase 1: correctness and scheduling safety +### Phase 1: correctness and scheduling safety — DONE 2026-08-11 -- Fix enum mismatches and add tracklet cannot-links. -- Gate face/body pairing. -- Wire action evidence and verifier feedback. -- Make stage clearing/resume behavior honest. -- Replace 409 lease stealing with a queue/wait policy. -- Run actual lease heartbeats. -- Register ComfyUI under the same GPU resource scheduler. +Implemented and verified by CPU-only self-checks. Nothing ran on the GPU. What landed, with evidence +and the check that covers it, is in `decisions/audit-phase1.md`. -Verification: CPU-only unit/self-checks. No full pipeline or GPU run until explicitly scheduled. +Two items from this list were NOT implemented and are recorded with a revisit trigger instead: +honest stage clearing (`caveats/audit-open.md#dishonest-clearing`) and ComfyUI under the GPU scheduler +(`caveats/audit-open.md#comfyui-unscheduled`). Both need a design decision, not a patch. ### Phase 2: safe throughput improvements -- Introduce the two-phase Gemma lease schedule. -- Add JSON-schema outputs. -- Add local media paths and the ephemeral file/reference cache. -- Reuse direction downloads. -- Batch SigLIP embeddings. -- Batch scene/DB operations. -- Add bounded fetch and framed-page crop concurrency. +Now tracked in `ROADMAP.md`, with the done-when condition for each phase. Verification: unit checks first, followed by one labeled chapter after GPU availability is approved. -### Phase 3: multi-view constrained identity +### Phases 3 and 4 -- Persist chapter-local tracklets. -- Add face/body galleries and crop-quality selection. -- Resolve with multi-view query evidence and global constraints. -- Separate name claims from visual identity. -- Expand identity evaluation. - -### Phase 4: evidence-ledger narration - -- Build ordered beat evidence artifacts. -- Add scene-scoped story and turn-taking state. -- Generate script chunks with evidence IDs. -- Add confidence-aware wording and targeted verification/repair. +Now tracked in `ROADMAP.md`. ## Acceptance criteria -Capture a baseline and compare the same labeled chapter after every phase: - -- stage wall time and model-load time; -- number of Gemma calls and repair calls; -- MinIO bytes downloaded/uploaded; -- prompt-evaluation and generation timings; -- tracklet false merges/splits and identity accuracy; -- name and speaker accuracy; -- unresolved correctness flags; -- script repetition, unsupported facts, and verifier retry rate. - -Recommended approval boundary: implement Phases 1 and 2 first, measure them, then decide whether to proceed -with the larger identity and narration changes. +Now tracked in `ROADMAP.md`, together with the approval boundary: measure Phase 2 before deciding +whether the larger identity and narration changes are worth their size. ## Second-pass findings (2026-08-11) @@ -572,7 +554,7 @@ The first pass audited each side on its own terms. Most of what follows lives in one repo changed a field's meaning and the other still reads the old one. The existing per-file `__main__` self-checks cannot catch any of it, because each one asserts its own side of the contract. -### P0: the scene stage discards every speaker +### P0: the scene stage discards every speaker — CLOSED, decisions/audit-phase1.md#speaker-ref-is-canonical `normalize_dialogue` rewrites `row["speaker"]` to a `character_id` or `None` (`orchestrator/correctness.py:47`). `run_stage_dialogue` saves that shape into the vision blob @@ -592,7 +574,7 @@ they were people. (`worker_scene.py:138`). Fix: prefer `speaker_ref` when its kind is `character_id`, keep the local-id path as the fallback. This is the strongest argument for the cross-stage metamorphic tests proposed above. -### P0: the script verifier rejects correct narration, and its retry is a no-op +### P0: the script verifier rejects correct narration, and its retry is a no-op — CLOSED, decisions/audit-phase1.md#verifier-false-positives `verify_script` fails a beat on two rules that fire on valid output (`orchestrator/correctness.py:100-116`). @@ -616,7 +598,7 @@ are dropped without an error. Fix order: correct the two rules first, then wire the feedback fields. Fixing the plumbing alone makes the model retry against a broken oracle. -### P0: correctness flags block TTS permanently in the default configuration +### P0: correctness flags block TTS permanently in the default configuration — CLOSED, decisions/audit-phase1.md#flag-resolution `run_stage_tts` refuses to start while any unresolved flag of kind `script-verifier`, `ambiguous-speaker`, `conflicting-name-claims`, or `partial-dialogue` exists (`service.py:1419-1423`). The only code that ever @@ -638,7 +620,7 @@ resolved, blocking TTS for all future jobs. Fix: resolve by flag identity rather than by surviving panel, and give the autonomous path an explicit policy (auto-resolve below a rank, or fail the stage loudly) instead of an unreachable gate. -### P0: `awaiting_review` is immediately overwritten by `failed` +### P0: `awaiting_review` is immediately overwritten by `failed` — CLOSED, decisions/audit-phase1.md#flag-resolution `run_stage_tts` sets `awaiting_review` and then raises (`service.py:1422-1423`). The raise unwinds into `_run_pipeline`'s catch-all, which sets the job to `failed` (`service.py:391-393`). The review state the @@ -646,7 +628,7 @@ stage just recorded is gone before anyone can read it, and the operator sees a g queue of flags. The gate path at `service.py:337-341` returns instead of raising and does not have this problem. -### P1: set-of-mark face pairing is enabled by default, contrary to its own documentation +### P1: set-of-mark face pairing is enabled by default, contrary to its own documentation — CLOSED, decisions/audit-phase1.md#gated-face-pairing `worker_vision.py:22` states the feature is off by default and should be enabled per title once tuned. `worker_vision.py:31` reads `SOM_ATTRIBUTION` with a default of `"1"`. It is on. @@ -661,7 +643,7 @@ default the flag to off as documented. The pairing is also greedy in face order rather than a joint assignment (`worker_vision.py:42-52`), so the first face processed can claim a character that fits a later face far better. -### P1: the JSON repair pass can fabricate content +### P1: the JSON repair pass can fabricate content — OPEN, caveats/audit-open.md#repair-fabricates When a response fails to parse, `call_gemma4_json` sends the model its own truncated text and asks for the JSON it should have been (`worker_vision.py:174-178`). The repair call carries no image. On a response @@ -671,7 +653,7 @@ adds is invented and is indistinguishable downstream from transcribed text. Schema-constrained generation, already recommended above, removes most of this path. Until then the repair pass should re-send the image, or a truncated response should be retried rather than repaired. -### P1: `/review/preview` silently replaces a beat clip +### P1: `/review/preview` silently replaces a beat clip — OPEN, caveats/audit-open.md#preview-overwrites-clip `review_preview` renders one panel alone and calls `save_clip(panel_id, ...)` (`service.py:1902-1904`). Its docstring calls this harmless because assemble would regenerate it. It does not. `_render_one_beat` returns @@ -680,7 +662,7 @@ pins the solo preview into the final video and drops the rest of the beat's pane right by calling `delete_clip` first (`service.py:2008`). Preview should write to a scratch key or delete the clip row afterwards. -### P1: the session manager can orphan a llama-server and hold the GPU +### P1: the session manager can orphan a llama-server and hold the GPU — CLOSED, decisions/audit-phase1.md#unlocked-model-load `open_session` claims the slot, releases the lock, then spawns and health-waits for up to 300 seconds (`session_manager.py:108-119`). A `/session/close` arriving during that window finds `proc = None`, tears @@ -693,7 +675,7 @@ Related asymmetry: `_supervise_once` calls `_start_subprocess` while holding `_l (`session_manager.py:169-181`), so a respawn blocks `/session/active`, `/session/close`, and `/session/open` for the full health wait. The open path was deliberately written to avoid exactly this. -### P1: nothing limits concurrent jobs +### P1: nothing limits concurrent jobs — CLOSED, decisions/audit-phase1.md#no-lease-stealing `_background_jobs` accepts any number of pipelines (`service.py:302-303`, `service.py:427-428`). Each opens its own model sessions. Combined with the 409 lease stealing already recorded, two jobs terminate each @@ -717,7 +699,7 @@ vector spaces, and the fixed 0.85 threshold is only valid for one of them. Nothi vectors into one gallery with no error. Write the model id and pooling mode next to the vector and refuse to compare across versions. -### P2: smaller confirmed defects +### P2: smaller confirmed defects — partly closed, see `decisions/audit-phase1.md#related` and `caveats/audit-open.md` - An out-of-range `choice` from the resolver is mapped to NONE and then reported as `state: "new"` (`worker_vision.py:898-899`), so a hallucinated index mints a brand new character. It should be @@ -755,16 +737,9 @@ compare across versions. - MinIO credentials are hardcoded as defaults in committed source (`transport.py:95-99`, `service.py:74-76`). -### What this changes in the plan +### What this changes in the plan — DONE -Add to Phase 1, before any throughput work: +All five additions landed in Phase 1. See `decisions/audit-phase1.md`. -1. Restore the speaker field across the dialogue, scene, and script boundary. -2. Correct the two verifier rules, then wire `beat` and `verifier_feedback`. -3. Give correctness flags a resolution path that does not require a disabled gate, and stop the TTS block - from erasing `awaiting_review`. -4. Decide the set-of-mark default deliberately, and gate face pairing before the label is trusted. -5. Add job admission control at the orchestrator, not only lease queueing in the proxy. - -Add to the acceptance criteria: the share of narrated lines whose speaker is a named character rather than -`Someone`. That single number would have caught the first finding on the day it landed. +The acceptance metric they suggested, the share of narrated lines whose speaker is a named character +rather than `Someone`, is now in `ROADMAP.md`. It has not been measured yet. diff --git a/CLAUDE.md b/CLAUDE.md index 9461ab6..6dffd06 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,67 +1,72 @@ # CLAUDE.md -This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +Goal, invariants, and working rules. Read this first. -## What this is +| file | holds | +| --- | --- | +| `NEXT.md` | the current state and the live plan | +| `ROADMAP.md` | the ordered outcomes past the current one | +| `JOURNAL.md` | what was run and when, append-only | +| `decisions/` | every settled question, indexed in `decisions/CLAUDE.md` | +| `caveats/` | every known limit and its revisit trigger, indexed in `caveats/CLAUDE.md` | +| `AGENTS.md` | commands, with the traps beside them | +| `AUDIT.md` | the 2026-08-11 pipeline audit, the source of the roadmap | +| `spec-v3.md` | current quality and look work, marked DONE/TODO per item | -The **workpc compute half** of a manga→narrated-video pipeline. This repo holds stateless -GPU/CPU workers only. State, job scheduling, and stage orchestration live in a **separate -homesrv orchestrator repo** (`/mnt/server/home/kami/apps/Maven/` region) — not here. The two -communicate over a fixed HTTP contract; never add sqlite or durable state to a worker. +Do not restate a finding here. Point at the decision. -Machine split (memorize): workers run on **workpc** (RX 7900 GRE, ROCm). MinIO + orchestrator -run on **homesrv** (`192.168.1.104`, CPU-only). Data dir `/mnt/server/home/kami/` is an SSHFS -mount of homesrv. +## The goal -## Run / test +This is the **workpc compute half** of a manga to narrated-video pipeline. It holds stateless GPU and +CPU workers only. State, job scheduling, and stage orchestration live in a separate homesrv +orchestrator repo (`/mnt/server/home/kami/docker-apps/manga-infra/orchestrator/`). The two talk over a +fixed HTTP contract. The hard part is that one GPU serves every model, so the schedule, not the model, +sets the wall time. + +Machine split: workers run on **workpc** (RX 7900 GRE, ROCm). MinIO and the orchestrator run on +**homesrv** (`192.168.1.104`, CPU-only). `/mnt/server/home/kami/` is an SSHFS mount of homesrv. + +## Invariants + +1. **No durable state in a worker.** No sqlite, no cross-request memory. A worker pulls inputs from + MinIO by URI, does one stage, pushes outputs back, returns URIs. A `/dev/shm` cache is allowed + because it may be dropped at any time. +2. **One warm model at a time.** Every GPU stage takes a lease from `session_manager.py` on 8095. A 409 + is a queue signal, never a stale lease + (`decisions/audit-phase1.md#no-lease-stealing`). +3. **Never hold `_lock` across a model load or a health wait** in `session_manager.py` + (`decisions/audit-phase1.md#unlocked-model-load`). +4. **A dialogue row's speaker is `speaker_ref`.** The flat `speaker` field is a compatibility value and + holds a `character_id`, not a panel-local id + (`decisions/audit-phase1.md#speaker-ref-is-canonical`). +5. **A stage never raises after setting `awaiting_review`.** The pipeline's catch-all overwrites it + with `failed` (`decisions/audit-phase1.md#flag-resolution`). +6. **Never mint a character from an unparseable or out-of-range model answer.** That is `unresolved` + (`decisions/audit-phase1.md#hallucinated-index`). +7. **The HTTP contract with the orchestrator is load-bearing.** Changing a worker's request or response + shape means reconciling the orchestrator in the same session. Neither repo's self-checks can catch a + contract break, because each asserts its own side. +8. **`ponytail:` comments mark deliberate simplifications** and name the upgrade path. Respect them. + +## Working rules ```bash ./start_workers.sh # dev: session_manager + 9 workers, each a uvicorn in a tmux window -tmux attach -t manga-workers # watch logs; per-worker window +tmux attach -t manga-workers # per-worker logs sudo systemd/install.sh # production: one systemd unit per process (User=kami) -python session_manager.py # each module has a __main__ self-check (assert-based, no framework) -python transport.py # run these to verify a file after editing it -python test_vision_parse.py # the one standalone pytest-free test +.venv/bin/python worker_scene.py # every module has an assert-based __main__ self-check +.venv/bin/python test_vision_parse.py + +cd /mnt/server/home/kami/docker-apps/manga-infra/orchestrator && pytest -q --ignore=test_api.py ``` -There is no lint/build step. `.venv` is the ROCm torch env; workers import `transport` by module name. +There is no lint or build step. `.venv` is the ROCm torch env. Workers import `transport` by module +name. Ports: crop 8000, vision 8002, identity 8003, scene 8004, script 8005, tts 8006, layers 8007, +render 8008, session_manager 8095. -## Architecture - -**Workers are stateless HTTP stages.** Each `worker_*.py` is a FastAPI app on a fixed port. It -pulls inputs from MinIO by URI to local disk (`/dev/shm`), does one stage, pushes outputs back, -returns URIs. No cross-request memory. Ports: crop 8000, vision 8002, identity 8003, scene 8004, -script 8005, tts 8006, layers 8007, render 8008, **session_manager 8095**. - -**`transport.py`** — shared MinIO client (`get`/`put`/`put_bytes`/`exists`) + `install_logging(app, name)` -(one log line per request with panel id). URIs are `s3://bucket/key`. Import it in every worker. - -**`session_manager.py`** — the **GPU mutex**. Only one warm model at a time on the single local GPU. -Two model kinds: -- *subprocess* (`gemma4`): it spawns/health-waits/terminates `llama-server`, and a supervisor - respawns it in-place if it crashes mid-session (keeps the same session_id + port). -- *in-process* (`siglip2`, `dots`): returns `port=None`; the **worker** loads the transformers model - itself and must expose `/unload` so `/session/close` can free the ~5GB VRAM (the mutex alone - can't reclaim it). Leases have a TTL + heartbeat; a reaper force-closes stale ones. - -A GPU worker's flow: `/session/open {model}` → (409 if busy) → do work against the returned port or -its own resident model → `/session/close`. CPU workers (crop) take no session. - -## Conventions - -- **`ponytail:` comments** mark deliberate simplifications and name the upgrade path — respect them, - don't "fix" them without reason. -- Any non-trivial logic gets ONE runnable check in `__main__` (assert-based `demo`/self-check), not a - test suite. Follow that pattern; run the file to verify. -- The HTTP contract with the orchestrator is load-bearing and shared across repos — changing a - worker's request/response shape means reconciling the orchestrator too (see commit history: - "reconcile worker contracts"). - -## Specs & docs - -- `spec-v3.md` — current quality/look work (narration voice, panel curation, render overhaul), marked - DONE/TODO per item. `legacy/` holds the old single-repo workers. -- `AGENTS.md` — use `scripts/analyze_video_frames.sh` (not manual seeking) to diagnose render output; - its frames are `/tmp` artifacts, never commit them. -- `collage.py` (behind `COLLAGE` flag) and the render worker carry the animated-layout planners. +- Any non-trivial logic gets ONE runnable check in `__main__`, assert-based, no framework. Run the file + to verify it. +- Editing a worker's request or response shape means editing the orchestrator too, in the same session. +- Update `NEXT.md` alongside any change that moves the plan, and append to `JOURNAL.md` after a run. +- Do not run GPU work or a full pipeline without asking. diff --git a/HANDOFF.md b/HANDOFF.md deleted file mode 100644 index e44c67f..0000000 --- a/HANDOFF.md +++ /dev/null @@ -1,54 +0,0 @@ -# HANDOFF: audit second pass (2026-08-11) - -## What was asked -Review `AUDIT.md` (untracked, first pass, 565 lines) and add anything missing. - -## What was done -Read both halves of the pipeline and checked the first pass against source. No code changed. -No GPU work, no pipeline run, no tests executed. - -Files read in full: -- workpc: `worker_vision.py` (1008), `worker_identity.py` (322), `worker_script.py` (300), - `worker_scene.py` (170), `worker_tts.py` (241), `session_manager.py` (238), `transport.py` (216), - `worker_crop.py` (first 120 of 359). -- homesrv `/mnt/server/home/kami/docker-apps/manga-infra/orchestrator/`: `service.py` (2048), - `db.py` (815), `session_proxy.py` (302), `correctness.py` (116), `tracklets.py` (121), - `minio_layout.py` (105), `grouping.py` (75). - -Result: `AUDIT.md` grew from 565 to 745 lines. New section `## Second-pass findings (2026-08-11)` -at line 566. 4 new P0, 6 new P1, 13 P2, plus 5 additions to the Phase 1 list and 1 to the -acceptance criteria. - -Four first-pass claims were confirmed in source and are not restated in the new section: - -- tracklet gender enum: `tracklets.py:43` uses `male|female`, vision emits `m|f|unknown`. -- missing action evidence: `worker_scene.py:85` emits `action`, `correctness.py:91` reads `actions`. -- dropped verifier feedback: `worker_script.py:13-23` defines neither field. -- 409 lease stealing: `session_proxy.py:40-49`. - -## Top 4 new findings -1. `worker_scene.py:78` reads `speaker` as a local id, but `correctness.py:47` already rewrote it - to a character_id. Every lookup returns None, so all narration says `Someone`. -2. `correctness.py:106-115` fails valid narration on sentence-initial capitals and on short - quotes, and `service.py:1384-1386` turns that into a halted chapter. -3. `service.py:1419-1423` blocks TTS on correctness flags that only `/review/approve` can clear, - while `GATES` defaults to off (`service.py:312`). -4. `session_manager.py:108-119` can orphan a llama-server that keeps its VRAM. - -## Still open -- Nothing from this session is half-finished. `AUDIT.md` is complete as written. -- No finding has been fixed. All are report-only. -- `AUDIT.md` is still untracked as of this session's start. Decide whether it belongs in git. -- Prose linter reports ~97 style hits in `AUDIT.md`. Almost all are in the first-pass sections - (lines 1-565), which were left as the author wrote them. -- Carried over from the earlier reconstruction handoff of the same day: - - `.venv/` is gone. Rebuild it, then install the ROCm torch wheel the way workpc had it. - - `dots.tts/` and `legacy/` are not recoverable from transcripts. - - `RESUME_SPEC.md`, `pipeline-design-notes.md`, and `spec-v2.md` are unrecoverable. - -## Next command -``` -cd /home/kami/Programs/n8n-worker -sed -n '566,745p' AUDIT.md # read the new section -``` -Then pick the Phase 1 order at `AUDIT.md:511` as amended at the end of the new section. diff --git a/JOURNAL.md b/JOURNAL.md new file mode 100644 index 0000000..6d90e68 --- /dev/null +++ b/JOURNAL.md @@ -0,0 +1,22 @@ +# JOURNAL + +Append-only, newest last. One block per session or run. Not a changelog: this records what happened on +the day a number was produced, so a later postmortem can find it. + +## 2026-08-11 Audit second pass [no task] + +Command: none. Source reading only. +Outcome: finished. `AUDIT.md` grew from 565 to 771 lines with a `## Second-pass findings` section: +4 new P0, 6 new P1, 13 P2, 5 additions to the Phase 1 list. +Produced: commit `6d9df5b`, `AUDIT.md:566`. + +## 2026-08-11 Audit Phase 1 implemented [#203] + +Command: `python worker_scene.py worker_script.py worker_vision.py session_manager.py`, +`pytest -q --ignore=test_api.py` in the orchestrator. +Outcome: finished. All self-checks pass, 108 orchestrator tests pass. No GPU work, no pipeline run. +`test_api.py` was skipped because fastapi is not installed in the workpc venv. +Produced: `decisions/audit-phase1.md`, `caveats/audit-open.md`, `ROADMAP.md`, and this scaffold. + +One existing test asserted the bug: `test_name_binding.test_conflict_flags_and_stays_unnamed` relied on +orphan flags leaking into every chapter, because it never created panel rows. It now creates them. diff --git a/NEXT.md b/NEXT.md new file mode 100644 index 0000000..99a4bca --- /dev/null +++ b/NEXT.md @@ -0,0 +1,42 @@ +# NEXT + +Updated 2026-08-11. Replaces the old `HANDOFF.md`. + +## State + +Audit Phase 1 is implemented and green. Nothing is half-finished. + +Changed on workpc: `worker_scene.py`, `worker_script.py`, `worker_vision.py`, `session_manager.py`. +Changed on homesrv (`/mnt/server/home/kami/docker-apps/manga-infra/orchestrator/`): `tracklets.py`, +`correctness.py`, `db.py`, `service.py`, `session_proxy.py`, `test_script_verify.py`, +`test_name_binding.py`. + +What landed and why: `decisions/audit-phase1.md`. What was left open: `caveats/audit-open.md`. + +Verification: CPU-only self-checks and the orchestrator test suite. 108 orchestrator tests pass +(`test_api.py` is excluded on workpc because fastapi is not installed in this venv). No GPU work ran +and no pipeline ran, so none of this is confirmed against a real chapter. + +The orchestrator changes are edited in place on the SSHFS mount and are NOT committed. Its git root is +`/mnt/server/home/kami/docker-apps`. Its container also needs a rebuild or restart to pick them up. + +## Next + +1. Commit the orchestrator half in `/mnt/server/home/kami/docker-apps` and restart the container. +2. Run one labeled chapter end to end and record the baseline numbers from `ROADMAP.md`, especially the + share of narrated lines with a named speaker. That number is the check on the largest Phase 1 fix. +3. Start Phase 2 from `ROADMAP.md`. Set SQLite `busy_timeout` before any concurrency work + (`caveats/audit-open.md#sqlite-locking`). + +## Open questions + +Four Phase 1 items have no Vikunja task and were not created, because writing to the tracker was not +asked for: the speaker contract fix, the verifier rules, the tracklet constraints, and the flag +resolution path. Only [#203] existed and is now closed by `decisions/audit-phase1.md#unlocked-model-load`. + +Three audit items are deliberately not done and are recorded as caveats rather than silently dropped: +honest stage clearing, ComfyUI under the session mutex, and reversible identity merges. Each needs a +design decision, not a patch. + +Carried over from the reconstruction: `.venv` needs the ROCm torch wheel reinstalled, and `dots.tts/`, +`legacy/`, `RESUME_SPEC.md`, `pipeline-design-notes.md`, `spec-v2.md` are unrecoverable. diff --git a/ROADMAP.md b/ROADMAP.md new file mode 100644 index 0000000..0b5172c --- /dev/null +++ b/ROADMAP.md @@ -0,0 +1,66 @@ +# ROADMAP + +Ordered outcomes past the current one. Phase 1 landed on 2026-08-11 and moved to +`decisions/audit-phase1.md`. The reasoning behind every phase is in `AUDIT.md`. + +Approval boundary from the audit: ship Phase 2 and measure it. Then decide whether Phase 3 and 4 are +worth the size of the change. + +## Phase 2: safe throughput + +Unblocks: a chapter that finishes in a fraction of the current wall time without changing any model. +Done when: the same labeled chapter runs with two gemma loads instead of seven. The acceptance numbers +below are recorded in a report. + +- Two-phase gemma lease schedule, so the model stays resident across compatible stages. +- JSON-schema-constrained generation for detect, dialogue, roster, direction, same-person, and resolve. +- `--media-path /dev/shm` and `file://` image paths instead of base64 data URIs. +- An ephemeral `/dev/shm` cache keyed by S3 URI plus etag, and reused direction downloads. +- Batched siglip embeddings, one forward pass per bounded image batch. +- Batched scene construction and SQLite writes. Set `busy_timeout` first + (`caveats/audit-open.md#sqlite-locking`). +- Bounded fetch and framed-page crop concurrency. +- Images before the differing instruction in every multi-call prompt, so llama-server reuses its KV + cache prefix across the two direction passes. + +## Phase 3: multi-view constrained identity + +Unblocks: an identity that survives a reviewer disagreeing with it. +Done when: false merges and false splits are reported per labeled chapter and the resolver beats the +Phase 1 baseline on both. + +- Chapter-local tracklets persisted as a resumable homesrv artifact, not worker memory. +- Face and body galleries with crop-quality selection, and 2-4 query views per tracklet. +- Global constraints at resolution: must-link inside a tracklet, cannot-link for co-presence, gender + and species gates, manual locks, one appearance per character per panel. +- Name claims separated from visual identity. Roster names stay candidate vocabulary. +- Reversible merges: a cluster with `canonical_id`, `member_ids`, and `cannot_link`, replacing the + destructive delete (`caveats/audit-open.md#destructive-reconcile`). +- Identity evaluation extended to purity, fragmentation, name binding, and speaker accuracy. + +## Phase 4: evidence-ledger narration + +Unblocks: narration that can be checked against the panel rather than trusted. +Done when: every beat carries an evidence artifact and the verifier reports quote grounding, action +coverage, and future-fact leakage against it. + +- Ordered beat evidence: panels, actions, chronology, dialogue ids, typed speaker refs, uncertainty. +- Scene-scoped story and turn-taking state, reset on location and time changes. +- Script chunks of 6-12 consecutive beats per text-only call, returning evidence ids. +- Confidence-aware wording: a weak speaker becomes `someone`, not a confident name. +- Targeted per-beat repair driven by the exact failures. + +## Acceptance criteria + +Capture a baseline and compare the same labeled chapter after every phase. + +- Stage wall time and model-load time. +- Gemma call count and repair call count. +- MinIO bytes moved, prompt-evaluation and generation timings. +- Tracklet false merges and splits, identity accuracy. +- Name and speaker accuracy, unresolved correctness flags. +- Script repetition, unsupported facts, verifier retry rate. +- The share of narrated lines whose speaker is a named character rather than `Someone`. That one + number would have caught `decisions/audit-phase1.md#speaker-ref-is-canonical` the day it landed. + +Read `caveats/audit-open.md#inconsistent-stage-policy` before trusting any per-stage count. diff --git a/caveats/CLAUDE.md b/caveats/CLAUDE.md new file mode 100644 index 0000000..459f244 --- /dev/null +++ b/caveats/CLAUDE.md @@ -0,0 +1,34 @@ +# caveats/ + +One entry per known breakage, deferral, or trap. An entry names what fails, who pays for it, and the +concrete trigger that makes fixing it worth the time. + +A caveat that gets resolved moves to `decisions/` or gets deleted. A caveat with no revisit trigger is +a complaint, so give it one or drop it. + +## Rules for this directory + +* One file per source, one `##` section per limit. +* State the limit as what fails, not as a topic. "Reconcile deletes the losing character" beats + "Reconcile". +* Delete an entry the moment its trigger fires and the work lands. + +## Index + +| caveat | source | +| --- | --- | +| [Reconcile deletes the losing character irreversibly](audit-open.md#destructive-reconcile) | AUDIT.md | +| [Clearing a stage does not undo what it wrote](audit-open.md#dishonest-clearing) | AUDIT.md | +| [ComfyUI uses the GPU outside the session mutex](audit-open.md#comfyui-unscheduled) | AUDIT.md | +| [The JSON repair pass can fabricate dialogue](audit-open.md#repair-fabricates) | AUDIT.md | +| [`/review/preview` pins a solo clip into the final video](audit-open.md#preview-overwrites-clip) | AUDIT.md | +| [Stored embeddings carry no model or pooling version](audit-open.md#untagged-embeddings) | AUDIT.md | +| [Worker endpoints block the event loop](audit-open.md#blocking-event-loop) | AUDIT.md | +| [SQLite has no busy timeout, so parallelism will surface as lock errors](audit-open.md#sqlite-locking) | AUDIT.md | +| [`layers` runs after `tts`, so pipelined solo beats lose parallax](audit-open.md#layers-after-tts) | AUDIT.md | +| [`completed` means something different in each stage](audit-open.md#inconsistent-stage-policy) | AUDIT.md | +| [Identity worker caches characters the orchestrator has deleted](audit-open.md#stale-known-cache) | AUDIT.md | +| [A character seen once gets no assignment at all](audit-open.md#pending-in-worker-memory) | AUDIT.md | +| [MinIO credentials are hardcoded in committed source](audit-open.md#hardcoded-credentials) | AUDIT.md | +| [Assemble marks a job completed with no clips](audit-open.md#empty-assemble) | AUDIT.md | +| [Reviewer timestamps drift against the crossfaded video](audit-open.md#timeline-drift) | AUDIT.md | diff --git a/caveats/audit-open.md b/caveats/audit-open.md new file mode 100644 index 0000000..acd8a7c --- /dev/null +++ b/caveats/audit-open.md @@ -0,0 +1,149 @@ +# Open limits from the 2026-08-11 audit + +Everything here was read in source during the audit and deliberately left unfixed in Phase 1. The +fixed findings live in `decisions/audit-phase1.md`. Line numbers are from the audit and may drift. + +## Reconcile deletes the losing character irreversibly {#destructive-reconcile} + +Reconciliation deletes the losing character row (`db.py:506`). Clearing the reconcile stage does not +undo it, and name claims attached to the merged-away character are not repointed. + +Costs: one bad merge is unrecoverable without rebuilding the identity stage for the whole manga. +Revisit when: identity work resumes, or a reviewer reports a wrong merge on a real chapter. +Workaround: none. Clear identity and rerun, which loses the good merges too. + +## Clearing a stage does not undo what it wrote {#dishonest-clearing} + +Dialogue and direction mutate the shared vision JSON. Clearing dialogue leaves its keys in place, so a +rerun treats old dialogue as completed. Clearing identity preserves the per-manga registry. + +Costs: a rerun silently reuses stale output, which reads as a reproducible result. +Revisit when: any stage is scheduled concurrently or resumed automatically. A stage must be idempotent +before either is safe. +Workaround: delete the keys by hand, or clear from `crop` down. + +## ComfyUI uses the GPU outside the session mutex {#comfyui-unscheduled} + +The layers stage calls ComfyUI directly and takes no lease. Another job can load gemma, siglip2, or +dots while ComfyUI holds the same GPU. + +Costs: out-of-memory failures that look random and land on an unrelated stage. +Revisit when: layers is enabled on a real run, or a second concurrent job is allowed. +Workaround: `MAX_CONCURRENT_JOBS=1` keeps one pipeline at a time, which is the current default. + +## The JSON repair pass can fabricate dialogue {#repair-fabricates} + +`call_gemma4_json` hands the model its own truncated text and asks for the JSON it should have been +(`worker_vision.py`). The repair call carries no image. On a response truncated by `max_tokens`, the +model completes dialogue it can no longer see. What it adds is indistinguishable downstream from +transcribed text. + +Costs: invented lines enter the script with normal provenance. +Revisit when: schema-constrained generation lands, which removes most of this path. +Workaround: resend the image on repair, or retry a truncated response instead of repairing it. + +## `/review/preview` pins a solo clip into the final video {#preview-overwrites-clip} + +`review_preview` renders one panel and calls `save_clip(panel_id, ...)`. `_render_one_beat` returns +early when a clip already exists for the leader. Previewing a beat leader therefore drops the rest of +the beat's panels. `review_retts` gets this right by calling `delete_clip` first. + +Costs: a reviewer silently corrupts the output by looking at it. +Revisit when: the review UI is used on a real chapter. +Workaround: never preview a beat leader, or delete the clip row afterwards. + +## Stored embeddings carry no model or pooling version {#untagged-embeddings} + +`embed_crop` uses `pooler_output` when present and falls back to mean-pooled patch tokens otherwise. +The two paths produce different vector spaces, and the fixed 0.85 threshold is valid for one of them. +Nothing beside a stored `.npy` records which model, revision, or pooling produced it. + +Costs: a transformers upgrade mixes incompatible vectors into one gallery with no error. +Revisit when: transformers or the siglip2 revision is upgraded. Before, not after. +Workaround: none. Write the model id and pooling mode beside the vector and refuse cross-version +comparison. + +## Worker endpoints block the event loop {#blocking-event-loop} + +Endpoints declared `async def` run blocking MinIO, OpenCV, torch, and ffmpeg calls directly on the +event loop. A busy worker cannot answer `/health` or `/unload`. + +Costs: `/health/workers` reports a working worker as unreachable, and the session manager's 30-second +`/unload` can time out exactly when VRAM needs freeing. +Revisit when: a stage stalls on `/unload`, or before any bounded parallelism lands. +Workaround: `def` instead of `async def` moves each handler to the threadpool. Tracked as [#199] for +the render worker. + +## SQLite has no busy timeout {#sqlite-locking} + +`get_conn` opens a connection per call with no `busy_timeout`. WAL tolerates one writer. `PIPELINE=1` +already writes clips from concurrent tasks while TTS writes audio. + +Costs: planned CPU parallelism will surface as `database is locked` before it surfaces as throughput. +Revisit when: Phase 2 concurrency work starts. Set the timeout first. +Workaround: keep `PIPELINE` off. + +## `layers` runs after `tts` {#layers-after-tts} + +`STAGES` orders `layers` after `tts`, while `run_stage_tts` warns that eager rendering under +`PIPELINE=1` needs layers to run first. + +Costs: with the flag on, solo beats always render without parallax. +Revisit when: a real run enables `PIPELINE=1`. +Workaround: keep `PIPELINE` off, or reorder `STAGES`. + +## `completed` means something different in each stage {#inconsistent-stage-policy} + +A dropped vision panel fails the stage and halts the pipeline. A failed direction window counts its +panels as done. Layers always finishes completed. + +Costs: the acceptance metrics in `ROADMAP.md` cannot be read across stages. +Revisit when: a baseline measurement is taken. The numbers are meaningless until then. +Workaround: none. + +## Identity worker caches characters the orchestrator has deleted {#stale-known-cache} + +`worker_identity._known_cache` is invalidated only by `_persist_char`. The reconcile stage deletes +losing characters directly in the orchestrator database. A long-lived worker keeps shortlisting and +assigning ids that no longer exist. + +Costs: assignments point at rows that are gone. +Revisit when: reconcile runs on a chapter without a worker restart between stages. Tracked as [#201], +which proposes caching per `manga_id`. +Workaround: restart the identity worker after reconcile. + +## A character seen once gets no assignment at all {#pending-in-worker-memory} + +`worker_identity._pending` holds full crop images in worker memory for a whole chapter, keyed by +session. That is durable per-chapter state inside a worker documented as stateless, and it is lost on +restart. A character seen exactly once receives no assignment, not even a chapter-local handle. + +Costs: one-off characters vanish from the scene graph. +Revisit when: chapter-local tracklet persistence lands (`ROADMAP.md`, Phase 3). +Workaround: none. + +## MinIO credentials are hardcoded in committed source {#hardcoded-credentials} + +Defaults live in `transport.py` and `service.py`. + +Costs: the credentials are in git history for anyone who gets the repo. +Revisit when: the repo leaves this machine, or MinIO is reachable outside the LAN. +Workaround: the environment variables already override them. Set them and remove the defaults. + +## Assemble marks a job completed with no clips {#empty-assemble} + +`run_stage_assemble` does not check that `clip_uris` is non-empty before assembling, then marks the job +completed. + +Costs: a failed chapter reports success. +Revisit when: any run reports completed without a video. One `if not clip_uris` guard fixes it. +Workaround: none. + +## Reviewer timestamps drift against the crossfaded video {#timeline-drift} + +`/review/panels` sums per-panel audio durations. Assemble crossfades clips using the per-beat +transitions, so every non-`cut` transition shortens the real video. + +Costs: reviewer timestamps drift further out of sync the further into the chapter they scrub. +Revisit when: the review UI is used for timing work. +Workaround: subtract the transition overlaps by hand. diff --git a/decisions/CLAUDE.md b/decisions/CLAUDE.md new file mode 100644 index 0000000..0dabe16 --- /dev/null +++ b/decisions/CLAUDE.md @@ -0,0 +1,29 @@ +# decisions/ + +One entry per settled question about this pipeline. An entry names the claim, the evidence, the date, +and what it forbids or permits next. + +No entry is a plan. `NEXT.md` holds the plan, `ROADMAP.md` holds the horizon, and a limitation that is +still live belongs in `caveats/`. + +## Rules for this directory + +* One file per topic, one `##` section per question, so the index can link an anchor. +* An entry cites a file and line, a commit, or a test. No citation means it is an opinion, and an + opinion gets deleted rather than defended. +* **Closed** means acting on it is safe. **Open** means investigated and undecided. **Void** means the + evidence turned out to be invalid and the claim must not be cited again. +* Rewrite an entry when something contradicts it, and say what changed. + +## Index + +| decision | state | +| --- | --- | +| [A dialogue row's speaker is `speaker_ref`, not `speaker`](audit-phase1.md#speaker-ref-is-canonical) | closed | +| [Script verification must not fail on sentence-initial capitals or short quotes](audit-phase1.md#verifier-false-positives) | closed | +| [Tracklet links are transitive over similarity, never across a hard constraint](audit-phase1.md#tracklet-hard-constraints) | closed | +| [A detected face takes an identity only when it falls inside that character's box](audit-phase1.md#gated-face-pairing) | closed | +| [A 409 from the GPU mutex is a queue signal, not a stale lease](audit-phase1.md#no-lease-stealing) | closed | +| [Correctness flags resolve by flag id, and never block autonomous TTS](audit-phase1.md#flag-resolution) | closed | +| [An out-of-range resolver index is `unresolved`, never a new character](audit-phase1.md#hallucinated-index) | closed | +| [The session manager holds no lock across a model load](audit-phase1.md#unlocked-model-load) | closed | diff --git a/decisions/audit-phase1.md b/decisions/audit-phase1.md new file mode 100644 index 0000000..b8fa5e8 --- /dev/null +++ b/decisions/audit-phase1.md @@ -0,0 +1,144 @@ +# Audit Phase 1: correctness and scheduling safety + +Settled 2026-08-11 from `AUDIT.md`. Every entry was verified by a runnable check in the same commit. +No GPU work ran and no pipeline run was executed. Each claim rests on source and on the CPU-only +self-checks named below. + +Files: `worker_scene.py`, `worker_script.py`, `worker_vision.py`, `session_manager.py` on workpc, and +`tracklets.py`, `correctness.py`, `db.py`, `service.py`, `session_proxy.py` in the homesrv orchestrator +(`/mnt/server/home/kami/docker-apps/manga-infra/orchestrator/`). + +## Speaker ref is canonical {#speaker-ref-is-canonical} + +**Closed.** A dialogue row's speaker is `speaker_ref` when its kind is `character_id`. The flat +`speaker` field is a compatibility value. The orchestrator already rewrote it from a panel-local id to +a `character_id`. + +Evidence: `correctness.py:normalize_dialogue` rewrites `row["speaker"]`. `worker_scene.build_scene` +mapped that value through `id_by_local`, whose keys are panel-local ids. Every lookup returned `None`, +so `worker_script._render_line` narrated every line as `Someone`. + +Forbids: reading `speaker` as a panel-local id anywhere downstream of the dialogue stage. +Check: `python worker_scene.py`, the `p007` case. + +The same field leaked into the dialogue resume path, where a stored `character_id` was pushed into +`recent` as a speaker name. It now renders the registry name (`service.py`, `run_stage_dialogue`). + +## Verifier false positives {#verifier-false-positives} + +**Closed.** `verify_script` must stay quiet on valid narration. A failure is not soft. The script stage +retries once and then raises, so one false positive halts the whole chapter. + +Two rules fired on correct output. + +1. `unsupported-proper-noun` flagged any capitalized token outside a 20-word stop list. That caught + ordinary sentence openers (`Suddenly`, `Behind`, `Inside`, `Both`, `Everyone`, `After`, `Two`) and + `Someone`. A capital opening a sentence is now grammar unless the word falls outside a real + narration vocabulary. A capital in the middle of a sentence still counts as name evidence. +2. `misquote` compared each quoted span against a WHOLE source line at ratio 0.82. The narration prompt + asks for a short quote of the actual words. A 15-character quote from a 40-character line scored + about 0.55. Grounding now matches the longest contiguous span instead. + +Forbids: adding a verifier rule without a test that a correct beat passes it. +Check: `pytest test_script_verify.py` in the orchestrator. + +Verifier feedback now reaches the retry. `worker_script.ScriptInput` declares `beat` and +`verifier_feedback`. Pydantic v2 dropped both silently before, so the retry was another blind sample. +The repair prompt names each failure and lists the exact quotable lines from the beat. + +## Tracklet hard constraints {#tracklet-hard-constraints} + +**Closed.** Tracklet linking is transitive over similarity evidence and never across a hard constraint. + +The hard constraints are gender and co-presence. Gender is canonicalized first. Vision emits +`m|f|unknown` while the registry says `male|female`, and the old gate tested only the second spelling, +so it never fired on real data. Co-presence means two detections in one panel are two people by +construction. A merge is rejected when any cross pair between the two groups violates either rule. +A weak chain can no longer bridge two people seen together. + +Forbids: comparing a raw gender string against a literal enum anywhere in the identity path. +Check: `python tracklets.py` and `pytest test_tracklets.py`. + +## Gated face pairing {#gated-face-pairing} + +**Closed.** A detected face takes a character's identity only when its centre falls inside that +character's gemma bbox. The box is grown by 25% first. An unpaired face stays `unknown`. + +This matters because the label becomes `speaker_method="som_face"`, the highest-trust provenance the +pipeline records. Unconditional nearest-neighbour pairing laundered a geometric guess into evidence. +Pairs are now taken globally shortest-first, so the first face processed cannot claim a character that +fits a later face better. + +Set-of-mark attribution stays ON by default now that the pairing is gated. The docstring said "off by +default" while the flag read `SOM_ATTRIBUTION` with default `"1"`. The code and the comment now agree. + +Forbids: trusting a `som_face` speaker without the containment gate. +Check: `python worker_vision.py`. + +## No lease stealing {#no-lease-stealing} + +**Closed.** A 409 from `/session/open` means another job legitimately holds the GPU. The proxy queues +on it. Only the session manager's TTL reaper clears a dead lease, because only it can tell a dead +lease from a busy one. + +`session_proxy.open_session` previously closed the active lease and retried, so two jobs terminated +each other's models. Job admission control now bounds concurrent pipelines through +`MAX_CONCURRENT_JOBS`, default 1. A lease queue alone still lets two pipelines interleave stages +against one GPU. + +`heartbeat_session` is finally called. `open_session` starts a beat task and `close_session` cancels +it, so a stage running past its TTL is no longer reaped mid-run. Beating from open and close keeps all +nine stage call sites unchanged. + +Forbids: closing a session the caller did not open. + +## Flag resolution {#flag-resolution} + +**Closed.** Correctness flags resolve by flag id, and unresolved flags never wedge an autonomous run. + +Two defects combined into a permanent block. `get_correctness_flags` accepted rows whose panel no +longer existed. A flag raised against a panel that a crop rerun deleted became visible to every +chapter and could never be resolved. `resolve_correctness_flags` could only touch flags whose +panel still existed. The view no longer admits orphans, and resolution takes explicit flag ids. + +The TTS block also had no reachable exit. It refused to start on four flag kinds that only +`/review/approve` clears, and `GATES` defaults to off. With gates on, TTS now returns rather than +raises, so `awaiting_review` survives instead of being overwritten by `failed` in the pipeline's +catch-all. With gates off, the flags are logged and cleared, because no reviewer exists to clear them. + +Forbids: raising out of a stage that has just set `awaiting_review`. +Check: `pytest test_name_binding.py` in the orchestrator. + +## Hallucinated index {#hallucinated-index} + +**Closed.** An out-of-range `choice` from `/vision/resolve` is `unresolved`, like a parse failure. Only +an explicit `0` means NONE and mints a new character. Mapping a bad index to NONE created a brand new +entry in the permanent registry from a hallucination. + +Check: `python worker_vision.py`. + +## Unlocked model load {#unlocked-model-load} [#203] + +**Closed.** The session manager never holds `_lock` across a model load or a health wait. + +`open_session` was already written that way, but it could orphan the server it spawned. A +`/session/close` arriving during the load found `proc = None`, tore down nothing, and cleared +`_active`. The finished server then held its VRAM unreferenced, and the next open spawned a second one +on the same port. Open now tears down its own process and reports 409 when its lease vanished mid-load. + +`_supervise_once` had the opposite asymmetry. It called `_start_subprocess` while holding `_lock`, +which blocked `/session/active`, `/session/close`, and `/session/open` for the full health wait. It now +claims the respawn by clearing `proc`, spawns unlocked, and tears the new process down if the lease +disappeared meanwhile. + +Check: `python session_manager.py`. + +## Related + +`_extract_json` in both `worker_vision.py` and `worker_script.py` now uses +`json.JSONDecoder().raw_decode` from the first brace. The greedy `\{.*\}` ran to the LAST brace in the +reply. A second object or trailing braced prose burned a repair call on a response that parsed fine. + +`worker_scene.build_scene` emits `actions` as a list beside the joined `action` string, because +`correctness.build_beat_artifact` reads the plural key. The verifier was receiving no action evidence +at all. The beat builder also falls back to splitting the singular string. diff --git a/session_manager.py b/session_manager.py index 6d20ca8..da3e9fc 100644 --- a/session_manager.py +++ b/session_manager.py @@ -116,7 +116,12 @@ def open_session(req: OpenReq): with _lock: if _active is not None and _active["session_id"] == session_id: _active["proc"] = proc - return {"session_id": session_id, "port": cfg.get("port")} + return {"session_id": session_id, "port": cfg.get("port")} + # The lease was closed or reaped while we were spawning (close saw proc=None and tore down + # nothing). The server we just started is unreferenced and would hold its VRAM until someone + # killed it by hand, and the next open would spawn a SECOND one on the same port. + _teardown({"proc": proc}) + raise HTTPException(409, "session closed while the model was loading") @app.post("/session/close") @@ -162,8 +167,8 @@ def _supervise_once(): talks to the port directly, so if we don't bring it back every remaining panel of the stage fails with 'connection refused'. Restarting under the SAME session keeps the lease + port valid so the orchestrator never has to re-open. Returns True if a respawn happened.""" + global _active with _lock: - global _active sess = _active if not sess: return False @@ -174,16 +179,29 @@ def _supervise_once(): proc.wait(timeout=1) # reap the zombie except subprocess.TimeoutExpired: pass - print(f"[supervise] gemma subprocess died (rc={proc.returncode}) session {sess['session_id']}; " - f"respawning", flush=True) - try: - sess["proc"] = _start_subprocess(MODELS[sess["model"]]) - sess["last_beat"] = time.time() # don't count the downtime against the TTL reaper + # claim the respawn before releasing the lock: proc=None makes the next _supervise_once pass + # return early, so the health wait can't be entered twice for one death. + sess["proc"] = None + session_id, model = sess["session_id"], sess["model"] + # ponytail: spawn + health-wait UNLOCKED, like open_session. Holding _lock here blocked + # /session/active, /close and /open for the full health wait (up to 300s). + print(f"[supervise] gemma subprocess died (rc={proc.returncode}) session {session_id}; " + f"respawning", flush=True) + try: + new_proc = _start_subprocess(MODELS[model]) + except Exception as e: + print(f"[supervise] respawn failed: {e}; clearing session so the mutex frees", flush=True) + with _lock: + if _active is not None and _active["session_id"] == session_id: + _active = None + return False + with _lock: + if _active is not None and _active["session_id"] == session_id: + _active["proc"] = new_proc + _active["last_beat"] = time.time() # don't count the downtime against the TTL reaper return True - except Exception as e: - print(f"[supervise] respawn failed: {e}; clearing session so the mutex frees", flush=True) - _active = None - return False + _teardown({"proc": new_proc}) # lease went away mid-respawn; don't orphan the server + return False def _reaper(): @@ -229,6 +247,24 @@ if __name__ == "__main__": close_session(SessionReq(session_id=s4["session_id"])) assert active() is None + # a close landing DURING the spawn must not orphan the server: open reports 409 and tears it down. + MODELS["gemma4"]["binary"] = "/bin/sleep" + MODELS["gemma4"]["port"] = None + real_start = _start_subprocess + + def racing_start(cfg): + p = real_start(cfg) + close_session(SessionReq(session_id=_active["session_id"])) # close mid-load + return p + _start_subprocess = racing_start + try: + open_session(OpenReq(model="gemma4", ttl=3600)) + assert False, "open must 409 when its lease vanished mid-load" + except HTTPException as e: + assert e.status_code == 409 + _start_subprocess = real_start + assert active() is None + # in-process model: close must POST /unload to the worker so its resident VRAM is freed. calls = [] requests.post = lambda url, **kw: calls.append(url) or type("R", (), {"status_code": 200})() diff --git a/worker_scene.py b/worker_scene.py index 5829581..11f0110 100644 --- a/worker_scene.py +++ b/worker_scene.py @@ -70,20 +70,32 @@ def build_scene(data: SceneInput): # Speaker attribution is the vision/dialogue model's job now (it reads bubble tails + turn-taking). # speaker is a local_id -> map to character_id via identity; None for narration/sfx/off-panel. + # The orchestrator's normalize_dialogue already rewrote `speaker` to a character_id and put the + # typed answer in `speaker_ref`, so the local-id map no longer matches it. Read speaker_ref FIRST; + # every lookup used to miss and every line narrated as "Someone". dialogue = [] for d in data.vision_result.get("dialogue", []): raw = d.get("speaker") + ref = d.get("speaker_ref") or {} # "unknown" is a DELIBERATE off-panel/indeterminate speaker (narrated as "someone"); it maps # to None just like narration. id_by_local.get already yields None for it. - dialogue.append({"speaker": id_by_local.get(raw), "text": d.get("text", ""), + if ref.get("kind") == "character_id" and ref.get("value"): + cid = ref["value"] + else: + cid = id_by_local.get(raw) or (raw if raw in name_by_id else None) + dialogue.append({"speaker": cid, "text": d.get("text", ""), "type": d.get("type", "speech"), "confidence": d.get("confidence"), "speaker_method": d.get("speaker_method", "unknown")}) vchars = data.vision_result.get("characters", []) - action = "; ".join(c.get("action", "") for c in vchars if c.get("action")) or "" + # `actions` is the list the orchestrator's beat builder reads for verifier evidence; `action` is the + # joined string the script prompt renders. Emitting only the string left verification blind. + actions = [c["action"].strip() for c in vchars if str(c.get("action") or "").strip()] + action = "; ".join(actions) return {"panel_id": data.panel_id, "characters": characters, "dialogue": dialogue, - "action": action, "entities": data.vision_result.get("entities", []), + "action": action, "actions": actions, + "entities": data.vision_result.get("entities", []), "camera": data.vision_result.get("camera", {}), # #8 direction passthrough -> render "transition": data.vision_result.get("transition", "cut")} # #6 transition out @@ -167,4 +179,21 @@ if __name__ == "__main__": )) assert out5["dialogue"][0]["confidence"] == 0.3 assert out5["dialogue"][0]["speaker_method"] == "turn_taking" + + # orchestrator-normalized rows: `speaker` is already a character_id and `speaker_ref` is typed. + # the local-id map cannot resolve either — speaker_ref must win, or everything narrates as Someone. + out6 = build_scene(SceneInput( + panel_id="p007", + vision_result={"characters": [{"local_id": "person_1", "bbox": [0, 0, 10, 10]}], + "dialogue": [{"speaker": "c1", "speaker_ref": {"kind": "character_id", "value": "c1"}, + "type": "speech", "text": "Hey."}, + {"speaker": None, "speaker_ref": {"kind": "unknown", "value": None}, + "type": "speech", "text": "..."}]}, + identity_assignments=[{"local_id": "person_1", "character_id": "c1"}], + characters_registry=[{"character_id": "c1", "name": "Teto"}], + )) + assert out6["dialogue"][0]["speaker"] == "c1", out6["dialogue"][0] + assert out6["dialogue"][1]["speaker"] is None, out6["dialogue"][1] + # actions survive as a list for the verifier, not only as the joined prompt string + assert out["actions"] == ["waving"] and out["action"] == "waving" print("worker_scene self-check ok") diff --git a/worker_script.py b/worker_script.py index 888f2a9..31522de 100644 --- a/worker_script.py +++ b/worker_script.py @@ -21,6 +21,10 @@ class ScriptInput(BaseModel): recent: list = [] # #1: last few panels' narration text (local flow, no repeats) introduced: list = [] # #2: names already narrated -> refer by name/pronoun, don't re-describe panel_count: int = 1 # scene-level narration: #panels in this beat -> scales the length budget + # the orchestrator has been sending both of these on a failed-script retry since the verifier + # landed; pydantic dropped them silently, so the retry was another blind roll of the dice. + beat: dict = {} # {beat_id, panels:[{panel_id, dialogue:[{text}], actions}]} — the evidence + verifier_feedback: list = [] # [{kind, values|quote}] from verify_script on the previous attempt class SummaryInput(BaseModel): @@ -80,9 +84,33 @@ def _chars_line(name_by_id, genders_by_id): return ", ".join(out) or "none" +def _feedback_block(feedback, beat) -> str: + """Render the previous attempt's verifier failures as concrete corrections. Without this the + retry is just another sample at the same temperature — the model never learns what was wrong.""" + if not feedback: + return "" + lines = [] + for f in feedback: + kind = f.get("kind") + if kind == "unsupported-proper-noun": + lines.append("- You used name(s) that are not in this beat: " + + ", ".join(str(v) for v in f.get("values", [])) + + ". Use only the names listed under 'Characters present', or a pronoun.") + elif kind == "misquote": + lines.append(f'- Your quote "{f.get("quote","")}" is not what the panel says. ' + "Quote the exact words below, or drop the quotation marks.") + else: + lines.append(f"- {kind}: {json.dumps(f, ensure_ascii=False)[:200]}") + quotes = [d.get("text", "") for p in (beat or {}).get("panels", []) + for d in p.get("dialogue", []) if d.get("text")] + exact = ("Exact lines you may quote from:\n" + "\n".join(f'- "{q}"' for q in quotes) + "\n") if quotes else "" + return ("Your previous attempt was REJECTED. Fix exactly these problems and rewrite it:\n" + + "\n".join(lines) + "\n" + exact + "\n") + + def build_prompt(sg: dict, chapter_context: str, names_by_id=None, brief: str = "", recent=None, introduced=None, genders_by_id=None, - panel_count: int = 1) -> str: + panel_count: int = 1, beat=None, verifier_feedback=None) -> str: name_by_id = _name_map(sg.get("characters", []), names_by_id) chars = _chars_line(name_by_id, genders_by_id) dialogue = "\n".join(_render_line(d, name_by_id) for d in sg.get("dialogue", [])) or "none" @@ -120,7 +148,7 @@ def build_prompt(sg: dict, chapter_context: str, names_by_id=None, else: unit, budget, thin = "this manga panel", "1-2 short sentences, max ~25 words", "panel" return ( - ov + rec + ctx + intro + + _feedback_block(verifier_feedback, beat) + ov + rec + ctx + intro + f"Write TIGHT recap narration for {unit}: {budget}, " "present tense. Tell it like you're recapping to a friend — natural, with momentum. Output " "only the narration — no preamble, no notes, no alternatives, never ask for more info. If the " @@ -182,7 +210,7 @@ async def summary(data: SummaryInput): async def script(data: ScriptInput): text = call_gemma4(build_prompt(data.scene_graph, data.chapter_context, data.names_by_id, data.brief, data.recent, data.introduced, data.genders_by_id, - data.panel_count)) + data.panel_count, data.beat, data.verifier_feedback)) return {"panel_id": data.panel_id, "text": text} @@ -199,10 +227,16 @@ class NormalizeInput(BaseModel): def _extract_json(raw: str) -> dict: text = _strip_thought(raw) - m = re.search(r"\{.*\}", text, re.DOTALL) - if not m: + i = text.find("{") + if i < 0: raise ValueError(f"no json: {text[:200]}") - return json.loads(m.group(0)) + # raw_decode stops at the end of the FIRST object. The old greedy `\{.*\}` swallowed any trailing + # brace in prose after it and turned a parseable reply into a wasted repair call. + try: + obj, _ = json.JSONDecoder().raw_decode(text[i:]) + except json.JSONDecodeError as e: + raise ValueError(f"bad json: {text[:200]}") from e + return obj def build_normalize_prompt(names, entities) -> str: @@ -297,4 +331,12 @@ if __name__ == "__main__": nr = _extract_json('{"entities":[{"name":"Everyday","kind":"shop"}],' '"name_fixes":{"CHOI HAESEON":"Choi Haeseon"}}') assert nr["entities"][0]["name"] == "Everyday" and nr["name_fixes"]["CHOI HAESEON"] == "Choi Haeseon" + # trailing braced prose after the object must not break the parse (was a wasted repair call) + assert _extract_json('{"a":1}\nnote: {not json}')["a"] == 1 + # verifier feedback reaches the retry prompt with the exact allowed quotes; absent by default + assert "REJECTED" not in p + beat = {"beat_id": "b1", "panels": [{"panel_id": "p1", "dialogue": [{"text": "stand proud"}]}]} + pf = build_prompt(sg, "s", beat=beat, verifier_feedback=[ + {"kind": "unsupported-proper-noun", "values": ["Kyoto"]}, {"kind": "misquote", "quote": "be proud"}]) + assert "REJECTED" in pf and "Kyoto" in pf and "be proud" in pf and '- "stand proud"' in pf print("worker_script self-check ok") diff --git a/worker_vision.py b/worker_vision.py index 698897b..68ad8df 100644 --- a/worker_vision.py +++ b/worker_vision.py @@ -19,7 +19,9 @@ GEMMA4_URL = os.environ.get("GEMMA4_URL", "http://127.0.0.1:8090") # set-of-mark speaker attribution: detect text regions with comic-text-detector (ONNX, CPU) and draw # numbered boxes on the panel so gemma transcribes/attributes grounded regions instead of eyeballing -# the whole image. Off by default — flip SOM_ATTRIBUTION=1 once tuned per title (see bubble_detect.py). +# the whole image. ON by default: face->identity pairing is now gated on containment, so an unmatched +# face is labelled "unknown" instead of borrowing the nearest name (see _pair_faces_to_present). +# SOM_ATTRIBUTION=0 falls back to the holistic path (see bubble_detect.py). try: import bubble_detect except Exception: # onnxruntime/model absent -> feature simply stays unavailable @@ -31,29 +33,45 @@ except Exception: SOM = os.environ.get("SOM_ATTRIBUTION", "1") == "1" -def _pair_faces_to_present(det_faces: list, present: list) -> list: +def _pair_faces_to_present(det_faces: list, present: list, margin: float = 0.25) -> list: """det_faces = real detector boxes (grounded but identity-less); present = characters gemma placed - in the panel (identity + a coarse, imprecise bbox). Assign each detected face the identity of the - present character whose gemma-bbox centre falls closest to it, so a green box carries a real name. - A face with no nearby present char stays unknown; a present char is used at most once.""" + in the panel (identity + a coarse, imprecise bbox). Pair a face with a present character ONLY when + the face centre falls inside that character's bbox grown by `margin` of its size — gemma's boxes are + imprecise but not arbitrary. An unpaired face stays unknown instead of borrowing whoever happens to + be nearest; the label it carries becomes `speaker_method="som_face"`, the highest-trust provenance + the pipeline has, so an ungated guess used to launder itself into evidence. + Pairs are taken globally shortest-first, so the first face processed cannot claim a character that + fits a later face far better. Each face and each character is used at most once.""" def cx_cy(b): return ((b[0] + b[2]) / 2, (b[1] + b[3]) / 2) - used = set() - out = [] - for i, f in enumerate(det_faces, 1): + + def contains(face_box, char_box) -> bool: + w, h = char_box[2] - char_box[0], char_box[3] - char_box[1] + if w <= 0 or h <= 0: + return False + fx, fy = cx_cy(face_box) + return (char_box[0] - margin * w <= fx <= char_box[2] + margin * w + and char_box[1] - margin * h <= fy <= char_box[3] + margin * h) + + pairs = [] + for i, f in enumerate(det_faces): fx, fy = cx_cy(f["bbox"]) - best, bestd = None, 1e18 for j, c in enumerate(present): b = c.get("bbox") - if not b or j in used: + if not b or len(b) != 4 or not contains(f["bbox"], b): continue px, py = cx_cy(b) - d = (px - fx) ** 2 + (py - fy) ** 2 - if d < bestd: - best, bestd = j, d - c = present[best] if best is not None else {} - if best is not None: - used.add(best) - out.append({"label": f"P{i}", "bbox": f["bbox"], "local_id": c.get("local_id"), + pairs.append(((px - fx) ** 2 + (py - fy) ** 2, i, j)) + taken_f, taken_c, match = set(), set(), {} + for _, i, j in sorted(pairs): + if i in taken_f or j in taken_c: + continue + taken_f.add(i) + taken_c.add(j) + match[i] = j + out = [] + for i, f in enumerate(det_faces): + c = present[match[i]] if i in match else {} + out.append({"label": f"P{i + 1}", "bbox": f["bbox"], "local_id": c.get("local_id"), "who": c.get("name") or c.get("desc") or "unknown", "gender": c.get("gender")}) return out @@ -127,10 +145,17 @@ def _strip_thought(text: str) -> str: def _extract_json(raw: str) -> dict: """strip gemma4 thought, pull the first JSON object, parse it.""" text = _strip_thought(raw) - m = re.search(r"\{.*\}", text, re.DOTALL) - if not m: + i = text.find("{") + if i < 0: raise ValueError(f"no json in response: {text[:200]}") - return json.loads(m.group(0)) + # raw_decode stops at the end of the FIRST object. The old greedy `\{.*\}` ran to the LAST brace in + # the reply, so a second object or any trailing braced prose produced an unparseable span and burned + # a repair call on a response that was already fine. + try: + obj, _ = json.JSONDecoder().raw_decode(text[i:]) + except json.JSONDecodeError as e: + raise ValueError(f"bad json in response: {text[:200]}") from e + return obj def _img_part(image_path: str) -> dict: @@ -893,10 +918,18 @@ async def vision_resolve(data: ResolveInput): os.remove(crop) for _, path in refs: os.remove(path) - # map gemma's 1-based choice back to a character_id; 0 / out-of-range -> NONE (new character). + # map gemma's 1-based choice back to a character_id. An explicit 0 means NONE -> a new character. + # An OUT-OF-RANGE index is a hallucination, not an answer: it must be `unresolved` like a parse + # failure, or a bad index silently mints a brand new character in the permanent registry. choice = result.get("choice", 0) - cid = data.candidates[choice - 1]["character_id"] if isinstance(choice, int) and 1 <= choice <= len(data.candidates) else None - state = "known" if cid else ("unresolved" if result.get("reason") == "parse_failed" else "new") + in_range = isinstance(choice, int) and 1 <= choice <= len(data.candidates) + cid = data.candidates[choice - 1]["character_id"] if in_range else None + if in_range: + state = "known" + elif choice == 0 and result.get("reason") != "parse_failed": + state = "new" + else: + state = "unresolved" return {"character_id": cid, "state": state, "confidence": float(result.get("confidence", 0.0)), "reason": result.get("reason", "")} @@ -1005,4 +1038,20 @@ if __name__ == "__main__": assert False except ValueError: pass + # trailing braced prose after a complete object parses (used to burn a repair call) + assert _extract_json('{"skip":false}\nnote {see above}')["skip"] is False + + # face->identity pairing is GATED on containment: a face outside every gemma bbox stays unknown. + faces = [{"bbox": [10, 10, 30, 30]}, {"bbox": [900, 900, 920, 920]}] + present = [{"local_id": "person_1", "name": "Teto", "bbox": [0, 0, 100, 200]}] + paired = _pair_faces_to_present(faces, present) + assert paired[0]["local_id"] == "person_1" and paired[0]["who"] == "Teto" + assert paired[1]["local_id"] is None and paired[1]["who"] == "unknown", paired[1] + # globally shortest-first: the first face must not claim a character that fits the second better. + faces2 = [{"bbox": [95, 95, 105, 105]}, {"bbox": [8, 8, 12, 12]}] + present2 = [{"local_id": "a", "bbox": [0, 0, 20, 20]}, {"local_id": "b", "bbox": [80, 80, 120, 120]}] + p2 = _pair_faces_to_present(faces2, present2) + assert [f["local_id"] for f in p2] == ["b", "a"], p2 + # no present characters at all -> every face unknown, never a phantom identity + assert _pair_faces_to_present(faces, [])[0]["local_id"] is None print("worker_vision self-check ok")