Skip to content

fix: hash UNet engine cache key so ONNX export path stays under Windows MAX_PATH#10

Merged
forkni merged 2 commits into
SDTD_032_devfrom
integ/maxpath-to-032
Jul 19, 2026
Merged

fix: hash UNet engine cache key so ONNX export path stays under Windows MAX_PATH#10
forkni merged 2 commits into
SDTD_032_devfrom
integ/maxpath-to-032

Conversation

@forkni

@forkni forkni commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Summary

Cherry-picks 153669b from fix/unet-engine-path-maxpath (dotsimulate upstream PR cumulo-autumn#29, still open/unmerged) onto SDTD_032_dev. Without this, an sd-turbo + fp8 + tensorrt build with a nested engine_dir produces a UNet cache-key directory name long enough that the derived unet.engine.onnx / .opt.onnx paths exceed Windows' 260-char MAX_PATH — mkdir succeeds but torch.onnx.export's open(path, "wb") raises a bare FileNotFoundError, surfaced to users as "Acceleration has failed".

The fix hashes the full UNet prefix (same idea as the existing _lora_signature LoRA-hash pattern) into a short, stable, collision-free directory name, and adds a warning if the derived path would still exceed MAX_PATH.

  • src/streamdiffusion/acceleration/tensorrt/engine_manager.py — UNet-only: short_hash = sha1(canonical_prefix)[:12], prefix = f"{base_name}{fp8_tag}--h{short_hash}{res_tag}", plus a longest_derived >= 250 warning.
  • tests/unit/test_engine_path_length.py — 3 regression tests. Adapted from the upstream commit: SDTD_032_dev's get_engine_path doesn't yet have the pin_cache_frames/cache_maxframes params (a 040-release-only feature), so _CRASH_UNET_KWARGS is filtered against inspect.signature(get_engine_path) before use — keeps the test portable across branches without hand-editing it per cherry-pick. The UNet hashing fix itself is unconditional and unaffected by this filtering.

Companion PR on SDTD_040_beta_release: #9.

Does not touch fix/unet-engine-path-maxpath or upstream PR cumulo-autumn#29 — that PR's merge decision is dotsimulate's; this only lands the fix on our fork's own branches.

Test plan

  • python -m py_compile on both changed files
  • pytest tests/unit/test_engine_path_length.py -v — 3 passed (validated against this branch's src/, not the stale editable install, via PYTHONPATH override)
  • CI (claude-code-review) green before merge

🤖 Generated with Claude Code

forkni and others added 2 commits July 19, 2026 08:00
…ws MAX_PATH

EngineManager.get_engine_path encoded every UNet build flag verbatim into the
on-disk directory name. With a realistic engine_dir (fp8 + static batch +
pin_cache_frames + optlvl + resolution), the folder grew to 246 chars, and the
derived unet.engine.onnx / unet.engine.opt.onnx paths hit 263/267 chars --
over Windows' 260-char MAX_PATH. Because the *directory* fit but the *file*
didn't, mkdir succeeded while torch.onnx.export's open(onnx_path, "wb") raised
FileNotFoundError, which wrapper.py's OOM-only fallback doesn't catch, so the
build failed with "Acceleration has failed: [Errno 2] No such file or
directory: ...unet.engine.onnx".

Fix: for EngineType.UNET only, hash the fully-assembled flag prefix (sha1,
first 12 hex chars) using the existing _lora_signature hashing idiom, keeping
model name + fp8 + resolution human-readable in the folder name. Cache-key
precision is unchanged (identical configs still hash identically, differing
configs still diverge) -- only the on-disk encoding is compacted, from ~267
chars down to ~132. VAE/ControlNet directory naming is untouched, so existing
VAE engines are not invalidated; UNet engines rebuild once under their new
hashed folder name. Also adds a belt-and-suspenders warning if a derived path
would still approach MAX_PATH (e.g. an unusually deep user engine_dir), and
casts the "filename" config lookup to str to keep the type checker precise
about self._configs' heterogeneous per-key value types.

Adds a regression test (test_engine_path_length.py) that reproduces the exact
crash-log config and asserts the derived .onnx / .opt.onnx paths stay under
MAX_PATH, plus determinism and no-collision checks.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
pin_cache_frames/cache_maxframes are a 040-release-only feature not present
on SDTD_032_dev's get_engine_path yet. Filtering _CRASH_UNET_KWARGS against
inspect.signature keeps the cherry-picked regression test portable across
branches instead of hand-editing it per port; the UNet short_hash/MAX_PATH
fix itself is unconditional and unaffected.
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown

Claude finished @forkni's task in 3m 6s —— View job


Code Review

Tasks:

  • Gather context (PR diff, CLAUDE.md files — none exist in this repo, so reviewed against repo style/domain conventions instead)
  • Summarize changes
  • Manual review pass (traced the new UNet hashing logic end-to-end)
  • Independent bug-hunt pass (opus agent, diff-only)
  • Validate flagged issues
  • Post final review

Summary

get_engine_path (src/streamdiffusion/acceleration/tensorrt/engine_manager.py:216-230) now replaces the fully-spelled-out UNet prefix with base_name + fp8_tag + h{sha1[:12]} + res_tag before it's joined into the final engine path, and adds a >= 250-char warning (:238-244) as a second line of defense. tests/unit/test_engine_path_length.py adds 3 regression tests (MAX_PATH bound, determinism, no-collision) built on the crash report's exact flag combination, with the 040-only pin_cache_frames/cache_maxframes kwargs filtered against the live get_engine_path signature so the test stays portable across branches.

Findings

No issues found.

Traced through the logic in detail:

  • canonical = prefix captures the prefix after every build-flag suffix (fp8, cached_attn, FI, controlnet, static-batch, optlvl, mode, TRT version/cc, resolution) has already been appended, so sha1(canonical)[:12] is derived from the full, unabbreviated config — collision-safety is preserved even though the human-readable prefix is discarded.
  • The re-appended fp8_tag/res_tag in the new short prefix are cosmetic only; uniqueness is carried entirely by the hash, so there's no double-counting risk.
  • resolution[0]/[1] access is guarded by resolution is not None; base_name, fp8, resolution are all in scope from the outer function; hashlib was already imported, cast was correctly added to the typing import.
  • The cast(str, ...) on filename is a type-checker-only change (per the inline comment, _configs[engine_type]["filename"] is always a str at runtime) — no behavior change.
  • New/changed lines stay within the 119-char line-length convention (the two pre-existing >119-char lines in this file are untouched by this diff).
  • No hot-path/tensor, hardcoded-device, or TensorRT buffer-safety concerns — this is pure string/path logic in the cache-key builder, not the render loop.
  • An independent bug-hunt pass (separate agent, diff-only) reached the same conclusion.

One thing worth calling out for the PR description/reviewers (not a defect, just an expected consequence): since the UNet cache-key directory name changes from the verbose form to the hashed form, previously-built UNet engines will be cache-missed and rebuilt once on first run after this lands — expected and unavoidable for this kind of fix, not something to change.

@forkni
forkni merged commit 576fc9e into SDTD_032_dev Jul 19, 2026
1 check passed
@forkni
forkni deleted the integ/maxpath-to-032 branch July 19, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant