fix(media-use): repoint the dead videogen tier, demote past unusable local models - #3509
Conversation
…models
`LOCAL_MODELS.videogen`'s `large` tier named `dgrauet/ltx-2.3-mlx-bf16`, which
returns HTTP 401 and cannot be downloaded at all. It was not a dormant entry:
`rankedByPreference` sorts by descending `needs.ramMB` when no `rank` is set,
so the largest fitting tier is tried FIRST by design. Any machine clearing
32 GB *available* RAM selected the dead entry, `ltxVideoGenerate` caught the
failure and returned a bare `null`, and since `ltx.local` is last in
`["heygen.video", "ltx.local"]` and network providers are skipped under
`--local-only` (`registry.mjs:206`), local video generation failed outright
instead of falling back to the tier that works.
It survived review because the table landed with "live verification on a 24GB
M-series Mac" - and a 24 GB machine cannot select a 32 GB tier, so that entry
was unreachable on the only machine that validated it. The unit fixtures
inherit the same ceiling (`fittingSpecs` is 20000MB), so every existing test
exercised the medium tier alone.
Two changes:
1. Repoint to `dgrauet/ltx-2.3-mlx-q8` (reachable) and correct `sizeMB` from
45000 to 28800. Measured against the HF API: the q8 repo totals 87.5 GB,
and the registry's own targeted `--include` subset is 28.76 GB. That
matches the sibling q4 entry's convention (`sizeMB: 20000` vs a measured
19.48 GB subset), so 45000 was wrong under either reading. `--low-ram` is
added because the entry's own note calls it required at this tier's 32 GB
floor, and the invoke omitted it.
2. A repoint alone is one bad URL from a repeat, so add the missing recovery.
`selectModelLadder` returns every fitting model best-first;
`selectModel`'s pick is now defined as that list's head. All three sites
that previously selected exactly one model and failed terminally walk the
ladder instead, demoting past a tier that cannot run here - gated weights,
runner off PATH, an OOM at a tier that nominally fits:
- `ltx-video-provider.mjs` (videogen, the reported failure)
- `mflux-provider.mjs` (imagegen - same shape, and its 32 GB/64 GB tiers
are equally unverifiable on a 24 GB machine)
- `local-run.mjs` (tts/asr/upscale - `fish-speech` missing should still
get you Kokoro)
Every demotion is logged rather than silent, so a quietly smaller model is
never mistaken for the tier the machine nominally qualified for.
Also fixes the `install` string both videogen entries share: it ended at
`uv sync --all-extras`, which leaves the entry point in `.venv/bin`, so the
"`ltx-2-mlx` not on PATH" hint named a command that following the instruction
would not put on PATH.
The q8 tier is NOT live-verified - no 32 GB+ Apple Silicon machine was
available - and its notes say so. Shipping it unverified is safe precisely
because of change 2: a wrong tier now costs one failed attempt, not the whole
local path.
- Rames Jusso
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head review at 90df164a22089aa0c6cefdc7fc00ffdee54853a6.
The ladder construction is clean: selectModel() is now defined from the same best-first list the retrying providers consume, and the 40 GB fixtures finally exercise the previously unreachable large rung (local-models.mjs:269-284, ltx-video-provider.test.mjs:102-226). The preferTier pin also preserves an explicit no-demotion escape hatch.
Blockers
-
skills/media-use/scripts/lib/local-models.mjs:137-147—sizeMB: 28800and the note do not describe the path this entry actually runs. The invoke passes the repo ID directly toltx-2-mlx; upstreamresolve_model_dir()calls unfilteredsnapshot_download(repo). The current q8 repository is 87,511,991,375 bytes, so a qualifying machine downloads roughly 87.5 GB, not a 28.8 GB targeted subset. More importantly,--two-stage --low-ramneeds bothtransformer-devandtransformer-distilledplus the x2 spatial upscaler; the 28.8 GB distilled-only subset described above the table is not sufficient for this invocation. Please either actually pre-resolve the exact two-stage artifact set to a local path and record that measured size, or report the full snapshot size/runtime behavior. -
skills/media-use/scripts/lib/ltx-video-provider.mjs:40-66andskills/media-use/scripts/lib/mflux-provider.mjs:59-100— the new retry boundary allocates a temp output per attempt, but every failurecontinueabandons that path. Generators can create a partial MP4/PNG before exiting nonzero (OOM/timeout is one of the advertised demotion cases); the lower tier can then succeed while the failed artifact remains hidden in the temp directory. Remove failed-attempt outputs before continuing and pin the partial-file case. The LTX path is especially material because a partial video can be large.
Local evidence: 35 focused tests passed; the full skills suite reproduced 538/538; git diff --check passed. I also verified the q8 repo and the runner flags against their current primary upstream sources.
Verdict: REQUEST CHANGES
Reasoning: The fallback mechanics and tests are strong, but the shipped large-tier download contract is currently off by tens of gigabytes and the new retry loops leak partial outputs on their intended failure paths.
— Magi
…discard failed partials Addresses review feedback on #3509 (CHANGES_REQUESTED at 90df164), plus the follow-on ask to tell the user what a download costs before they accept it. 1. `sizeMB` described a targeted `--include` subset that no run ever gets. Both videogen invokes pass a repo id to `--model`, and upstream `resolve_model_dir()` (`ltx_pipelines_mlx/utils/_orchestration.py:35-40`) calls `snapshot_download(repo)` with no `allow_patterns`, so the full repo lands regardless of what was pre-fetched. Corrected to measured repo totals: q8 87500 (87,511,991,375 B) and q4 59700 (59,686,429,583 B). q4 was wrong the same way at 20000, so both are fixed together rather than leaving one convention on each side. My earlier claim that 28800 "matches the sibling q4 entry's convention" was wrong in the way that matters: the convention itself described a subset the runner does not honor. The file's own comment already said "blind snapshot-downloads the lot (60 GB q4, 88 GB q8)" three lines above the fields that contradicted it, and the original report measured it too ("the q4 cache ended at 56 GB and q8 at 82 GB"), which reconciles exactly once read as GiB: 59.69 GB = 55.6 GiB, 87.51 GB = 81.5 GiB. So the download is the complete repo both times, not a partial fetch. Removed the `--include` recipe rather than repairing it: it is ineffective (the runner refetches at generate time) and insufficient (`--two-stage` is "dev model + CFG at half-res, upscale, distilled LoRA refine" per upstream's own help text, so it needs transformer-dev AND transformer-distilled AND spatial_upscaler_x2; `--distilled` needs an upscaler too). The q4 tier verified on a 24 GB Mac only worked BECAUSE the download is unfiltered. 2. Nothing told the user what they were agreeing to before a tool started pulling tens of GB. `describeDownload()` in `specs.mjs` names the size and the directory the weights land in, and checks free space with `statfs` against that directory rather than cwd, since the weights do not land in cwd. A tier that will not fit is still offered, with a plain statement that it will not fit: hiding it would make a machine that could free up space look like it has no large tier. Unknown free space reports as unknown, not as zero. Wired into both providers' install hints, the `runLocalModel` install payload (now carrying `sizeMB`), and `describeModelLadder`. 3. Each retry attempt mints its own timestamped temp path, so a partial artifact from a failed tier was orphaned rather than overwritten, and a lower tier then succeeding hid it. Both providers discard the partial before demoting, guarded so a file that cannot be removed never masks the generate failure it came from. Video is the material case: a partial mp4 is large. `local-run.mjs` is deliberately unchanged here. Its `out` is caller-provided and identical across attempts, so a partial is overwritten rather than orphaned, and unlinking a path the caller named would be a footgun. The rule the two providers follow is: clean up what you allocate. Tests: 553/553 across `skills/**/*.test.mjs` (+15). New coverage pins the cleanup (failed tier's partial removed, returned artifact survives, one discard per attempt on the all-fail path, an unremovable partial still surfaces the real failure) and the disclosure (cache-dir precedence, statfs walk-up to the deepest existing ancestor, unknown-vs-zero, and the will-not-fit wording). Every new guard mutation-tested: removing any one of them turns tests red. - Rames Jusso Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed blocker-fix head 6195d5a7f2caf65aa8eadbe864d66ddf4b436a22.
Both original blockers are closed:
skills/media-use/scripts/lib/local-models.mjs:117-150now records the actual unfiltered snapshot contract: q4 59.7 GB and q8 87.5 GB, with the ineffective/insufficient targeted-subset recipe removed. The two-stage note correctly names dev + distilled + x2 upscaler, and the ladder/install payload now exposessizeMB.ltx-video-provider.mjs:22-33,67andmflux-provider.mjs:41-52,101remove failed-attempt partials before demotion. The cleanup is best-effort, never touches a successful returned artifact, and is pinned for fail-then-succeed, all-fail, success, and unlink-failure paths.
The added download disclosure is also coherent: Hugging Face cache precedence is modeled explicitly, statfs walks to the deepest existing ancestor, and insufficient/unknown free space is reported without silently removing a tier.
Local verification reproduced 56 focused tests and the complete skills suite at 553/553; git diff --check passed. GitHub reports no failing or pending checks at this head.
No remaining blocker.
Verdict: APPROVE
Reasoning: The model-size contract now matches the actual runtime download, retry cleanup owns every failed temp artifact, and the new disclosure/free-space behavior is bounded and tested.
— Magi
What
Three things, in the local-model registry behind the
media-useskill:LOCAL_MODELS.videogen'slargetier pointed atdgrauet/ltx-2.3-mlx-bf16, which returns HTTP 401 and cannot be downloaded. Repointed todgrauet/ltx-2.3-mlx-q8.sizeMBvalues were describing a download that never happens, and nothing told the user what a pull would cost before it started. Corrected to measured repo totals, and the size is now disclosed at the moment someone decides to install, with a free-space check.Why
The dead entry was not dormant.
rankedByPreferencesorts by descendingneeds.ramMBwhen norankis set, so the largest fitting tier is tried first, by design. On any machine clearing 32 GB available RAM (meetsSpecsgates onavailableRamMB ?? ramMB) selection landed on the 401'ing entry,ltxVideoGeneratecaught the failure and returned a barenull, and becauseltx.localis last in["heygen.video", "ltx.local"]and network providers are skipped under--local-only(registry.mjs:206), local video generation failed outright — with the working q4 tier sitting right below it, never tried.Worth recording why it survived review: the table landed with "live verification on a 24GB M-series Mac", and a 24 GB machine cannot select a 32 GB tier. The entry was structurally unreachable on the only machine that validated it. The unit fixtures inherit the same ceiling —
fittingSpecsis20000MB — so every existing test inltx-video-provider.test.mjsexercised the medium tier alone. When a table's entries are gated on the verifier's own hardware, everything above their ceiling is untested no matter how careful the pass was.How
The repoint, and the size numbers
sizeMBdescribed a targetedhf download --includesubset. No run ever gets that subset. Both videogen invokes pass a bare repo id to--model, and upstreamresolve_model_dir()(ltx_pipelines_mlx/utils/_orchestration.py:35-40) is:No
allow_patterns, noignore_patterns— the whole repo lands regardless of what was pre-fetched. Corrected to the measured totals (?blobs=true, every blob summed):ltx-2.3-mlx-q845000, then2880087500ltx-2.3-mlx-q42000059700A correction to my own earlier reasoning in this PR. I justified
28800as matching "the sibling q4 entry's convention". That was wrong in the way that matters: the convention itself described a subset the runner does not honor, so q4's20000was wrong too. Both are fixed together rather than leaving one convention on each side.Two independent confirmations that the download is the complete repo:
dulabels GiB asG): 59.69 GB = 55.6 GiB, 87.51 GB = 81.5 GiB. So it is not "fetches what it needs" — it fetches everything, both times.The
--includerecipe is removed rather than repaired, because it is both ineffective and insufficient. Insufficient because--two-stageis "dev model + CFG at half-res, upscale, distilled LoRA refine" (upstream's owncli.py:393help text), so it needstransformer-devandtransformer-distilledandspatial_upscaler_x2; the documented distilled-only subset has none of the first and neither of the last two.--distilledneeds an upscaler too. The q4 tier that was live-verified on a 24 GB Mac only worked because the download is unfiltered.I deliberately did not take the other available route — making
invokepre-resolve a filtered artifact set to a local path. Getting that include-list right requires validating it on a 32 GB+ Mac I do not have, which is the exact failure this PR exists to fix. It also saves less than it appears: two-stage needs ~59 of the 87.5 GB, and the slack is duplicate variants (transformer-distilledvs-1.1, two 7.61 GB LoRA copies).Telling the user what a download costs
describeDownload(sizeMB)inspecs.mjsnames the size and the directory the weights land in, and checks free space withstatfsagainst that directory —weightsCacheDir()followshuggingface_hub's own precedence (HF_HUB_CACHE→HUGGINGFACE_HUB_CACHE→HF_HOME/hub→~/.cache/huggingface/hub), because a check against cwd measures the wrong filesystem. The cache dir usually does not exist until the first download andstatfsthrows on a missing path, so it walks up to the deepest existing ancestor.Three deliberate choices:
Wired into both providers' install hints (the actual accept moment a user reads), the
runLocalModelinstall payload (now carryingsizeMB), anddescribeModelLadder.On this box the large tier correctly reads:
The demotion, and cleaning up after it
selectModelLadder(capability, specs, { preferTier })returns every fitting model best-first;selectModel's pick is now defined as that list's head, so the existing single-select behaviour is unchanged by construction. Three call sites previously selected exactly one model and failed terminally — all three walk the ladder now:ltx-video-provider.mjsmflux-provider.mjslocal-run.mjsfish-speechmissing cost you Kokoro; a brokenseedvr2cost youreal-esrganEvery demotion is logged, not silent — a quietly smaller model would otherwise be indistinguishable from the tier the machine nominally qualified for.
Because each attempt mints its own timestamped temp path, a partial artifact from a failed tier was orphaned rather than overwritten, and a lower tier then succeeding hid it. Both providers now discard the partial before demoting, guarded so a file that cannot be removed never masks the generate failure it came from. Video is the material case: a partial mp4 is large.
local-run.mjsis deliberately excluded from that cleanup. Itsoutis caller-provided and identical across attempts, so a partial is overwritten rather than orphaned, and unlinking a path the caller named would be a footgun. The rule the two providers follow is: clean up what you allocate.Also fixed: the
installstring both videogen entries share ended atuv sync --all-extras, which leaves the entry point in.venv/bin. The "ltx-2-mlxnot on PATH" hint was naming a command that following its own instruction would not put on PATH.Test plan
553/553 across
skills/**/*.test.mjs(72 files, matching CI's ownfind-based enumeration).Added, beyond the ladder/demotion coverage:
specs.test.mjs— cache-dir precedence, thestatfswalk-up to the deepest existing ancestor, unknown-vs-zero, and the will-not-fit wording.ltx-video-provider.test.mjs/mflux-provider.test.mjs— the failed tier's partial is removed while the returned artifact survives; one discard per attempt on the all-fail path; an unremovable partial still surfaces the real failure; each install hint states that tier's download cost.local-run.test.mjs— the install recommendation carriessizeMBand states the download size.local-models.test.mjs— a guard that aninvokenaming anowner/repomodel agrees with the entryid, so a half-done repoint cannot select one model and download another.Mutation-tested rather than trusted green. Every new guard was removed in turn and the suite confirmed red:
discardPartialremoved from the LTX catchdiscardPartialremoved from the mflux catchdescribeDownloadfreeSpaceMBancestor walk-up removedsizeMBstripped from the install payloadq8, invokebf16)continuereplaced withreturn nullmeetsSpecsdropped from the ladder filterselectModeltestsGates run locally:
oxfmt --checkclean (733 files),oxlint0 warnings / 0 errors,git diff --checkclean,check-tracked-artifactspassed,check-skill-mirrorOK (24 files byte-for-byte),lint-skills32 files clean.skills-manifest.jsonis the pre-commit hook's regeneration of the media-use hash.A correction to this PR's earlier test plan: it claimed
lint-registry-itemswas "372 items clean". That was wrong — it reports 26 pre-existing errors onmain(font-face declarations incompositions/). They are outside this diff, which touches zero composition or registry files, and the script is referenced by no workflow and is not in thelintchain, so it is not a CI gate. Flagging it rather than restating a claim that does not hold.Not verified — flagged deliberately: the q8 tier has had no live run; no 32 GB+ Apple Silicon machine was available, and its
notessay so in the table. Shipping it unverified is safe precisely because of change 2 — a wrong tier now costs one failed attempt instead of the whole local path. A reviewer with a 32 GB+ Mac can confirm the--two-stage --low-ramcombination.A product question I did not decide: an 87.5 GB large tier against q4's 59.7 GB is a real tradeoff, not a rounding difference. Whether that tier is worth offering at all is a product call, not a bug fix, so this PR makes the cost honest and visible and leaves the decision open.
Known follow-up, deliberately not in this PR:
resolve.mjs:395buildsctxfrom six fields, soctx.width/ctx.height/ctx.frames/ctx.preferTierare all unreachable — every local video is 512x320 x 33 frames, and the manual tier override is dead. That is a separate change to the resolve surface and gets its own PR.