refactor: remediation phases 1-4 (perf, exception hygiene, correctness, TRT atomicity)#4
Merged
Merged
Conversation
…il-fast) Re-applies the previously-reverted Phase 1 batch from the perf/best-practices audit: fix the FP8 Q/DQ gate comment to match the actual threshold, convert a dict() call to a dict literal (clears the one pre-existing ruff C408 on a touched file), require an explicit opt_batch_size in EngineBuilder.build() and its 5 compile_* wrappers (real callers already always pass it), drop a dead try/except around a single logger.info call, and tighten two type hints in StreamDiffusion.pipeline (Optional/Union/Tuple). No behavior change except the opt_batch_size fail-fast, which only affects callers that previously relied on an unused default of 1.
- Add shared _is_oom_error() helper (typed torch.cuda.OutOfMemoryError + string-heuristic fallback) and use it at both TensorRT UNet/VAE engine build fallback sites instead of duplicated inline substring checks. - Log swallowed LoRA candidate-weight-name failures in _load_lora_with_offline_fallback instead of silently continuing. - Log the inner fallback failure in advanced-model-detection instead of a bare except/pass. - Hoist the optional diffusers_ipadapter helper imports in IPAdapterModule.build_unet_hook out of the per-frame hook into a single build-time resolution, and log all previously-silent per-step fallback branches for observability. - Make _load_model fail fast (raise RuntimeError) when an SDXL pipeline retry after a type mismatch also fails, instead of silently continuing with the wrong pipeline type; also null out the stale mismatched `pipe` reference so a later loading-method failure can't let it slip through the final success check. - Add regression tests for _is_oom_error and the SDXL fail-fast path.
Ruff warns that top-level ignore/select/isort/per-file-ignores are deprecated in favour of the lint section. Move them under [tool.ruff.lint] (and .isort / .per-file-ignores); line-length stays at the top level. Verified against cuda-link's pyproject.toml, which already uses this layout. No behavior change: ruff check/format report identical results before and after (same 479 pre-existing repo-wide findings, 0 on the files touched by the Phase 1/2 commits). Investigated adding a matching [tool.pyrefly] section for parity with cuda-link, but did not adopt it: pyrefly falls back to a bundled 'basic' preset when no pyrefly config exists, and that preset is substantially more lenient than explicit config. Adding even an empty [tool.pyrefly] section opts out of it and jumps reported errors from 72 to ~900 (mostly dynamically-set attributes on StreamDiffusion / StreamDiffusionWrapper that were never checked before). Doing this properly needs the same per-module triage cuda-link performed (docs/adr/0005-static-typing-hardening.md) before turning on strict project-includes checking, which is out of scope here.
_make_fake_engine (tests/unit/test_trt_engine_guards.py) built engines via TensorRTEngine.__new__() to bypass __init__, but never set _dedicated_stream, _pre_exec_event, _post_exec_event, or _buf_cache -- attributes __init__ has initialized since infer() gained cross-stream sync and LRU shape-cache support. This caused 3 AttributeErrors long treated as "pre-existing, out of scope" failures in every prior phase gate. Not a production bug: __init__ correctly sets all four to None/empty, and infer() guards each with `is not None`. Fix mirrors __init__ in the fake. Full unit suite now fully green (91 passed, 0 failed) with no waived tests.
Four targeted fixes from the perf/best-practices audit, quick-wins #4 and #5: - StreamDiffusion.prepare(): generator defaulted to a pre-constructed torch.Generator(), a mutable default evaluated once at def-time so every instance sharing the default shared one Generator. Default is now None, constructed fresh in the method body. - StreamDiffusionWrapper.prepare(): both runtime stream.prepare() calls (single-prompt and prompt-blending paths) omitted seed, so every runtime prompt change silently reset the RNG to stream.prepare()'s own default. Now forwards seed=getattr(self.stream, "current_seed", 2) to preserve the active seed across prompt changes. - postprocess_image()'s "latent" branch returned an internal decode buffer by reference; callers retaining it across frames would see it mutate. Now clones at this public API boundary. The "np" pinned-buffer alias and the internal __call__/txt2img fast-path aliases are documented as intentional, not cloned (would defeat the pinned-buffer/reuse optimizations). - cleanup_engines_and_rebuild's hardcoded engines_dir = "engines" now honors self._engine_dir when set, matching the existing idiom elsewhere in the file. - cleanup_gpu_memory dropped two explicit Engine.__del__() calls; the method already does del + triple gc.collect() + empty_cache() + ipc_collect() at its tail, and Engine.__del__ is self-guarding, so the explicit dunder calls were redundant. Adds tests/unit/test_phase3_correctness.py (5 regression tests, model-free CPU-only). Full unit suite: 91 passed, 0 failed (no pre-existing failures remain -- see cf3df18).
Guard TRT engine and timing-cache writes against truncation from an interrupted build via a temp-file + os.replace atomic write helper, matching the idiom already used for FP8 calibration data. Add self-healing recovery around cudaGraphLaunch (reset + fallback to plain execution, re-captures next frame) and a stream-sync guard around the defensive, disabled-by-default Engine.refit() path. Verified: 3 new CPU-only unit tests cover the atomic-write helper's happy path and both failure-preservation cases; a live-GPU smoke run against a cached VAE engine exercised capture, happy-path replay, a forced cudaGraphLaunch failure (recovery + safe fallback, no crash), and self-healed re-capture.
|
Claude finished @forkni's task in 4m 43s —— View job Code Review — PR #4 (remediation phases 1-4)Task list:
SummaryThis PR squashes four remediation passes (perf/best-practices, exception hygiene, correctness/resource-safety, TensorRT atomicity) cut from Key functional changes verified against the diff:
Review findingsNo issues found. Checked for bugs and correctness problems in the diff. Two independent bug-scan passes (diff-only, and full-file-context) traced through the highest-risk changes — the mutable-default fix, seed threading, buffer-aliasing/clone correctness, the IP-Adapter import hoisting (no Three points already surfaced by GitHub Copilot's automated review and addressed by the author in review comments were re-checked and are consistent with the author's stated rationale (not re-flagged here):
No CLAUDE.md files exist in this repository, so no CLAUDE.md-compliance findings apply. |
There was a problem hiding this comment.
Pull request overview
This PR consolidates remediation phases 1–4 into a single stacked change set, focusing on runtime performance in hot paths, improved exception hygiene/observability, correctness fixes around seeding and buffer aliasing, and stronger TensorRT artifact handling (atomic engine writes + refit/graph robustness).
Changes:
- Add CPU-only regression tests covering wrapper exception hygiene, Phase 3 correctness fixes (seed threading, generator default, latent cloning), and TensorRT atomic engine writes.
- Improve runtime correctness and safety in
StreamDiffusionWrapper/StreamDiffusion(seed propagation on prompt changes, explicit buffer-aliasing notes, shared OOM helper). - Harden TensorRT utilities (atomic writes, refit safety guard, CUDA graph launch fallback) and adjust TensorRT builder APIs/config (including
opt_batch_sizehandling) plus Ruff config migration.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_wrapper_exception_hygiene.py | Adds regression coverage for OOM detection helper and SDXL pipeline-mismatch fail-fast behavior. |
| tests/unit/test_trt_engine_guards.py | Updates TRT engine test doubles to mirror new runtime attributes used by infer(). |
| tests/unit/test_trt_atomic_engine_write.py | Adds unit tests for atomic engine/timing-cache writes without requiring GPU/TRT runtime. |
| tests/unit/test_phase3_correctness.py | Adds CPU-only tests for generator default fix, seed threading, and latent output cloning. |
| src/streamdiffusion/wrapper.py | Introduces _is_oom_error, threads seed through runtime prepare paths, fixes latent output aliasing, and improves failure signaling/logging. |
| src/streamdiffusion/pipeline.py | Fixes shared mutable default generator, adds debug logging, and clarifies buffer-aliasing contract. |
| src/streamdiffusion/modules/ipadapter_module.py | Avoids per-frame imports and improves debug visibility for IP-Adapter scaling logic. |
| src/streamdiffusion/acceleration/tensorrt/utilities.py | Adds atomic write helper, strengthens refit/graph robustness, and minor cleanups. |
| src/streamdiffusion/acceleration/tensorrt/builder.py | Changes opt_batch_size handling to fail-fast when omitted; minor refactors. |
| src/streamdiffusion/acceleration/tensorrt/init.py | Propagates opt_batch_size signature changes through compile helpers. |
| pyproject.toml | Migrates Ruff config keys to the newer [tool.ruff.lint] structure and sets line-length at top level. |
Comment on lines
+27
to
+30
| error_msg = str(exc).lower() | ||
| return ( | ||
| "out of memory" in error_msg or "outofmemory" in error_msg or "oom" in error_msg or "cuda error" in error_msg | ||
| ) |
Owner
Author
There was a problem hiding this comment.
Verified — the repo's own test (test_wrapper_exception_hygiene.py:54) explicitly asserts that exact "CUDA error: an illegal memory access was encountered" string should classify as OOM. I confirmed by narrowing the heuristic locally: it breaks that test. This is a deliberate prior design decision, not an oversight — treating any generic CUDA RuntimeError as OOM-fallback-worthy is intentional here, since falling back to PyTorch is considered a safe generic recovery even for non-OOM CUDA corruption, and under-triggering the fallback is judged riskier than over-triggering it. Leaving as-is; not a bug in this PR.
Comment on lines
+121
to
+122
| if opt_batch_size is None: | ||
| raise ValueError("build() requires an explicit opt_batch_size") |
Owner
Author
There was a problem hiding this comment.
Confirmed intentional — this is Phase 4's stated goal (fail-fast hardening on TensorRT engine builds). As your review noted, every in-repo call site already passes opt_batch_size explicitly, so the raise is unreachable internally; it only tightens the contract for any external caller, who should be passing it explicitly anyway. Leaving as-is.
Comment on lines
63
to
70
| def compile_vae_encoder( | ||
| vae: TorchVAEEncoder, | ||
| model_data: BaseModel, | ||
| onnx_path: str, | ||
| onnx_opt_path: str, | ||
| engine_path: str, | ||
| opt_batch_size: int = 1, | ||
| opt_batch_size: Optional[int] = None, | ||
| engine_build_options: Optional[dict] = None, |
Owner
Author
There was a problem hiding this comment.
Same as the builder.py comment on this PR — intentional fail-fast hardening (Phase 4), all in-repo callers already pass opt_batch_size explicitly, so this only tightens the contract for external callers. Leaving as-is.
Adds this workflow to the PR 1 branch and widens the base-branch filter to include the chained PR 2-5 branch names, so the Claude Code Action fires on PRs based on this branch (not just SDTD_032_dev) -- GitHub Actions resolves the pull_request workflow from the PR's base branch. Byte-identical change also applied to forkni:main for claude-code-action@v1 parity.
…flow file add/add conflict
2 tasks
…mission visibility
…rite) to unblock permission-denied max-turns failures
…e find and output redirection
This was referenced Jul 14, 2026
forkni
added a commit
that referenced
this pull request
Jul 19, 2026
* refactor: re-apply Phase 1 perf/best-practices cleanup (cosmetic + fail-fast) Re-applies the previously-reverted Phase 1 batch from the perf/best-practices audit: fix the FP8 Q/DQ gate comment to match the actual threshold, convert a dict() call to a dict literal (clears the one pre-existing ruff C408 on a touched file), require an explicit opt_batch_size in EngineBuilder.build() and its 5 compile_* wrappers (real callers already always pass it), drop a dead try/except around a single logger.info call, and tighten two type hints in StreamDiffusion.pipeline (Optional/Union/Tuple). No behavior change except the opt_batch_size fail-fast, which only affects callers that previously relied on an unused default of 1. * refactor: Phase 2 exception hygiene and observability fixes - Add shared _is_oom_error() helper (typed torch.cuda.OutOfMemoryError + string-heuristic fallback) and use it at both TensorRT UNet/VAE engine build fallback sites instead of duplicated inline substring checks. - Log swallowed LoRA candidate-weight-name failures in _load_lora_with_offline_fallback instead of silently continuing. - Log the inner fallback failure in advanced-model-detection instead of a bare except/pass. - Hoist the optional diffusers_ipadapter helper imports in IPAdapterModule.build_unet_hook out of the per-frame hook into a single build-time resolution, and log all previously-silent per-step fallback branches for observability. - Make _load_model fail fast (raise RuntimeError) when an SDXL pipeline retry after a type mismatch also fails, instead of silently continuing with the wrong pipeline type; also null out the stale mismatched `pipe` reference so a later loading-method failure can't let it slip through the final success check. - Add regression tests for _is_oom_error and the SDXL fail-fast path. * chore: migrate deprecated top-level ruff settings to [tool.ruff.lint] Ruff warns that top-level ignore/select/isort/per-file-ignores are deprecated in favour of the lint section. Move them under [tool.ruff.lint] (and .isort / .per-file-ignores); line-length stays at the top level. Verified against cuda-link's pyproject.toml, which already uses this layout. No behavior change: ruff check/format report identical results before and after (same 479 pre-existing repo-wide findings, 0 on the files touched by the Phase 1/2 commits). Investigated adding a matching [tool.pyrefly] section for parity with cuda-link, but did not adopt it: pyrefly falls back to a bundled 'basic' preset when no pyrefly config exists, and that preset is substantially more lenient than explicit config. Adding even an empty [tool.pyrefly] section opts out of it and jumps reported errors from 72 to ~900 (mostly dynamically-set attributes on StreamDiffusion / StreamDiffusionWrapper that were never checked before). Doing this properly needs the same per-module triage cuda-link performed (docs/adr/0005-static-typing-hardening.md) before turning on strict project-includes checking, which is out of scope here. * fix: sync _make_fake_engine test double with TensorRTEngine.__init__ _make_fake_engine (tests/unit/test_trt_engine_guards.py) built engines via TensorRTEngine.__new__() to bypass __init__, but never set _dedicated_stream, _pre_exec_event, _post_exec_event, or _buf_cache -- attributes __init__ has initialized since infer() gained cross-stream sync and LRU shape-cache support. This caused 3 AttributeErrors long treated as "pre-existing, out of scope" failures in every prior phase gate. Not a production bug: __init__ correctly sets all four to None/empty, and infer() guards each with `is not None`. Fix mirrors __init__ in the fake. Full unit suite now fully green (91 passed, 0 failed) with no waived tests. * fix: Phase 3 correctness and resource-safety fixes Four targeted fixes from the perf/best-practices audit, quick-wins #4 and #5: - StreamDiffusion.prepare(): generator defaulted to a pre-constructed torch.Generator(), a mutable default evaluated once at def-time so every instance sharing the default shared one Generator. Default is now None, constructed fresh in the method body. - StreamDiffusionWrapper.prepare(): both runtime stream.prepare() calls (single-prompt and prompt-blending paths) omitted seed, so every runtime prompt change silently reset the RNG to stream.prepare()'s own default. Now forwards seed=getattr(self.stream, "current_seed", 2) to preserve the active seed across prompt changes. - postprocess_image()'s "latent" branch returned an internal decode buffer by reference; callers retaining it across frames would see it mutate. Now clones at this public API boundary. The "np" pinned-buffer alias and the internal __call__/txt2img fast-path aliases are documented as intentional, not cloned (would defeat the pinned-buffer/reuse optimizations). - cleanup_engines_and_rebuild's hardcoded engines_dir = "engines" now honors self._engine_dir when set, matching the existing idiom elsewhere in the file. - cleanup_gpu_memory dropped two explicit Engine.__del__() calls; the method already does del + triple gc.collect() + empty_cache() + ipc_collect() at its tail, and Engine.__del__ is self-guarding, so the explicit dunder calls were redundant. Adds tests/unit/test_phase3_correctness.py (5 regression tests, model-free CPU-only). Full unit suite: 91 passed, 0 failed (no pre-existing failures remain -- see cf3df18). * fix: Phase 4 TensorRT engine-write atomicity and graph/refit robustness Guard TRT engine and timing-cache writes against truncation from an interrupted build via a temp-file + os.replace atomic write helper, matching the idiom already used for FP8 calibration data. Add self-healing recovery around cudaGraphLaunch (reset + fallback to plain execution, re-captures next frame) and a stream-sync guard around the defensive, disabled-by-default Engine.refit() path. Verified: 3 new CPU-only unit tests cover the atomic-write helper's happy path and both failure-preservation cases; a live-GPU smoke run against a cached VAE engine exercised capture, happy-path replay, a forced cudaGraphLaunch failure (recovery + safe fallback, no crash), and self-healed re-capture. * perf: instrument unet_step with profiler.region() spans (Sub-phase 5.0) Adds 5 graph-safe profiler.region() spans (prep/sdxl_cond/hooks/engine/post) inside unet_step to attribute the audit's "~66% unattributed" frame time. Baseline capture (RTX 4090, SDXL-turbo FP8) shows that slice is GPU kernel time inside the single TRT UNet engine call (~93% of frame), not host overhead - every host sub-span measures <0.01ms. Instrumentation only, no behavior change; validated by the existing 94/0 unit suite. * perf: skip redundant set_tensor_address rebind in TRT graph replay (Sub-phase 5.1) Engine.infer() unconditionally re-bound every tensor address via set_tensor_address() before each engine call, even in graphed steady state where the addresses are already baked into the captured CUDA graph (self.tensors buffers are persistent and reused via copy_() - see allocate_buffers/_can_reuse_buffers, which resets cuda_graph_instance to None whenever a buffer is actually reallocated). Skip the rebind loop once a graph is captured; only rebind on first capture or after a reset_cuda_graph() recovery. Verified live on the RTX 4090 td_config.yaml runtime path (the UNet engine loads with use_cuda_graph=True unconditionally via engine_manager.py's loader): 24-frame smoke run with real graph capture + replay shows no errors and frame timing (p50 32.10ms, mean 47.33ms) matching a pre-fix A/B run (p50 31.74ms, mean 47.00ms) - confirms correctness with the expected ~0 measured perf delta (the audit's own 5.0 baseline showed host-side region time is <0.01ms; this is a hygiene/false-dependency fix, not a throughput win). Suite 94/0; ruff clean; pyrefly 72 errors (baseline, no new). * perf: sync-free output/PIL/IPC paths (Sub-phase 5.2) Three output-side sync/allocation fixes, ordered by real-world impact on the TD path: - 5f (only hot-path site): _ipc_pack_rgba / _ipc_pack_unit_rgba now write into a persistent HWC x4 GPU buffer (alpha set once at allocation, B/G/R channels written in place) instead of allocating a fresh torch.full_like alpha + torch.cat every frame. Safe only because the IPC exporters are forced to blocking export (ADR-0001); comments updated at both _lazy_init_ipc_exporter and _lazy_init_cn_ipc_exporter to reflect the buffer is now persistent, not transient. - 5e: _tensor_to_pil_safe (preprocessing_orchestrator.py) reordered CPU-first, following the in-repo base.py:tensor_to_pil template -- the D2H transfer now happens before the tensor.min()/.max() range-check syncs instead of after, collapsing 3 GPU host syncs down to 1. - 5d (hygiene; TD's IPC output path never hits this): _tensor_to_pil_optimized now routes through the shared _output_pin_buf/_d2h_event pinned-buffer + Event machinery instead of a blocking, unpinned .cpu() into pageable memory. Documented the same buffer-reuse caveat as the "np" path (PIL images returned wrap a view of the pinned buffer; callers retaining them across frames must .copy()). Verified via tests/unit/test_sync_free_output_5_2.py: byte-identical parity against frozen pre-5.2 reference formulas for all three sites, plus persistent-buffer reuse/realloc and independent-buffer-identity checks (10 new tests, GPU-gated). Suite 104/0 (94 baseline + 10 new); ruff clean on touched files; pyrefly 72 errors (baseline, no new). * fix: batch-dynamic engines skip l2tc tiling, silencing benign VALIDATE FAIL build_engine() passed dynamic_shapes=build_dynamic_shape into Engine.build(), which tracks only dynamic resolution. On the default "Flexible" preset the UNet is resolution-static but batch-dynamic (build_static_batch=False), so dynamic_shapes was passed False, and _apply_gpu_profile_to_config's tiling branch ran against a graph that still has a symbolic batch dim -- TRT emitted "[l2tc] VALIDATE FAIL - Graph contains symbolic shape" as a no-op for every applicable layer, for the entire ~11.5 min production TD session analyzed this round. Fix: dynamic_shapes=build_dynamic_shape or not build_static_batch (one line, utilities.py:1372) -- True whenever any dim, resolution or batch, is symbolic, matching l2tc's actual requirement (ALL dims must be concrete). Fully-static engines (e.g. ControlNet: build_static_batch=True, build_dynamic_shape=False) are unaffected -- still get tiling as before. Tightened the _apply_gpu_profile_to_config docstring to match. Verified via new tests/unit/test_l2tc_dynamic_shapes.py: calls build_engine() with GPU detection/memory query/the real TRT build all monkeypatched out (no CUDA/TRT context required) and asserts the dynamic_shapes kwarg it wires into Engine.build() for the previously-broken case (batch-dynamic, resolution-static -> True), the unaffected fully-static case (-> False), and the already-working resolution-dynamic case (-> True). Suite 107/0 (104 baseline + 3 new); ruff clean; pyrefly 72 errors (baseline, no new). Impact is log-spam-only -- zero perf/numerical change, visible on the next engine rebuild. * perf: elide trt.input_staging DtoD copy for kvo/fio UNet cache inputs (Sub-phase 5.6) Engine.infer() copied every feed_dict tensor into its own persistent staging buffer each frame solely to give TensorRT a stable contiguous address to bind. nsys profiling (5.0/5.5) attributed ~9% of frame time (p95 3.1ms, ~161 DtoD copies/frame) to this region on the UNet call, dominated by the kvo/fio StreamV2V cache inputs. Those cache tensors are already persistent, address-stable, TRT-contiguous buffers (create_kvo_cache/create_fi_cache build them with layers_in_bucket as the outermost dim specifically for this) — the only event that moves their address is a batch-size change, which already forces a full buffer realloc+graph reset today. So copying them every frame is pure waste. Add an opt-in zero_copy_names parameter to Engine.infer() and a pure _staging_action() decision helper (copy/bind/bind_and_reset) that lets TensorRT bind directly to the caller's tensor instead of copying into self.tensors[name], guarded by contiguity and dtype-match fallbacks. Wire it in UNet2DConditionModelEngine for the kvo/fio input names only; every other caller (VAE, ControlNet, safety, preprocessing) passes nothing and keeps the original copy path byte-for-byte. * fix: lower ControlNet dynamic-shape floor 384->256 via shared min_image_shape (bug #6) ControlNet's dynamic-shape get_input_profile() hardcoded a 384px floor independent of the UNet's 256px floor (BaseModel.min_image_shape), so any resolution in [256, 384) hard-failed at Engine.allocate_buffers the moment ControlNet was enabled. Derive the ControlNet floor/ceiling from the same BaseModel.min_image_shape/max_image_shape the UNet already uses (SDXL inherits via super()), lower the matching engine_manager.py build-option default, and relabel the ControlNet dynamic-engine cache directory (--dyn-384-1024 -> --dyn-256-1024) so a re-floored build never collides with a stale 384-floored cache. Trade-off: widening the dynamic profile can slightly reduce TRT tactic specialization at the optimum resolution -- acceptable given the alternative is a hard failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: normalize blend weights on CPU + persistent ControlNet conditioning_scale scalar (Sub-phase 5.4) Two per-frame GPU creation syncs removed: - _normalize_weights built a CUDA tensor from a Python list every call, but every consumer (.item()/.tolist() in slerp/multi_slerp/cosine_weighted paths, float*cuda_tensor in the linear paths) immediately reads it back or treats it as a scalar operand. Build it on CPU float32 instead -- same consumers work unchanged (a 0-dim CPU tensor is treated as a wrapped scalar when multiplied against a CUDA tensor), the sync and readback are both gone, and float32 is more precise than the model's fp16 for the normalization divide. - ControlNetModelEngine rebuilt a conditioning_scale tensor via torch.tensor(...) on every __call__. Replace with one lazily-allocated persistent scalar updated via fill_() (an async kernel launch, no sync), mirroring the existing _fi_strength_tensor.fill_() idiom. Each active ControlNet has its own engine instance, so a per-instance buffer is correct; the binding rank (_cs_rank1) is fixed at load time so the shape never changes underneath it. Also drops three dead variable assignments and fixes one out-of-order import in stream_parameter_updater.py (pre-existing lint debt in a file this change already touches; the repo's commit gate lints the whole staged file, not just the diff). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 1-frame-delayed async NSFW safety-checker readback (Sub-phase 5.3) Removes the cudaStreamSynchronize that NSFWDetectorEngine.__call__ forced on the hot path via probs[0,0].item() whenever use_safety_checker=True. The classifier now writes its GPU probability into a pinned host scalar via a non_blocking copy; _apply_safety_checker reads the *previous* frame's pinned verdict before launching this frame's classification, mirroring the existing SimilarImageFilter 1-frame-delayed readback pattern (image_filter.py). Edge cases: first frame passes through unconditionally (no prior verdict to act on); a 1-frame detection-leak window is possible before flagging lands. Both are acceptable for an opt-in, off-by-default, TensorRT-only feature. Updated tests/unit/test_safety_checker.py for the new (tensor, prob_pin) -> None checker contract and the delayed-readback semantics (10/10 passing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: add Claude review workflow to PR2 branch for head-ref validation * chore: widen Claude review allowedTools (Skill, python/pytest, /tmp Write) to unblock permission-denied max-turns failures * chore: raise Claude review max-turns to 60, discourage filesystem-wide find and output redirection * fix: correct NSFW verdict-to-frame attribution and guard pin_memory on CPU-only builds _apply_safety_checker previously read the prior frame's async verdict but applied it to the current frame's pixels, letting an isolated NSFW frame leak through and wrongly blanking the next clean frame. Buffer the raw frame in _pending_frame and emit it once its own verdict lands (+1 frame latency, first call now emits a black startup frame instead of raw pixels). Also guard .pin_memory() behind torch.cuda.is_available() so the CPU-only test contract in test_safety_checker.py holds. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
forkni
added a commit
that referenced
this pull request
Jul 19, 2026
…uard, ONNX export) * refactor: re-apply Phase 1 perf/best-practices cleanup (cosmetic + fail-fast) Re-applies the previously-reverted Phase 1 batch from the perf/best-practices audit: fix the FP8 Q/DQ gate comment to match the actual threshold, convert a dict() call to a dict literal (clears the one pre-existing ruff C408 on a touched file), require an explicit opt_batch_size in EngineBuilder.build() and its 5 compile_* wrappers (real callers already always pass it), drop a dead try/except around a single logger.info call, and tighten two type hints in StreamDiffusion.pipeline (Optional/Union/Tuple). No behavior change except the opt_batch_size fail-fast, which only affects callers that previously relied on an unused default of 1. * refactor: Phase 2 exception hygiene and observability fixes - Add shared _is_oom_error() helper (typed torch.cuda.OutOfMemoryError + string-heuristic fallback) and use it at both TensorRT UNet/VAE engine build fallback sites instead of duplicated inline substring checks. - Log swallowed LoRA candidate-weight-name failures in _load_lora_with_offline_fallback instead of silently continuing. - Log the inner fallback failure in advanced-model-detection instead of a bare except/pass. - Hoist the optional diffusers_ipadapter helper imports in IPAdapterModule.build_unet_hook out of the per-frame hook into a single build-time resolution, and log all previously-silent per-step fallback branches for observability. - Make _load_model fail fast (raise RuntimeError) when an SDXL pipeline retry after a type mismatch also fails, instead of silently continuing with the wrong pipeline type; also null out the stale mismatched `pipe` reference so a later loading-method failure can't let it slip through the final success check. - Add regression tests for _is_oom_error and the SDXL fail-fast path. * chore: migrate deprecated top-level ruff settings to [tool.ruff.lint] Ruff warns that top-level ignore/select/isort/per-file-ignores are deprecated in favour of the lint section. Move them under [tool.ruff.lint] (and .isort / .per-file-ignores); line-length stays at the top level. Verified against cuda-link's pyproject.toml, which already uses this layout. No behavior change: ruff check/format report identical results before and after (same 479 pre-existing repo-wide findings, 0 on the files touched by the Phase 1/2 commits). Investigated adding a matching [tool.pyrefly] section for parity with cuda-link, but did not adopt it: pyrefly falls back to a bundled 'basic' preset when no pyrefly config exists, and that preset is substantially more lenient than explicit config. Adding even an empty [tool.pyrefly] section opts out of it and jumps reported errors from 72 to ~900 (mostly dynamically-set attributes on StreamDiffusion / StreamDiffusionWrapper that were never checked before). Doing this properly needs the same per-module triage cuda-link performed (docs/adr/0005-static-typing-hardening.md) before turning on strict project-includes checking, which is out of scope here. * fix: sync _make_fake_engine test double with TensorRTEngine.__init__ _make_fake_engine (tests/unit/test_trt_engine_guards.py) built engines via TensorRTEngine.__new__() to bypass __init__, but never set _dedicated_stream, _pre_exec_event, _post_exec_event, or _buf_cache -- attributes __init__ has initialized since infer() gained cross-stream sync and LRU shape-cache support. This caused 3 AttributeErrors long treated as "pre-existing, out of scope" failures in every prior phase gate. Not a production bug: __init__ correctly sets all four to None/empty, and infer() guards each with `is not None`. Fix mirrors __init__ in the fake. Full unit suite now fully green (91 passed, 0 failed) with no waived tests. * fix: Phase 3 correctness and resource-safety fixes Four targeted fixes from the perf/best-practices audit, quick-wins #4 and #5: - StreamDiffusion.prepare(): generator defaulted to a pre-constructed torch.Generator(), a mutable default evaluated once at def-time so every instance sharing the default shared one Generator. Default is now None, constructed fresh in the method body. - StreamDiffusionWrapper.prepare(): both runtime stream.prepare() calls (single-prompt and prompt-blending paths) omitted seed, so every runtime prompt change silently reset the RNG to stream.prepare()'s own default. Now forwards seed=getattr(self.stream, "current_seed", 2) to preserve the active seed across prompt changes. - postprocess_image()'s "latent" branch returned an internal decode buffer by reference; callers retaining it across frames would see it mutate. Now clones at this public API boundary. The "np" pinned-buffer alias and the internal __call__/txt2img fast-path aliases are documented as intentional, not cloned (would defeat the pinned-buffer/reuse optimizations). - cleanup_engines_and_rebuild's hardcoded engines_dir = "engines" now honors self._engine_dir when set, matching the existing idiom elsewhere in the file. - cleanup_gpu_memory dropped two explicit Engine.__del__() calls; the method already does del + triple gc.collect() + empty_cache() + ipc_collect() at its tail, and Engine.__del__ is self-guarding, so the explicit dunder calls were redundant. Adds tests/unit/test_phase3_correctness.py (5 regression tests, model-free CPU-only). Full unit suite: 91 passed, 0 failed (no pre-existing failures remain -- see cf3df18). * fix: Phase 4 TensorRT engine-write atomicity and graph/refit robustness Guard TRT engine and timing-cache writes against truncation from an interrupted build via a temp-file + os.replace atomic write helper, matching the idiom already used for FP8 calibration data. Add self-healing recovery around cudaGraphLaunch (reset + fallback to plain execution, re-captures next frame) and a stream-sync guard around the defensive, disabled-by-default Engine.refit() path. Verified: 3 new CPU-only unit tests cover the atomic-write helper's happy path and both failure-preservation cases; a live-GPU smoke run against a cached VAE engine exercised capture, happy-path replay, a forced cudaGraphLaunch failure (recovery + safe fallback, no crash), and self-healed re-capture. * perf: instrument unet_step with profiler.region() spans (Sub-phase 5.0) Adds 5 graph-safe profiler.region() spans (prep/sdxl_cond/hooks/engine/post) inside unet_step to attribute the audit's "~66% unattributed" frame time. Baseline capture (RTX 4090, SDXL-turbo FP8) shows that slice is GPU kernel time inside the single TRT UNet engine call (~93% of frame), not host overhead - every host sub-span measures <0.01ms. Instrumentation only, no behavior change; validated by the existing 94/0 unit suite. * perf: skip redundant set_tensor_address rebind in TRT graph replay (Sub-phase 5.1) Engine.infer() unconditionally re-bound every tensor address via set_tensor_address() before each engine call, even in graphed steady state where the addresses are already baked into the captured CUDA graph (self.tensors buffers are persistent and reused via copy_() - see allocate_buffers/_can_reuse_buffers, which resets cuda_graph_instance to None whenever a buffer is actually reallocated). Skip the rebind loop once a graph is captured; only rebind on first capture or after a reset_cuda_graph() recovery. Verified live on the RTX 4090 td_config.yaml runtime path (the UNet engine loads with use_cuda_graph=True unconditionally via engine_manager.py's loader): 24-frame smoke run with real graph capture + replay shows no errors and frame timing (p50 32.10ms, mean 47.33ms) matching a pre-fix A/B run (p50 31.74ms, mean 47.00ms) - confirms correctness with the expected ~0 measured perf delta (the audit's own 5.0 baseline showed host-side region time is <0.01ms; this is a hygiene/false-dependency fix, not a throughput win). Suite 94/0; ruff clean; pyrefly 72 errors (baseline, no new). * perf: sync-free output/PIL/IPC paths (Sub-phase 5.2) Three output-side sync/allocation fixes, ordered by real-world impact on the TD path: - 5f (only hot-path site): _ipc_pack_rgba / _ipc_pack_unit_rgba now write into a persistent HWC x4 GPU buffer (alpha set once at allocation, B/G/R channels written in place) instead of allocating a fresh torch.full_like alpha + torch.cat every frame. Safe only because the IPC exporters are forced to blocking export (ADR-0001); comments updated at both _lazy_init_ipc_exporter and _lazy_init_cn_ipc_exporter to reflect the buffer is now persistent, not transient. - 5e: _tensor_to_pil_safe (preprocessing_orchestrator.py) reordered CPU-first, following the in-repo base.py:tensor_to_pil template -- the D2H transfer now happens before the tensor.min()/.max() range-check syncs instead of after, collapsing 3 GPU host syncs down to 1. - 5d (hygiene; TD's IPC output path never hits this): _tensor_to_pil_optimized now routes through the shared _output_pin_buf/_d2h_event pinned-buffer + Event machinery instead of a blocking, unpinned .cpu() into pageable memory. Documented the same buffer-reuse caveat as the "np" path (PIL images returned wrap a view of the pinned buffer; callers retaining them across frames must .copy()). Verified via tests/unit/test_sync_free_output_5_2.py: byte-identical parity against frozen pre-5.2 reference formulas for all three sites, plus persistent-buffer reuse/realloc and independent-buffer-identity checks (10 new tests, GPU-gated). Suite 104/0 (94 baseline + 10 new); ruff clean on touched files; pyrefly 72 errors (baseline, no new). * fix: batch-dynamic engines skip l2tc tiling, silencing benign VALIDATE FAIL build_engine() passed dynamic_shapes=build_dynamic_shape into Engine.build(), which tracks only dynamic resolution. On the default "Flexible" preset the UNet is resolution-static but batch-dynamic (build_static_batch=False), so dynamic_shapes was passed False, and _apply_gpu_profile_to_config's tiling branch ran against a graph that still has a symbolic batch dim -- TRT emitted "[l2tc] VALIDATE FAIL - Graph contains symbolic shape" as a no-op for every applicable layer, for the entire ~11.5 min production TD session analyzed this round. Fix: dynamic_shapes=build_dynamic_shape or not build_static_batch (one line, utilities.py:1372) -- True whenever any dim, resolution or batch, is symbolic, matching l2tc's actual requirement (ALL dims must be concrete). Fully-static engines (e.g. ControlNet: build_static_batch=True, build_dynamic_shape=False) are unaffected -- still get tiling as before. Tightened the _apply_gpu_profile_to_config docstring to match. Verified via new tests/unit/test_l2tc_dynamic_shapes.py: calls build_engine() with GPU detection/memory query/the real TRT build all monkeypatched out (no CUDA/TRT context required) and asserts the dynamic_shapes kwarg it wires into Engine.build() for the previously-broken case (batch-dynamic, resolution-static -> True), the unaffected fully-static case (-> False), and the already-working resolution-dynamic case (-> True). Suite 107/0 (104 baseline + 3 new); ruff clean; pyrefly 72 errors (baseline, no new). Impact is log-spam-only -- zero perf/numerical change, visible on the next engine rebuild. * perf: elide trt.input_staging DtoD copy for kvo/fio UNet cache inputs (Sub-phase 5.6) Engine.infer() copied every feed_dict tensor into its own persistent staging buffer each frame solely to give TensorRT a stable contiguous address to bind. nsys profiling (5.0/5.5) attributed ~9% of frame time (p95 3.1ms, ~161 DtoD copies/frame) to this region on the UNet call, dominated by the kvo/fio StreamV2V cache inputs. Those cache tensors are already persistent, address-stable, TRT-contiguous buffers (create_kvo_cache/create_fi_cache build them with layers_in_bucket as the outermost dim specifically for this) — the only event that moves their address is a batch-size change, which already forces a full buffer realloc+graph reset today. So copying them every frame is pure waste. Add an opt-in zero_copy_names parameter to Engine.infer() and a pure _staging_action() decision helper (copy/bind/bind_and_reset) that lets TensorRT bind directly to the caller's tensor instead of copying into self.tensors[name], guarded by contiguity and dtype-match fallbacks. Wire it in UNet2DConditionModelEngine for the kvo/fio input names only; every other caller (VAE, ControlNet, safety, preprocessing) passes nothing and keeps the original copy path byte-for-byte. * fix: lower ControlNet dynamic-shape floor 384->256 via shared min_image_shape (bug #6) ControlNet's dynamic-shape get_input_profile() hardcoded a 384px floor independent of the UNet's 256px floor (BaseModel.min_image_shape), so any resolution in [256, 384) hard-failed at Engine.allocate_buffers the moment ControlNet was enabled. Derive the ControlNet floor/ceiling from the same BaseModel.min_image_shape/max_image_shape the UNet already uses (SDXL inherits via super()), lower the matching engine_manager.py build-option default, and relabel the ControlNet dynamic-engine cache directory (--dyn-384-1024 -> --dyn-256-1024) so a re-floored build never collides with a stale 384-floored cache. Trade-off: widening the dynamic profile can slightly reduce TRT tactic specialization at the optimum resolution -- acceptable given the alternative is a hard failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: normalize blend weights on CPU + persistent ControlNet conditioning_scale scalar (Sub-phase 5.4) Two per-frame GPU creation syncs removed: - _normalize_weights built a CUDA tensor from a Python list every call, but every consumer (.item()/.tolist() in slerp/multi_slerp/cosine_weighted paths, float*cuda_tensor in the linear paths) immediately reads it back or treats it as a scalar operand. Build it on CPU float32 instead -- same consumers work unchanged (a 0-dim CPU tensor is treated as a wrapped scalar when multiplied against a CUDA tensor), the sync and readback are both gone, and float32 is more precise than the model's fp16 for the normalization divide. - ControlNetModelEngine rebuilt a conditioning_scale tensor via torch.tensor(...) on every __call__. Replace with one lazily-allocated persistent scalar updated via fill_() (an async kernel launch, no sync), mirroring the existing _fi_strength_tensor.fill_() idiom. Each active ControlNet has its own engine instance, so a per-instance buffer is correct; the binding rank (_cs_rank1) is fixed at load time so the shape never changes underneath it. Also drops three dead variable assignments and fixes one out-of-order import in stream_parameter_updater.py (pre-existing lint debt in a file this change already touches; the repo's commit gate lints the whole staged file, not just the diff). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 1-frame-delayed async NSFW safety-checker readback (Sub-phase 5.3) Removes the cudaStreamSynchronize that NSFWDetectorEngine.__call__ forced on the hot path via probs[0,0].item() whenever use_safety_checker=True. The classifier now writes its GPU probability into a pinned host scalar via a non_blocking copy; _apply_safety_checker reads the *previous* frame's pinned verdict before launching this frame's classification, mirroring the existing SimilarImageFilter 1-frame-delayed readback pattern (image_filter.py). Edge cases: first frame passes through unconditionally (no prior verdict to act on); a 1-frame detection-leak window is possible before flagging lands. Both are acceptable for an opt-in, off-by-default, TensorRT-only feature. Updated tests/unit/test_safety_checker.py for the new (tensor, prob_pin) -> None checker contract and the delayed-readback semantics (10/10 passing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: persistent multi-ControlNet residual merge buffer (Phase-2 prep) The multi-CN merge previously reallocated `merged_down`/`merged_mid` every frame (`merged_down[j] = merged_down[j] + ds[j]`), so residual pointers changed every frame. That would thrash a CUDA graph if these tensors are later bound zero-copy into the UNet's input_control_* inputs (Phase-2 D2). Replace it with lazily-allocated, shape-guarded persistent buffers (_cn_merged_down/_cn_merged_mid) that accumulate in place via copy_/add_, reallocating only on a genuine shape/dtype/device change. Does not alias down_samples_list[0]/mid_samples_list[0] (engine A's own persistent output buffers). Numerically identical to the old sum; single-ControlNet path is unaffected (already address-stable, no merge buffer allocated). Also removes pre-existing lint debt in the same file (duplicate OrchestratorUser import, unused base_kwargs dict), required to pass the commit gate; verified via git stash that both predated this change. Adds tests/unit/test_controlnet_residual_merge.py covering numerical correctness, cross-frame pointer stability, reallocation on shape change, non-aliasing of engine A's buffer, single-CN bypass, and install() reset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 256-byte alignment guard for zero-copy bind path (audit M2) setTensorAddress requires >=256-byte alignment; _staging_action's zero-copy eligibility test didn't enforce it, relying on an arithmetic proof instead of a runtime invariant. Add cur_ptr % 256 == 0 to the eligibility check so a mis-aligned pointer always falls back to the safe copy path. Real torch CUDA allocations are >=512-byte aligned, so this guard never fires in production — it only defends against future config drift, and is a prerequisite for Phase-2 D2's new zero-copy input_control_* bindings. Realigns the test suite's PTR_A/PTR_B fixtures to 256-byte-aligned values (the old 0xDEAD_BEEF/0xFEED_FACE constants were both mis-aligned and would have broken under the new guard) and adds two rows asserting the misaligned fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: zero-copy bind ControlNet residual UNet inputs (Phase-2 D2) Widen UNet2DConditionModelEngine._zero_copy_names to include input_control_00..19 and input_control_middle alongside the existing kvo/fio cache names, so Engine.infer() binds TensorRT directly to the ControlNet residual tensors instead of DtoD-copying them into a staging buffer every frame (the Sub-phase 5.6 mechanism, now audit-M2 alignment guarded). In steady state the tensors placed in input_dict[input_control_*] are the same persistent objects each frame: multi-ControlNet uses D1's persistent merge buffers (controlnet_module.py), single-ControlNet reuses the engine's own persistent output buffers, and idle frames reuse the persistent dummy zero cache. Pointers only change on a resolution/batch/model change or the idle<->active toggle, which _staging_action already handles via bind_and_reset (one graph re-capture on the toggle frame, not per-frame). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor: harden TRT runtime API usage (audit M1/H1/H2) M1: TensorRTEngine (trt_base.py) ignored the bool return of set_input_shape (allocate_buffers, infer) and set_tensor_address (infer). An out-of-profile shape or invalid address was silently accepted, surfacing later as an execute_async_v3 failure or garbage output. Mirror the core Engine pattern (utilities.py) and raise RuntimeError with the tensor name/shape when either call returns False. Affects depth/pose (+HED/NormalBae via the shared base). H1: replace the try/except (AttributeError, TypeError) around the SM_120 TacticSource scoping block with an explicit hasattr(trt.TacticSource, "CUBLAS") gate. CUBLAS/CUBLAS_LT are removed in TRT 11, so the old except would silently swallow the AttributeError and drop the scoping intent without a trace; the new branch logs the TRT>=11 case explicitly. No behavior change on the installed TRT 10.16 (all members still present). H2: delete the dead reuse_device_memory branch in Engine.activate() — no caller across any of the ~10 activate() call sites passes it, and the one-arg context.device_memory setter it used has been superseded since TRT 10.1. Migrating to set_device_memory(ptr, engine.device_memory_size_v2) is deferred until device-memory reuse is actually wanted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor: modernize ONNX compile/export path — create_network(), drop deprecated export flag, robust external-data detection (audit M3/L1/L3) M3: replace deprecated create_network(1<<EXPLICIT_BATCH) with create_network() in compile_depth_anything_tensorrt.py, mirroring compile_raft_tensorrt.py. Also swaps the unused-import onnx availability check for importlib.util.find_spec (ruff F401 in the same file's try/except block). L1: drop the deprecated do_constant_folding=True kwarg from the torch.onnx.export call in utilities.py; dynamo=False stays explicit (audit L2: no action needed). L3: replace the ".pb"-suffix directory scan in optimize_onnx() with a check of TensorProto.data_location == EXTERNAL on the loaded (but not yet materialized) model, removing the filename-heuristic's dependence on export_onnx's consolidate-to-weights.pb ordering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: make ControlNet zero-copy revert permanent; harden staging bind->copy gap Post-mortem: f8ff50f ("zero-copy bind ControlNet residual UNet inputs") caused a confirmed regression - ControlNet produced no visual change for both scribble (ramp 0->0.36) and Canny (static 1.0), at any scale. An interim revert (dropping input_control_* from the UNet engine's _zero_copy_names) was verified working on the rig for both models. This commit makes that revert permanent rather than re-enabling the optimization. Root cause, confirmed by code inspection (not just bisection): a zero-copy bind drops the copy_() call that runs on the engine stream and orders the ControlNet engine's residual write before the UNet CUDA graph's read of it ("copy_() executes on engine stream to guarantee ordering" - utilities.py). An earlier hypothesis assumed the failure was a bind->copy staging fallback triggered by cfg_type="self" slicing; that was disproven by inspection: single-ControlNet residuals (controlnet_module.py build_unet_hook) are the ControlNet engine's own persistent, contiguous, 256-aligned output buffers used unmodified, so _staging_action never takes the copy-fallback path for them - re-enabling zero-copy on that assumption would very likely have re-broken ControlNet. The staging bind->copy gap is real, however, for any zero-copy input that CAN take that fallback path (kvo/fio today): _staging_action returned "copy" on fallback with no signal to reset a live CUDA graph, so the graph kept replaying a stale bound address. Added a "copy_and_reset" outcome (utilities.py _staging_action + Engine.infer) as decoupled latent hardening - copies fresh data AND forces one graph re-capture, then converges to plain "copy" the following frame. This does not re-add ControlNet names to _zero_copy_names. Regression test: test_zero_copy_staging_5_6.py locks the exact decision that regressed (a previously-bound name falling back to copy while a graph is live must now return "copy_and_reset", not silently "copy"), plus coverage for the non-contiguous fallback variant and the never-bound/no-graph no-op cases. 139/139 unit tests pass (up from 136: +3 new/updated staging cases). * chore: add Claude review workflow to PR2 branch for head-ref validation * chore: widen Claude review allowedTools (Skill, python/pytest, /tmp Write) to unblock permission-denied max-turns failures * chore: raise Claude review max-turns to 60, discourage filesystem-wide find and output redirection * fix: correct NSFW verdict-to-frame attribution and guard pin_memory on CPU-only builds _apply_safety_checker previously read the prior frame's async verdict but applied it to the current frame's pixels, letting an isolated NSFW frame leak through and wrongly blanking the next clean frame. Buffer the raw frame in _pending_frame and emit it once its own verdict lands (+1 frame latency, first call now emits a black startup frame instead of raw pixels). Also guard .pin_memory() behind torch.cuda.is_available() so the CPU-only test contract in test_safety_checker.py holds. * chore: add Claude review workflow to PR3 branch for head-ref validation --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
forkni
added a commit
that referenced
this pull request
Jul 19, 2026
* refactor: re-apply Phase 1 perf/best-practices cleanup (cosmetic + fail-fast) Re-applies the previously-reverted Phase 1 batch from the perf/best-practices audit: fix the FP8 Q/DQ gate comment to match the actual threshold, convert a dict() call to a dict literal (clears the one pre-existing ruff C408 on a touched file), require an explicit opt_batch_size in EngineBuilder.build() and its 5 compile_* wrappers (real callers already always pass it), drop a dead try/except around a single logger.info call, and tighten two type hints in StreamDiffusion.pipeline (Optional/Union/Tuple). No behavior change except the opt_batch_size fail-fast, which only affects callers that previously relied on an unused default of 1. * refactor: Phase 2 exception hygiene and observability fixes - Add shared _is_oom_error() helper (typed torch.cuda.OutOfMemoryError + string-heuristic fallback) and use it at both TensorRT UNet/VAE engine build fallback sites instead of duplicated inline substring checks. - Log swallowed LoRA candidate-weight-name failures in _load_lora_with_offline_fallback instead of silently continuing. - Log the inner fallback failure in advanced-model-detection instead of a bare except/pass. - Hoist the optional diffusers_ipadapter helper imports in IPAdapterModule.build_unet_hook out of the per-frame hook into a single build-time resolution, and log all previously-silent per-step fallback branches for observability. - Make _load_model fail fast (raise RuntimeError) when an SDXL pipeline retry after a type mismatch also fails, instead of silently continuing with the wrong pipeline type; also null out the stale mismatched `pipe` reference so a later loading-method failure can't let it slip through the final success check. - Add regression tests for _is_oom_error and the SDXL fail-fast path. * chore: migrate deprecated top-level ruff settings to [tool.ruff.lint] Ruff warns that top-level ignore/select/isort/per-file-ignores are deprecated in favour of the lint section. Move them under [tool.ruff.lint] (and .isort / .per-file-ignores); line-length stays at the top level. Verified against cuda-link's pyproject.toml, which already uses this layout. No behavior change: ruff check/format report identical results before and after (same 479 pre-existing repo-wide findings, 0 on the files touched by the Phase 1/2 commits). Investigated adding a matching [tool.pyrefly] section for parity with cuda-link, but did not adopt it: pyrefly falls back to a bundled 'basic' preset when no pyrefly config exists, and that preset is substantially more lenient than explicit config. Adding even an empty [tool.pyrefly] section opts out of it and jumps reported errors from 72 to ~900 (mostly dynamically-set attributes on StreamDiffusion / StreamDiffusionWrapper that were never checked before). Doing this properly needs the same per-module triage cuda-link performed (docs/adr/0005-static-typing-hardening.md) before turning on strict project-includes checking, which is out of scope here. * fix: sync _make_fake_engine test double with TensorRTEngine.__init__ _make_fake_engine (tests/unit/test_trt_engine_guards.py) built engines via TensorRTEngine.__new__() to bypass __init__, but never set _dedicated_stream, _pre_exec_event, _post_exec_event, or _buf_cache -- attributes __init__ has initialized since infer() gained cross-stream sync and LRU shape-cache support. This caused 3 AttributeErrors long treated as "pre-existing, out of scope" failures in every prior phase gate. Not a production bug: __init__ correctly sets all four to None/empty, and infer() guards each with `is not None`. Fix mirrors __init__ in the fake. Full unit suite now fully green (91 passed, 0 failed) with no waived tests. * fix: Phase 3 correctness and resource-safety fixes Four targeted fixes from the perf/best-practices audit, quick-wins #4 and #5: - StreamDiffusion.prepare(): generator defaulted to a pre-constructed torch.Generator(), a mutable default evaluated once at def-time so every instance sharing the default shared one Generator. Default is now None, constructed fresh in the method body. - StreamDiffusionWrapper.prepare(): both runtime stream.prepare() calls (single-prompt and prompt-blending paths) omitted seed, so every runtime prompt change silently reset the RNG to stream.prepare()'s own default. Now forwards seed=getattr(self.stream, "current_seed", 2) to preserve the active seed across prompt changes. - postprocess_image()'s "latent" branch returned an internal decode buffer by reference; callers retaining it across frames would see it mutate. Now clones at this public API boundary. The "np" pinned-buffer alias and the internal __call__/txt2img fast-path aliases are documented as intentional, not cloned (would defeat the pinned-buffer/reuse optimizations). - cleanup_engines_and_rebuild's hardcoded engines_dir = "engines" now honors self._engine_dir when set, matching the existing idiom elsewhere in the file. - cleanup_gpu_memory dropped two explicit Engine.__del__() calls; the method already does del + triple gc.collect() + empty_cache() + ipc_collect() at its tail, and Engine.__del__ is self-guarding, so the explicit dunder calls were redundant. Adds tests/unit/test_phase3_correctness.py (5 regression tests, model-free CPU-only). Full unit suite: 91 passed, 0 failed (no pre-existing failures remain -- see cf3df18). * fix: Phase 4 TensorRT engine-write atomicity and graph/refit robustness Guard TRT engine and timing-cache writes against truncation from an interrupted build via a temp-file + os.replace atomic write helper, matching the idiom already used for FP8 calibration data. Add self-healing recovery around cudaGraphLaunch (reset + fallback to plain execution, re-captures next frame) and a stream-sync guard around the defensive, disabled-by-default Engine.refit() path. Verified: 3 new CPU-only unit tests cover the atomic-write helper's happy path and both failure-preservation cases; a live-GPU smoke run against a cached VAE engine exercised capture, happy-path replay, a forced cudaGraphLaunch failure (recovery + safe fallback, no crash), and self-healed re-capture. * perf: instrument unet_step with profiler.region() spans (Sub-phase 5.0) Adds 5 graph-safe profiler.region() spans (prep/sdxl_cond/hooks/engine/post) inside unet_step to attribute the audit's "~66% unattributed" frame time. Baseline capture (RTX 4090, SDXL-turbo FP8) shows that slice is GPU kernel time inside the single TRT UNet engine call (~93% of frame), not host overhead - every host sub-span measures <0.01ms. Instrumentation only, no behavior change; validated by the existing 94/0 unit suite. * perf: skip redundant set_tensor_address rebind in TRT graph replay (Sub-phase 5.1) Engine.infer() unconditionally re-bound every tensor address via set_tensor_address() before each engine call, even in graphed steady state where the addresses are already baked into the captured CUDA graph (self.tensors buffers are persistent and reused via copy_() - see allocate_buffers/_can_reuse_buffers, which resets cuda_graph_instance to None whenever a buffer is actually reallocated). Skip the rebind loop once a graph is captured; only rebind on first capture or after a reset_cuda_graph() recovery. Verified live on the RTX 4090 td_config.yaml runtime path (the UNet engine loads with use_cuda_graph=True unconditionally via engine_manager.py's loader): 24-frame smoke run with real graph capture + replay shows no errors and frame timing (p50 32.10ms, mean 47.33ms) matching a pre-fix A/B run (p50 31.74ms, mean 47.00ms) - confirms correctness with the expected ~0 measured perf delta (the audit's own 5.0 baseline showed host-side region time is <0.01ms; this is a hygiene/false-dependency fix, not a throughput win). Suite 94/0; ruff clean; pyrefly 72 errors (baseline, no new). * perf: sync-free output/PIL/IPC paths (Sub-phase 5.2) Three output-side sync/allocation fixes, ordered by real-world impact on the TD path: - 5f (only hot-path site): _ipc_pack_rgba / _ipc_pack_unit_rgba now write into a persistent HWC x4 GPU buffer (alpha set once at allocation, B/G/R channels written in place) instead of allocating a fresh torch.full_like alpha + torch.cat every frame. Safe only because the IPC exporters are forced to blocking export (ADR-0001); comments updated at both _lazy_init_ipc_exporter and _lazy_init_cn_ipc_exporter to reflect the buffer is now persistent, not transient. - 5e: _tensor_to_pil_safe (preprocessing_orchestrator.py) reordered CPU-first, following the in-repo base.py:tensor_to_pil template -- the D2H transfer now happens before the tensor.min()/.max() range-check syncs instead of after, collapsing 3 GPU host syncs down to 1. - 5d (hygiene; TD's IPC output path never hits this): _tensor_to_pil_optimized now routes through the shared _output_pin_buf/_d2h_event pinned-buffer + Event machinery instead of a blocking, unpinned .cpu() into pageable memory. Documented the same buffer-reuse caveat as the "np" path (PIL images returned wrap a view of the pinned buffer; callers retaining them across frames must .copy()). Verified via tests/unit/test_sync_free_output_5_2.py: byte-identical parity against frozen pre-5.2 reference formulas for all three sites, plus persistent-buffer reuse/realloc and independent-buffer-identity checks (10 new tests, GPU-gated). Suite 104/0 (94 baseline + 10 new); ruff clean on touched files; pyrefly 72 errors (baseline, no new). * fix: batch-dynamic engines skip l2tc tiling, silencing benign VALIDATE FAIL build_engine() passed dynamic_shapes=build_dynamic_shape into Engine.build(), which tracks only dynamic resolution. On the default "Flexible" preset the UNet is resolution-static but batch-dynamic (build_static_batch=False), so dynamic_shapes was passed False, and _apply_gpu_profile_to_config's tiling branch ran against a graph that still has a symbolic batch dim -- TRT emitted "[l2tc] VALIDATE FAIL - Graph contains symbolic shape" as a no-op for every applicable layer, for the entire ~11.5 min production TD session analyzed this round. Fix: dynamic_shapes=build_dynamic_shape or not build_static_batch (one line, utilities.py:1372) -- True whenever any dim, resolution or batch, is symbolic, matching l2tc's actual requirement (ALL dims must be concrete). Fully-static engines (e.g. ControlNet: build_static_batch=True, build_dynamic_shape=False) are unaffected -- still get tiling as before. Tightened the _apply_gpu_profile_to_config docstring to match. Verified via new tests/unit/test_l2tc_dynamic_shapes.py: calls build_engine() with GPU detection/memory query/the real TRT build all monkeypatched out (no CUDA/TRT context required) and asserts the dynamic_shapes kwarg it wires into Engine.build() for the previously-broken case (batch-dynamic, resolution-static -> True), the unaffected fully-static case (-> False), and the already-working resolution-dynamic case (-> True). Suite 107/0 (104 baseline + 3 new); ruff clean; pyrefly 72 errors (baseline, no new). Impact is log-spam-only -- zero perf/numerical change, visible on the next engine rebuild. * perf: elide trt.input_staging DtoD copy for kvo/fio UNet cache inputs (Sub-phase 5.6) Engine.infer() copied every feed_dict tensor into its own persistent staging buffer each frame solely to give TensorRT a stable contiguous address to bind. nsys profiling (5.0/5.5) attributed ~9% of frame time (p95 3.1ms, ~161 DtoD copies/frame) to this region on the UNet call, dominated by the kvo/fio StreamV2V cache inputs. Those cache tensors are already persistent, address-stable, TRT-contiguous buffers (create_kvo_cache/create_fi_cache build them with layers_in_bucket as the outermost dim specifically for this) — the only event that moves their address is a batch-size change, which already forces a full buffer realloc+graph reset today. So copying them every frame is pure waste. Add an opt-in zero_copy_names parameter to Engine.infer() and a pure _staging_action() decision helper (copy/bind/bind_and_reset) that lets TensorRT bind directly to the caller's tensor instead of copying into self.tensors[name], guarded by contiguity and dtype-match fallbacks. Wire it in UNet2DConditionModelEngine for the kvo/fio input names only; every other caller (VAE, ControlNet, safety, preprocessing) passes nothing and keeps the original copy path byte-for-byte. * fix: lower ControlNet dynamic-shape floor 384->256 via shared min_image_shape (bug #6) ControlNet's dynamic-shape get_input_profile() hardcoded a 384px floor independent of the UNet's 256px floor (BaseModel.min_image_shape), so any resolution in [256, 384) hard-failed at Engine.allocate_buffers the moment ControlNet was enabled. Derive the ControlNet floor/ceiling from the same BaseModel.min_image_shape/max_image_shape the UNet already uses (SDXL inherits via super()), lower the matching engine_manager.py build-option default, and relabel the ControlNet dynamic-engine cache directory (--dyn-384-1024 -> --dyn-256-1024) so a re-floored build never collides with a stale 384-floored cache. Trade-off: widening the dynamic profile can slightly reduce TRT tactic specialization at the optimum resolution -- acceptable given the alternative is a hard failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: normalize blend weights on CPU + persistent ControlNet conditioning_scale scalar (Sub-phase 5.4) Two per-frame GPU creation syncs removed: - _normalize_weights built a CUDA tensor from a Python list every call, but every consumer (.item()/.tolist() in slerp/multi_slerp/cosine_weighted paths, float*cuda_tensor in the linear paths) immediately reads it back or treats it as a scalar operand. Build it on CPU float32 instead -- same consumers work unchanged (a 0-dim CPU tensor is treated as a wrapped scalar when multiplied against a CUDA tensor), the sync and readback are both gone, and float32 is more precise than the model's fp16 for the normalization divide. - ControlNetModelEngine rebuilt a conditioning_scale tensor via torch.tensor(...) on every __call__. Replace with one lazily-allocated persistent scalar updated via fill_() (an async kernel launch, no sync), mirroring the existing _fi_strength_tensor.fill_() idiom. Each active ControlNet has its own engine instance, so a per-instance buffer is correct; the binding rank (_cs_rank1) is fixed at load time so the shape never changes underneath it. Also drops three dead variable assignments and fixes one out-of-order import in stream_parameter_updater.py (pre-existing lint debt in a file this change already touches; the repo's commit gate lints the whole staged file, not just the diff). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 1-frame-delayed async NSFW safety-checker readback (Sub-phase 5.3) Removes the cudaStreamSynchronize that NSFWDetectorEngine.__call__ forced on the hot path via probs[0,0].item() whenever use_safety_checker=True. The classifier now writes its GPU probability into a pinned host scalar via a non_blocking copy; _apply_safety_checker reads the *previous* frame's pinned verdict before launching this frame's classification, mirroring the existing SimilarImageFilter 1-frame-delayed readback pattern (image_filter.py). Edge cases: first frame passes through unconditionally (no prior verdict to act on); a 1-frame detection-leak window is possible before flagging lands. Both are acceptable for an opt-in, off-by-default, TensorRT-only feature. Updated tests/unit/test_safety_checker.py for the new (tensor, prob_pin) -> None checker contract and the delayed-readback semantics (10/10 passing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: persistent multi-ControlNet residual merge buffer (Phase-2 prep) The multi-CN merge previously reallocated `merged_down`/`merged_mid` every frame (`merged_down[j] = merged_down[j] + ds[j]`), so residual pointers changed every frame. That would thrash a CUDA graph if these tensors are later bound zero-copy into the UNet's input_control_* inputs (Phase-2 D2). Replace it with lazily-allocated, shape-guarded persistent buffers (_cn_merged_down/_cn_merged_mid) that accumulate in place via copy_/add_, reallocating only on a genuine shape/dtype/device change. Does not alias down_samples_list[0]/mid_samples_list[0] (engine A's own persistent output buffers). Numerically identical to the old sum; single-ControlNet path is unaffected (already address-stable, no merge buffer allocated). Also removes pre-existing lint debt in the same file (duplicate OrchestratorUser import, unused base_kwargs dict), required to pass the commit gate; verified via git stash that both predated this change. Adds tests/unit/test_controlnet_residual_merge.py covering numerical correctness, cross-frame pointer stability, reallocation on shape change, non-aliasing of engine A's buffer, single-CN bypass, and install() reset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 256-byte alignment guard for zero-copy bind path (audit M2) setTensorAddress requires >=256-byte alignment; _staging_action's zero-copy eligibility test didn't enforce it, relying on an arithmetic proof instead of a runtime invariant. Add cur_ptr % 256 == 0 to the eligibility check so a mis-aligned pointer always falls back to the safe copy path. Real torch CUDA allocations are >=512-byte aligned, so this guard never fires in production — it only defends against future config drift, and is a prerequisite for Phase-2 D2's new zero-copy input_control_* bindings. Realigns the test suite's PTR_A/PTR_B fixtures to 256-byte-aligned values (the old 0xDEAD_BEEF/0xFEED_FACE constants were both mis-aligned and would have broken under the new guard) and adds two rows asserting the misaligned fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: zero-copy bind ControlNet residual UNet inputs (Phase-2 D2) Widen UNet2DConditionModelEngine._zero_copy_names to include input_control_00..19 and input_control_middle alongside the existing kvo/fio cache names, so Engine.infer() binds TensorRT directly to the ControlNet residual tensors instead of DtoD-copying them into a staging buffer every frame (the Sub-phase 5.6 mechanism, now audit-M2 alignment guarded). In steady state the tensors placed in input_dict[input_control_*] are the same persistent objects each frame: multi-ControlNet uses D1's persistent merge buffers (controlnet_module.py), single-ControlNet reuses the engine's own persistent output buffers, and idle frames reuse the persistent dummy zero cache. Pointers only change on a resolution/batch/model change or the idle<->active toggle, which _staging_action already handles via bind_and_reset (one graph re-capture on the toggle frame, not per-frame). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor: harden TRT runtime API usage (audit M1/H1/H2) M1: TensorRTEngine (trt_base.py) ignored the bool return of set_input_shape (allocate_buffers, infer) and set_tensor_address (infer). An out-of-profile shape or invalid address was silently accepted, surfacing later as an execute_async_v3 failure or garbage output. Mirror the core Engine pattern (utilities.py) and raise RuntimeError with the tensor name/shape when either call returns False. Affects depth/pose (+HED/NormalBae via the shared base). H1: replace the try/except (AttributeError, TypeError) around the SM_120 TacticSource scoping block with an explicit hasattr(trt.TacticSource, "CUBLAS") gate. CUBLAS/CUBLAS_LT are removed in TRT 11, so the old except would silently swallow the AttributeError and drop the scoping intent without a trace; the new branch logs the TRT>=11 case explicitly. No behavior change on the installed TRT 10.16 (all members still present). H2: delete the dead reuse_device_memory branch in Engine.activate() — no caller across any of the ~10 activate() call sites passes it, and the one-arg context.device_memory setter it used has been superseded since TRT 10.1. Migrating to set_device_memory(ptr, engine.device_memory_size_v2) is deferred until device-memory reuse is actually wanted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor: modernize ONNX compile/export path — create_network(), drop deprecated export flag, robust external-data detection (audit M3/L1/L3) M3: replace deprecated create_network(1<<EXPLICIT_BATCH) with create_network() in compile_depth_anything_tensorrt.py, mirroring compile_raft_tensorrt.py. Also swaps the unused-import onnx availability check for importlib.util.find_spec (ruff F401 in the same file's try/except block). L1: drop the deprecated do_constant_folding=True kwarg from the torch.onnx.export call in utilities.py; dynamo=False stays explicit (audit L2: no action needed). L3: replace the ".pb"-suffix directory scan in optimize_onnx() with a check of TensorProto.data_location == EXTERNAL on the loaded (but not yet materialized) model, removing the filename-heuristic's dependence on export_onnx's consolidate-to-weights.pb ordering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: make ControlNet zero-copy revert permanent; harden staging bind->copy gap Post-mortem: f8ff50f ("zero-copy bind ControlNet residual UNet inputs") caused a confirmed regression - ControlNet produced no visual change for both scribble (ramp 0->0.36) and Canny (static 1.0), at any scale. An interim revert (dropping input_control_* from the UNet engine's _zero_copy_names) was verified working on the rig for both models. This commit makes that revert permanent rather than re-enabling the optimization. Root cause, confirmed by code inspection (not just bisection): a zero-copy bind drops the copy_() call that runs on the engine stream and orders the ControlNet engine's residual write before the UNet CUDA graph's read of it ("copy_() executes on engine stream to guarantee ordering" - utilities.py). An earlier hypothesis assumed the failure was a bind->copy staging fallback triggered by cfg_type="self" slicing; that was disproven by inspection: single-ControlNet residuals (controlnet_module.py build_unet_hook) are the ControlNet engine's own persistent, contiguous, 256-aligned output buffers used unmodified, so _staging_action never takes the copy-fallback path for them - re-enabling zero-copy on that assumption would very likely have re-broken ControlNet. The staging bind->copy gap is real, however, for any zero-copy input that CAN take that fallback path (kvo/fio today): _staging_action returned "copy" on fallback with no signal to reset a live CUDA graph, so the graph kept replaying a stale bound address. Added a "copy_and_reset" outcome (utilities.py _staging_action + Engine.infer) as decoupled latent hardening - copies fresh data AND forces one graph re-capture, then converges to plain "copy" the following frame. This does not re-add ControlNet names to _zero_copy_names. Regression test: test_zero_copy_staging_5_6.py locks the exact decision that regressed (a previously-bound name falling back to copy while a graph is live must now return "copy_and_reset", not silently "copy"), plus coverage for the non-contiguous fallback variant and the never-bound/no-graph no-op cases. 139/139 unit tests pass (up from 136: +3 new/updated staging cases). * fix: enforce keyword-only StreamParameterUpdater binding; correct seed interpolation kwarg * perf: FP8 build disk-space preflight and hardened intermediate cleanup * style: apply ruff format to builder.py (CGW restage gap left prior commit unformatted) * refactor: centralize wrapper/updater params in param_schema (single source of truth) - add src/streamdiffusion/param_schema.py: 25 wrapper / 23 updater param names, construction-time DEFAULTS (t_index_list as immutable tuple, G3), and shared floor_num_inference_steps / rescale_t_index_list helpers - wrapper.py & stream_parameter_updater.py: source interpolation-method Literals and floor/rescale arithmetic from the schema; preserve branch-specific warnings (G2); fix D1 comment off-by-one - config.py: delegate 12 defaults across both _extract_wrapper_params and _extract_prepare_params to param_schema.DEFAULTS (G1); drop dead _validate_pipeline_hook_configs (zero callers) - add drift-lock + golden/parity tests (test_param_schema.py, test_config_extraction_golden.py, test_td_pending_params.py); tests/unit/: 165 passed * chore: add Claude review workflow to PR2 branch for head-ref validation * chore: widen Claude review allowedTools (Skill, python/pytest, /tmp Write) to unblock permission-denied max-turns failures * chore: raise Claude review max-turns to 60, discourage filesystem-wide find and output redirection * fix: correct NSFW verdict-to-frame attribution and guard pin_memory on CPU-only builds _apply_safety_checker previously read the prior frame's async verdict but applied it to the current frame's pixels, letting an isolated NSFW frame leak through and wrongly blanking the next clean frame. Buffer the raw frame in _pending_frame and emit it once its own verdict lands (+1 frame latency, first call now emits a black startup frame instead of raw pixels). Also guard .pin_memory() behind torch.cuda.is_available() so the CPU-only test contract in test_safety_checker.py holds. * chore: add Claude review workflow to PR3 branch for head-ref validation * chore: add Claude review workflow to PR4 branch for head-ref validation --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
forkni
added a commit
that referenced
this pull request
Jul 19, 2026
* refactor: re-apply Phase 1 perf/best-practices cleanup (cosmetic + fail-fast) Re-applies the previously-reverted Phase 1 batch from the perf/best-practices audit: fix the FP8 Q/DQ gate comment to match the actual threshold, convert a dict() call to a dict literal (clears the one pre-existing ruff C408 on a touched file), require an explicit opt_batch_size in EngineBuilder.build() and its 5 compile_* wrappers (real callers already always pass it), drop a dead try/except around a single logger.info call, and tighten two type hints in StreamDiffusion.pipeline (Optional/Union/Tuple). No behavior change except the opt_batch_size fail-fast, which only affects callers that previously relied on an unused default of 1. * refactor: Phase 2 exception hygiene and observability fixes - Add shared _is_oom_error() helper (typed torch.cuda.OutOfMemoryError + string-heuristic fallback) and use it at both TensorRT UNet/VAE engine build fallback sites instead of duplicated inline substring checks. - Log swallowed LoRA candidate-weight-name failures in _load_lora_with_offline_fallback instead of silently continuing. - Log the inner fallback failure in advanced-model-detection instead of a bare except/pass. - Hoist the optional diffusers_ipadapter helper imports in IPAdapterModule.build_unet_hook out of the per-frame hook into a single build-time resolution, and log all previously-silent per-step fallback branches for observability. - Make _load_model fail fast (raise RuntimeError) when an SDXL pipeline retry after a type mismatch also fails, instead of silently continuing with the wrong pipeline type; also null out the stale mismatched `pipe` reference so a later loading-method failure can't let it slip through the final success check. - Add regression tests for _is_oom_error and the SDXL fail-fast path. * chore: migrate deprecated top-level ruff settings to [tool.ruff.lint] Ruff warns that top-level ignore/select/isort/per-file-ignores are deprecated in favour of the lint section. Move them under [tool.ruff.lint] (and .isort / .per-file-ignores); line-length stays at the top level. Verified against cuda-link's pyproject.toml, which already uses this layout. No behavior change: ruff check/format report identical results before and after (same 479 pre-existing repo-wide findings, 0 on the files touched by the Phase 1/2 commits). Investigated adding a matching [tool.pyrefly] section for parity with cuda-link, but did not adopt it: pyrefly falls back to a bundled 'basic' preset when no pyrefly config exists, and that preset is substantially more lenient than explicit config. Adding even an empty [tool.pyrefly] section opts out of it and jumps reported errors from 72 to ~900 (mostly dynamically-set attributes on StreamDiffusion / StreamDiffusionWrapper that were never checked before). Doing this properly needs the same per-module triage cuda-link performed (docs/adr/0005-static-typing-hardening.md) before turning on strict project-includes checking, which is out of scope here. * fix: sync _make_fake_engine test double with TensorRTEngine.__init__ _make_fake_engine (tests/unit/test_trt_engine_guards.py) built engines via TensorRTEngine.__new__() to bypass __init__, but never set _dedicated_stream, _pre_exec_event, _post_exec_event, or _buf_cache -- attributes __init__ has initialized since infer() gained cross-stream sync and LRU shape-cache support. This caused 3 AttributeErrors long treated as "pre-existing, out of scope" failures in every prior phase gate. Not a production bug: __init__ correctly sets all four to None/empty, and infer() guards each with `is not None`. Fix mirrors __init__ in the fake. Full unit suite now fully green (91 passed, 0 failed) with no waived tests. * fix: Phase 3 correctness and resource-safety fixes Four targeted fixes from the perf/best-practices audit, quick-wins #4 and #5: - StreamDiffusion.prepare(): generator defaulted to a pre-constructed torch.Generator(), a mutable default evaluated once at def-time so every instance sharing the default shared one Generator. Default is now None, constructed fresh in the method body. - StreamDiffusionWrapper.prepare(): both runtime stream.prepare() calls (single-prompt and prompt-blending paths) omitted seed, so every runtime prompt change silently reset the RNG to stream.prepare()'s own default. Now forwards seed=getattr(self.stream, "current_seed", 2) to preserve the active seed across prompt changes. - postprocess_image()'s "latent" branch returned an internal decode buffer by reference; callers retaining it across frames would see it mutate. Now clones at this public API boundary. The "np" pinned-buffer alias and the internal __call__/txt2img fast-path aliases are documented as intentional, not cloned (would defeat the pinned-buffer/reuse optimizations). - cleanup_engines_and_rebuild's hardcoded engines_dir = "engines" now honors self._engine_dir when set, matching the existing idiom elsewhere in the file. - cleanup_gpu_memory dropped two explicit Engine.__del__() calls; the method already does del + triple gc.collect() + empty_cache() + ipc_collect() at its tail, and Engine.__del__ is self-guarding, so the explicit dunder calls were redundant. Adds tests/unit/test_phase3_correctness.py (5 regression tests, model-free CPU-only). Full unit suite: 91 passed, 0 failed (no pre-existing failures remain -- see cf3df18). * fix: Phase 4 TensorRT engine-write atomicity and graph/refit robustness Guard TRT engine and timing-cache writes against truncation from an interrupted build via a temp-file + os.replace atomic write helper, matching the idiom already used for FP8 calibration data. Add self-healing recovery around cudaGraphLaunch (reset + fallback to plain execution, re-captures next frame) and a stream-sync guard around the defensive, disabled-by-default Engine.refit() path. Verified: 3 new CPU-only unit tests cover the atomic-write helper's happy path and both failure-preservation cases; a live-GPU smoke run against a cached VAE engine exercised capture, happy-path replay, a forced cudaGraphLaunch failure (recovery + safe fallback, no crash), and self-healed re-capture. * perf: instrument unet_step with profiler.region() spans (Sub-phase 5.0) Adds 5 graph-safe profiler.region() spans (prep/sdxl_cond/hooks/engine/post) inside unet_step to attribute the audit's "~66% unattributed" frame time. Baseline capture (RTX 4090, SDXL-turbo FP8) shows that slice is GPU kernel time inside the single TRT UNet engine call (~93% of frame), not host overhead - every host sub-span measures <0.01ms. Instrumentation only, no behavior change; validated by the existing 94/0 unit suite. * perf: skip redundant set_tensor_address rebind in TRT graph replay (Sub-phase 5.1) Engine.infer() unconditionally re-bound every tensor address via set_tensor_address() before each engine call, even in graphed steady state where the addresses are already baked into the captured CUDA graph (self.tensors buffers are persistent and reused via copy_() - see allocate_buffers/_can_reuse_buffers, which resets cuda_graph_instance to None whenever a buffer is actually reallocated). Skip the rebind loop once a graph is captured; only rebind on first capture or after a reset_cuda_graph() recovery. Verified live on the RTX 4090 td_config.yaml runtime path (the UNet engine loads with use_cuda_graph=True unconditionally via engine_manager.py's loader): 24-frame smoke run with real graph capture + replay shows no errors and frame timing (p50 32.10ms, mean 47.33ms) matching a pre-fix A/B run (p50 31.74ms, mean 47.00ms) - confirms correctness with the expected ~0 measured perf delta (the audit's own 5.0 baseline showed host-side region time is <0.01ms; this is a hygiene/false-dependency fix, not a throughput win). Suite 94/0; ruff clean; pyrefly 72 errors (baseline, no new). * perf: sync-free output/PIL/IPC paths (Sub-phase 5.2) Three output-side sync/allocation fixes, ordered by real-world impact on the TD path: - 5f (only hot-path site): _ipc_pack_rgba / _ipc_pack_unit_rgba now write into a persistent HWC x4 GPU buffer (alpha set once at allocation, B/G/R channels written in place) instead of allocating a fresh torch.full_like alpha + torch.cat every frame. Safe only because the IPC exporters are forced to blocking export (ADR-0001); comments updated at both _lazy_init_ipc_exporter and _lazy_init_cn_ipc_exporter to reflect the buffer is now persistent, not transient. - 5e: _tensor_to_pil_safe (preprocessing_orchestrator.py) reordered CPU-first, following the in-repo base.py:tensor_to_pil template -- the D2H transfer now happens before the tensor.min()/.max() range-check syncs instead of after, collapsing 3 GPU host syncs down to 1. - 5d (hygiene; TD's IPC output path never hits this): _tensor_to_pil_optimized now routes through the shared _output_pin_buf/_d2h_event pinned-buffer + Event machinery instead of a blocking, unpinned .cpu() into pageable memory. Documented the same buffer-reuse caveat as the "np" path (PIL images returned wrap a view of the pinned buffer; callers retaining them across frames must .copy()). Verified via tests/unit/test_sync_free_output_5_2.py: byte-identical parity against frozen pre-5.2 reference formulas for all three sites, plus persistent-buffer reuse/realloc and independent-buffer-identity checks (10 new tests, GPU-gated). Suite 104/0 (94 baseline + 10 new); ruff clean on touched files; pyrefly 72 errors (baseline, no new). * fix: batch-dynamic engines skip l2tc tiling, silencing benign VALIDATE FAIL build_engine() passed dynamic_shapes=build_dynamic_shape into Engine.build(), which tracks only dynamic resolution. On the default "Flexible" preset the UNet is resolution-static but batch-dynamic (build_static_batch=False), so dynamic_shapes was passed False, and _apply_gpu_profile_to_config's tiling branch ran against a graph that still has a symbolic batch dim -- TRT emitted "[l2tc] VALIDATE FAIL - Graph contains symbolic shape" as a no-op for every applicable layer, for the entire ~11.5 min production TD session analyzed this round. Fix: dynamic_shapes=build_dynamic_shape or not build_static_batch (one line, utilities.py:1372) -- True whenever any dim, resolution or batch, is symbolic, matching l2tc's actual requirement (ALL dims must be concrete). Fully-static engines (e.g. ControlNet: build_static_batch=True, build_dynamic_shape=False) are unaffected -- still get tiling as before. Tightened the _apply_gpu_profile_to_config docstring to match. Verified via new tests/unit/test_l2tc_dynamic_shapes.py: calls build_engine() with GPU detection/memory query/the real TRT build all monkeypatched out (no CUDA/TRT context required) and asserts the dynamic_shapes kwarg it wires into Engine.build() for the previously-broken case (batch-dynamic, resolution-static -> True), the unaffected fully-static case (-> False), and the already-working resolution-dynamic case (-> True). Suite 107/0 (104 baseline + 3 new); ruff clean; pyrefly 72 errors (baseline, no new). Impact is log-spam-only -- zero perf/numerical change, visible on the next engine rebuild. * perf: elide trt.input_staging DtoD copy for kvo/fio UNet cache inputs (Sub-phase 5.6) Engine.infer() copied every feed_dict tensor into its own persistent staging buffer each frame solely to give TensorRT a stable contiguous address to bind. nsys profiling (5.0/5.5) attributed ~9% of frame time (p95 3.1ms, ~161 DtoD copies/frame) to this region on the UNet call, dominated by the kvo/fio StreamV2V cache inputs. Those cache tensors are already persistent, address-stable, TRT-contiguous buffers (create_kvo_cache/create_fi_cache build them with layers_in_bucket as the outermost dim specifically for this) — the only event that moves their address is a batch-size change, which already forces a full buffer realloc+graph reset today. So copying them every frame is pure waste. Add an opt-in zero_copy_names parameter to Engine.infer() and a pure _staging_action() decision helper (copy/bind/bind_and_reset) that lets TensorRT bind directly to the caller's tensor instead of copying into self.tensors[name], guarded by contiguity and dtype-match fallbacks. Wire it in UNet2DConditionModelEngine for the kvo/fio input names only; every other caller (VAE, ControlNet, safety, preprocessing) passes nothing and keeps the original copy path byte-for-byte. * fix: lower ControlNet dynamic-shape floor 384->256 via shared min_image_shape (bug #6) ControlNet's dynamic-shape get_input_profile() hardcoded a 384px floor independent of the UNet's 256px floor (BaseModel.min_image_shape), so any resolution in [256, 384) hard-failed at Engine.allocate_buffers the moment ControlNet was enabled. Derive the ControlNet floor/ceiling from the same BaseModel.min_image_shape/max_image_shape the UNet already uses (SDXL inherits via super()), lower the matching engine_manager.py build-option default, and relabel the ControlNet dynamic-engine cache directory (--dyn-384-1024 -> --dyn-256-1024) so a re-floored build never collides with a stale 384-floored cache. Trade-off: widening the dynamic profile can slightly reduce TRT tactic specialization at the optimum resolution -- acceptable given the alternative is a hard failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: normalize blend weights on CPU + persistent ControlNet conditioning_scale scalar (Sub-phase 5.4) Two per-frame GPU creation syncs removed: - _normalize_weights built a CUDA tensor from a Python list every call, but every consumer (.item()/.tolist() in slerp/multi_slerp/cosine_weighted paths, float*cuda_tensor in the linear paths) immediately reads it back or treats it as a scalar operand. Build it on CPU float32 instead -- same consumers work unchanged (a 0-dim CPU tensor is treated as a wrapped scalar when multiplied against a CUDA tensor), the sync and readback are both gone, and float32 is more precise than the model's fp16 for the normalization divide. - ControlNetModelEngine rebuilt a conditioning_scale tensor via torch.tensor(...) on every __call__. Replace with one lazily-allocated persistent scalar updated via fill_() (an async kernel launch, no sync), mirroring the existing _fi_strength_tensor.fill_() idiom. Each active ControlNet has its own engine instance, so a per-instance buffer is correct; the binding rank (_cs_rank1) is fixed at load time so the shape never changes underneath it. Also drops three dead variable assignments and fixes one out-of-order import in stream_parameter_updater.py (pre-existing lint debt in a file this change already touches; the repo's commit gate lints the whole staged file, not just the diff). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 1-frame-delayed async NSFW safety-checker readback (Sub-phase 5.3) Removes the cudaStreamSynchronize that NSFWDetectorEngine.__call__ forced on the hot path via probs[0,0].item() whenever use_safety_checker=True. The classifier now writes its GPU probability into a pinned host scalar via a non_blocking copy; _apply_safety_checker reads the *previous* frame's pinned verdict before launching this frame's classification, mirroring the existing SimilarImageFilter 1-frame-delayed readback pattern (image_filter.py). Edge cases: first frame passes through unconditionally (no prior verdict to act on); a 1-frame detection-leak window is possible before flagging lands. Both are acceptable for an opt-in, off-by-default, TensorRT-only feature. Updated tests/unit/test_safety_checker.py for the new (tensor, prob_pin) -> None checker contract and the delayed-readback semantics (10/10 passing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: persistent multi-ControlNet residual merge buffer (Phase-2 prep) The multi-CN merge previously reallocated `merged_down`/`merged_mid` every frame (`merged_down[j] = merged_down[j] + ds[j]`), so residual pointers changed every frame. That would thrash a CUDA graph if these tensors are later bound zero-copy into the UNet's input_control_* inputs (Phase-2 D2). Replace it with lazily-allocated, shape-guarded persistent buffers (_cn_merged_down/_cn_merged_mid) that accumulate in place via copy_/add_, reallocating only on a genuine shape/dtype/device change. Does not alias down_samples_list[0]/mid_samples_list[0] (engine A's own persistent output buffers). Numerically identical to the old sum; single-ControlNet path is unaffected (already address-stable, no merge buffer allocated). Also removes pre-existing lint debt in the same file (duplicate OrchestratorUser import, unused base_kwargs dict), required to pass the commit gate; verified via git stash that both predated this change. Adds tests/unit/test_controlnet_residual_merge.py covering numerical correctness, cross-frame pointer stability, reallocation on shape change, non-aliasing of engine A's buffer, single-CN bypass, and install() reset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 256-byte alignment guard for zero-copy bind path (audit M2) setTensorAddress requires >=256-byte alignment; _staging_action's zero-copy eligibility test didn't enforce it, relying on an arithmetic proof instead of a runtime invariant. Add cur_ptr % 256 == 0 to the eligibility check so a mis-aligned pointer always falls back to the safe copy path. Real torch CUDA allocations are >=512-byte aligned, so this guard never fires in production — it only defends against future config drift, and is a prerequisite for Phase-2 D2's new zero-copy input_control_* bindings. Realigns the test suite's PTR_A/PTR_B fixtures to 256-byte-aligned values (the old 0xDEAD_BEEF/0xFEED_FACE constants were both mis-aligned and would have broken under the new guard) and adds two rows asserting the misaligned fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: zero-copy bind ControlNet residual UNet inputs (Phase-2 D2) Widen UNet2DConditionModelEngine._zero_copy_names to include input_control_00..19 and input_control_middle alongside the existing kvo/fio cache names, so Engine.infer() binds TensorRT directly to the ControlNet residual tensors instead of DtoD-copying them into a staging buffer every frame (the Sub-phase 5.6 mechanism, now audit-M2 alignment guarded). In steady state the tensors placed in input_dict[input_control_*] are the same persistent objects each frame: multi-ControlNet uses D1's persistent merge buffers (controlnet_module.py), single-ControlNet reuses the engine's own persistent output buffers, and idle frames reuse the persistent dummy zero cache. Pointers only change on a resolution/batch/model change or the idle<->active toggle, which _staging_action already handles via bind_and_reset (one graph re-capture on the toggle frame, not per-frame). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor: harden TRT runtime API usage (audit M1/H1/H2) M1: TensorRTEngine (trt_base.py) ignored the bool return of set_input_shape (allocate_buffers, infer) and set_tensor_address (infer). An out-of-profile shape or invalid address was silently accepted, surfacing later as an execute_async_v3 failure or garbage output. Mirror the core Engine pattern (utilities.py) and raise RuntimeError with the tensor name/shape when either call returns False. Affects depth/pose (+HED/NormalBae via the shared base). H1: replace the try/except (AttributeError, TypeError) around the SM_120 TacticSource scoping block with an explicit hasattr(trt.TacticSource, "CUBLAS") gate. CUBLAS/CUBLAS_LT are removed in TRT 11, so the old except would silently swallow the AttributeError and drop the scoping intent without a trace; the new branch logs the TRT>=11 case explicitly. No behavior change on the installed TRT 10.16 (all members still present). H2: delete the dead reuse_device_memory branch in Engine.activate() — no caller across any of the ~10 activate() call sites passes it, and the one-arg context.device_memory setter it used has been superseded since TRT 10.1. Migrating to set_device_memory(ptr, engine.device_memory_size_v2) is deferred until device-memory reuse is actually wanted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor: modernize ONNX compile/export path — create_network(), drop deprecated export flag, robust external-data detection (audit M3/L1/L3) M3: replace deprecated create_network(1<<EXPLICIT_BATCH) with create_network() in compile_depth_anything_tensorrt.py, mirroring compile_raft_tensorrt.py. Also swaps the unused-import onnx availability check for importlib.util.find_spec (ruff F401 in the same file's try/except block). L1: drop the deprecated do_constant_folding=True kwarg from the torch.onnx.export call in utilities.py; dynamo=False stays explicit (audit L2: no action needed). L3: replace the ".pb"-suffix directory scan in optimize_onnx() with a check of TensorProto.data_location == EXTERNAL on the loaded (but not yet materialized) model, removing the filename-heuristic's dependence on export_onnx's consolidate-to-weights.pb ordering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: make ControlNet zero-copy revert permanent; harden staging bind->copy gap Post-mortem: f8ff50f ("zero-copy bind ControlNet residual UNet inputs") caused a confirmed regression - ControlNet produced no visual change for both scribble (ramp 0->0.36) and Canny (static 1.0), at any scale. An interim revert (dropping input_control_* from the UNet engine's _zero_copy_names) was verified working on the rig for both models. This commit makes that revert permanent rather than re-enabling the optimization. Root cause, confirmed by code inspection (not just bisection): a zero-copy bind drops the copy_() call that runs on the engine stream and orders the ControlNet engine's residual write before the UNet CUDA graph's read of it ("copy_() executes on engine stream to guarantee ordering" - utilities.py). An earlier hypothesis assumed the failure was a bind->copy staging fallback triggered by cfg_type="self" slicing; that was disproven by inspection: single-ControlNet residuals (controlnet_module.py build_unet_hook) are the ControlNet engine's own persistent, contiguous, 256-aligned output buffers used unmodified, so _staging_action never takes the copy-fallback path for them - re-enabling zero-copy on that assumption would very likely have re-broken ControlNet. The staging bind->copy gap is real, however, for any zero-copy input that CAN take that fallback path (kvo/fio today): _staging_action returned "copy" on fallback with no signal to reset a live CUDA graph, so the graph kept replaying a stale bound address. Added a "copy_and_reset" outcome (utilities.py _staging_action + Engine.infer) as decoupled latent hardening - copies fresh data AND forces one graph re-capture, then converges to plain "copy" the following frame. This does not re-add ControlNet names to _zero_copy_names. Regression test: test_zero_copy_staging_5_6.py locks the exact decision that regressed (a previously-bound name falling back to copy while a graph is live must now return "copy_and_reset", not silently "copy"), plus coverage for the non-contiguous fallback variant and the never-bound/no-graph no-op cases. 139/139 unit tests pass (up from 136: +3 new/updated staging cases). * fix: enforce keyword-only StreamParameterUpdater binding; correct seed interpolation kwarg * perf: FP8 build disk-space preflight and hardened intermediate cleanup * style: apply ruff format to builder.py (CGW restage gap left prior commit unformatted) * refactor: centralize wrapper/updater params in param_schema (single source of truth) - add src/streamdiffusion/param_schema.py: 25 wrapper / 23 updater param names, construction-time DEFAULTS (t_index_list as immutable tuple, G3), and shared floor_num_inference_steps / rescale_t_index_list helpers - wrapper.py & stream_parameter_updater.py: source interpolation-method Literals and floor/rescale arithmetic from the schema; preserve branch-specific warnings (G2); fix D1 comment off-by-one - config.py: delegate 12 defaults across both _extract_wrapper_params and _extract_prepare_params to param_schema.DEFAULTS (G1); drop dead _validate_pipeline_hook_configs (zero callers) - add drift-lock + golden/parity tests (test_param_schema.py, test_config_extraction_golden.py, test_td_pending_params.py); tests/unit/: 165 passed * style: adopt expanded ruff ruleset + safe auto-fixes repo-wide - pyproject.toml: expand ruff select to E,W,F,I,C4,B,UP,SIM,RUF,PERF,NPY; target py310; add extend-exclude + per-file-ignores; docstring-code-format; add [tool.pyrefly] (baseline=pyrefly-baseline.json, pytorch-efficiency-lints) - apply safe ruff check --fix + ruff format across the repo (mechanical) - ignore non-auto-fixable residual families pending incremental adoption (SIM/PERF/RUF-unicode/RUF012-013/B905/B00x/RUF006-009/RUF022/RUF046/UP008/NPY002) — documented inline - mark 25 pre-existing debt sites (F401 x2, F841 x21, E722 x2) under already-active rules with per-site noqa + TODO comments; zero behavior change * chore: add pyrefly baseline freezing pre-existing type errors * style: whitelist idiomatic call-defaults; re-enable B008/RUF009 - flake8-bugbear.extend-immutable-calls: fastapi.Depends/File + torch.device - drop B008/RUF009 from ignore (FastAPI DI + torch.device dataclass defaults are intentional) * fix: chain exception causes + resolve real-bug lints; re-enable rules - B904: add `raise … from e`/`from None` across src, scripts, and demo routes (47 sites) - B004 callable(); B006 None-sentinel default; B018 discard via `_ =`; B026 reorder *args - RUF006: retain InputManager background-task references (prevent GC cancellation) - NPY002: wrapper random-seed path uses np.random.default_rng().integers - drop the fixed codes from [tool.ruff.lint].ignore (rules now enforce) * fix: harden RealESRGAN fallback loader with torch.load(weights_only=True) - only unguarded torch.load repo-wide; prevents arbitrary-code exec on untrusted checkpoints * chore: add Claude review workflow to PR2 branch for head-ref validation * chore: widen Claude review allowedTools (Skill, python/pytest, /tmp Write) to unblock permission-denied max-turns failures * chore: raise Claude review max-turns to 60, discourage filesystem-wide find and output redirection * fix: correct NSFW verdict-to-frame attribution and guard pin_memory on CPU-only builds _apply_safety_checker previously read the prior frame's async verdict but applied it to the current frame's pixels, letting an isolated NSFW frame leak through and wrongly blanking the next clean frame. Buffer the raw frame in _pending_frame and emit it once its own verdict lands (+1 frame latency, first call now emits a black startup frame instead of raw pixels). Also guard .pin_memory() behind torch.cuda.is_available() so the CPU-only test contract in test_safety_checker.py holds. * chore: add Claude review workflow to PR3 branch for head-ref validation * chore: add Claude review workflow to PR4 branch for head-ref validation * chore: add Claude review workflow to PR5 branch for head-ref validation * chore: fix import ordering in merge-carried diagnostics files --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
forkni
added a commit
that referenced
this pull request
Jul 19, 2026
…s, TRT atomicity) * refactor: re-apply Phase 1 perf/best-practices cleanup (cosmetic + fail-fast) Re-applies the previously-reverted Phase 1 batch from the perf/best-practices audit: fix the FP8 Q/DQ gate comment to match the actual threshold, convert a dict() call to a dict literal (clears the one pre-existing ruff C408 on a touched file), require an explicit opt_batch_size in EngineBuilder.build() and its 5 compile_* wrappers (real callers already always pass it), drop a dead try/except around a single logger.info call, and tighten two type hints in StreamDiffusion.pipeline (Optional/Union/Tuple). No behavior change except the opt_batch_size fail-fast, which only affects callers that previously relied on an unused default of 1. * refactor: Phase 2 exception hygiene and observability fixes - Add shared _is_oom_error() helper (typed torch.cuda.OutOfMemoryError + string-heuristic fallback) and use it at both TensorRT UNet/VAE engine build fallback sites instead of duplicated inline substring checks. - Log swallowed LoRA candidate-weight-name failures in _load_lora_with_offline_fallback instead of silently continuing. - Log the inner fallback failure in advanced-model-detection instead of a bare except/pass. - Hoist the optional diffusers_ipadapter helper imports in IPAdapterModule.build_unet_hook out of the per-frame hook into a single build-time resolution, and log all previously-silent per-step fallback branches for observability. - Make _load_model fail fast (raise RuntimeError) when an SDXL pipeline retry after a type mismatch also fails, instead of silently continuing with the wrong pipeline type; also null out the stale mismatched `pipe` reference so a later loading-method failure can't let it slip through the final success check. - Add regression tests for _is_oom_error and the SDXL fail-fast path. * chore: migrate deprecated top-level ruff settings to [tool.ruff.lint] Ruff warns that top-level ignore/select/isort/per-file-ignores are deprecated in favour of the lint section. Move them under [tool.ruff.lint] (and .isort / .per-file-ignores); line-length stays at the top level. Verified against cuda-link's pyproject.toml, which already uses this layout. No behavior change: ruff check/format report identical results before and after (same 479 pre-existing repo-wide findings, 0 on the files touched by the Phase 1/2 commits). Investigated adding a matching [tool.pyrefly] section for parity with cuda-link, but did not adopt it: pyrefly falls back to a bundled 'basic' preset when no pyrefly config exists, and that preset is substantially more lenient than explicit config. Adding even an empty [tool.pyrefly] section opts out of it and jumps reported errors from 72 to ~900 (mostly dynamically-set attributes on StreamDiffusion / StreamDiffusionWrapper that were never checked before). Doing this properly needs the same per-module triage cuda-link performed (docs/adr/0005-static-typing-hardening.md) before turning on strict project-includes checking, which is out of scope here. * fix: sync _make_fake_engine test double with TensorRTEngine.__init__ _make_fake_engine (tests/unit/test_trt_engine_guards.py) built engines via TensorRTEngine.__new__() to bypass __init__, but never set _dedicated_stream, _pre_exec_event, _post_exec_event, or _buf_cache -- attributes __init__ has initialized since infer() gained cross-stream sync and LRU shape-cache support. This caused 3 AttributeErrors long treated as "pre-existing, out of scope" failures in every prior phase gate. Not a production bug: __init__ correctly sets all four to None/empty, and infer() guards each with `is not None`. Fix mirrors __init__ in the fake. Full unit suite now fully green (91 passed, 0 failed) with no waived tests. * fix: Phase 3 correctness and resource-safety fixes Four targeted fixes from the perf/best-practices audit, quick-wins #4 and #5: - StreamDiffusion.prepare(): generator defaulted to a pre-constructed torch.Generator(), a mutable default evaluated once at def-time so every instance sharing the default shared one Generator. Default is now None, constructed fresh in the method body. - StreamDiffusionWrapper.prepare(): both runtime stream.prepare() calls (single-prompt and prompt-blending paths) omitted seed, so every runtime prompt change silently reset the RNG to stream.prepare()'s own default. Now forwards seed=getattr(self.stream, "current_seed", 2) to preserve the active seed across prompt changes. - postprocess_image()'s "latent" branch returned an internal decode buffer by reference; callers retaining it across frames would see it mutate. Now clones at this public API boundary. The "np" pinned-buffer alias and the internal __call__/txt2img fast-path aliases are documented as intentional, not cloned (would defeat the pinned-buffer/reuse optimizations). - cleanup_engines_and_rebuild's hardcoded engines_dir = "engines" now honors self._engine_dir when set, matching the existing idiom elsewhere in the file. - cleanup_gpu_memory dropped two explicit Engine.__del__() calls; the method already does del + triple gc.collect() + empty_cache() + ipc_collect() at its tail, and Engine.__del__ is self-guarding, so the explicit dunder calls were redundant. Adds tests/unit/test_phase3_correctness.py (5 regression tests, model-free CPU-only). Full unit suite: 91 passed, 0 failed (no pre-existing failures remain -- see cf3df18). * fix: Phase 4 TensorRT engine-write atomicity and graph/refit robustness Guard TRT engine and timing-cache writes against truncation from an interrupted build via a temp-file + os.replace atomic write helper, matching the idiom already used for FP8 calibration data. Add self-healing recovery around cudaGraphLaunch (reset + fallback to plain execution, re-captures next frame) and a stream-sync guard around the defensive, disabled-by-default Engine.refit() path. Verified: 3 new CPU-only unit tests cover the atomic-write helper's happy path and both failure-preservation cases; a live-GPU smoke run against a cached VAE engine exercised capture, happy-path replay, a forced cudaGraphLaunch failure (recovery + safe fallback, no crash), and self-healed re-capture. * style: add bytes type hint to _atomic_write_bytes data param * fix: use device-wide sync before TRT refit, not current-stream-only * chore: widen Claude review workflow branch filter for chained PR stack Adds this workflow to the PR 1 branch and widens the base-branch filter to include the chained PR 2-5 branch names, so the Claude Code Action fires on PRs based on this branch (not just SDTD_032_dev) -- GitHub Actions resolves the pull_request workflow from the PR's base branch. Byte-identical change also applied to forkni:main for claude-code-action@v1 parity. * chore: enable show_full_output on Claude review workflow for tool-permission visibility * chore: widen Claude review allowedTools (Skill, python/pytest, /tmp Write) to unblock permission-denied max-turns failures * chore: raise Claude review max-turns to 60, discourage filesystem-wide find and output redirection
forkni
added a commit
that referenced
this pull request
Jul 19, 2026
* refactor: re-apply Phase 1 perf/best-practices cleanup (cosmetic + fail-fast) Re-applies the previously-reverted Phase 1 batch from the perf/best-practices audit: fix the FP8 Q/DQ gate comment to match the actual threshold, convert a dict() call to a dict literal (clears the one pre-existing ruff C408 on a touched file), require an explicit opt_batch_size in EngineBuilder.build() and its 5 compile_* wrappers (real callers already always pass it), drop a dead try/except around a single logger.info call, and tighten two type hints in StreamDiffusion.pipeline (Optional/Union/Tuple). No behavior change except the opt_batch_size fail-fast, which only affects callers that previously relied on an unused default of 1. * refactor: Phase 2 exception hygiene and observability fixes - Add shared _is_oom_error() helper (typed torch.cuda.OutOfMemoryError + string-heuristic fallback) and use it at both TensorRT UNet/VAE engine build fallback sites instead of duplicated inline substring checks. - Log swallowed LoRA candidate-weight-name failures in _load_lora_with_offline_fallback instead of silently continuing. - Log the inner fallback failure in advanced-model-detection instead of a bare except/pass. - Hoist the optional diffusers_ipadapter helper imports in IPAdapterModule.build_unet_hook out of the per-frame hook into a single build-time resolution, and log all previously-silent per-step fallback branches for observability. - Make _load_model fail fast (raise RuntimeError) when an SDXL pipeline retry after a type mismatch also fails, instead of silently continuing with the wrong pipeline type; also null out the stale mismatched `pipe` reference so a later loading-method failure can't let it slip through the final success check. - Add regression tests for _is_oom_error and the SDXL fail-fast path. * chore: migrate deprecated top-level ruff settings to [tool.ruff.lint] Ruff warns that top-level ignore/select/isort/per-file-ignores are deprecated in favour of the lint section. Move them under [tool.ruff.lint] (and .isort / .per-file-ignores); line-length stays at the top level. Verified against cuda-link's pyproject.toml, which already uses this layout. No behavior change: ruff check/format report identical results before and after (same 479 pre-existing repo-wide findings, 0 on the files touched by the Phase 1/2 commits). Investigated adding a matching [tool.pyrefly] section for parity with cuda-link, but did not adopt it: pyrefly falls back to a bundled 'basic' preset when no pyrefly config exists, and that preset is substantially more lenient than explicit config. Adding even an empty [tool.pyrefly] section opts out of it and jumps reported errors from 72 to ~900 (mostly dynamically-set attributes on StreamDiffusion / StreamDiffusionWrapper that were never checked before). Doing this properly needs the same per-module triage cuda-link performed (docs/adr/0005-static-typing-hardening.md) before turning on strict project-includes checking, which is out of scope here. * fix: sync _make_fake_engine test double with TensorRTEngine.__init__ _make_fake_engine (tests/unit/test_trt_engine_guards.py) built engines via TensorRTEngine.__new__() to bypass __init__, but never set _dedicated_stream, _pre_exec_event, _post_exec_event, or _buf_cache -- attributes __init__ has initialized since infer() gained cross-stream sync and LRU shape-cache support. This caused 3 AttributeErrors long treated as "pre-existing, out of scope" failures in every prior phase gate. Not a production bug: __init__ correctly sets all four to None/empty, and infer() guards each with `is not None`. Fix mirrors __init__ in the fake. Full unit suite now fully green (91 passed, 0 failed) with no waived tests. * fix: Phase 3 correctness and resource-safety fixes Four targeted fixes from the perf/best-practices audit, quick-wins #4 and #5: - StreamDiffusion.prepare(): generator defaulted to a pre-constructed torch.Generator(), a mutable default evaluated once at def-time so every instance sharing the default shared one Generator. Default is now None, constructed fresh in the method body. - StreamDiffusionWrapper.prepare(): both runtime stream.prepare() calls (single-prompt and prompt-blending paths) omitted seed, so every runtime prompt change silently reset the RNG to stream.prepare()'s own default. Now forwards seed=getattr(self.stream, "current_seed", 2) to preserve the active seed across prompt changes. - postprocess_image()'s "latent" branch returned an internal decode buffer by reference; callers retaining it across frames would see it mutate. Now clones at this public API boundary. The "np" pinned-buffer alias and the internal __call__/txt2img fast-path aliases are documented as intentional, not cloned (would defeat the pinned-buffer/reuse optimizations). - cleanup_engines_and_rebuild's hardcoded engines_dir = "engines" now honors self._engine_dir when set, matching the existing idiom elsewhere in the file. - cleanup_gpu_memory dropped two explicit Engine.__del__() calls; the method already does del + triple gc.collect() + empty_cache() + ipc_collect() at its tail, and Engine.__del__ is self-guarding, so the explicit dunder calls were redundant. Adds tests/unit/test_phase3_correctness.py (5 regression tests, model-free CPU-only). Full unit suite: 91 passed, 0 failed (no pre-existing failures remain -- see cf3df18). * fix: Phase 4 TensorRT engine-write atomicity and graph/refit robustness Guard TRT engine and timing-cache writes against truncation from an interrupted build via a temp-file + os.replace atomic write helper, matching the idiom already used for FP8 calibration data. Add self-healing recovery around cudaGraphLaunch (reset + fallback to plain execution, re-captures next frame) and a stream-sync guard around the defensive, disabled-by-default Engine.refit() path. Verified: 3 new CPU-only unit tests cover the atomic-write helper's happy path and both failure-preservation cases; a live-GPU smoke run against a cached VAE engine exercised capture, happy-path replay, a forced cudaGraphLaunch failure (recovery + safe fallback, no crash), and self-healed re-capture. * perf: instrument unet_step with profiler.region() spans (Sub-phase 5.0) Adds 5 graph-safe profiler.region() spans (prep/sdxl_cond/hooks/engine/post) inside unet_step to attribute the audit's "~66% unattributed" frame time. Baseline capture (RTX 4090, SDXL-turbo FP8) shows that slice is GPU kernel time inside the single TRT UNet engine call (~93% of frame), not host overhead - every host sub-span measures <0.01ms. Instrumentation only, no behavior change; validated by the existing 94/0 unit suite. * perf: skip redundant set_tensor_address rebind in TRT graph replay (Sub-phase 5.1) Engine.infer() unconditionally re-bound every tensor address via set_tensor_address() before each engine call, even in graphed steady state where the addresses are already baked into the captured CUDA graph (self.tensors buffers are persistent and reused via copy_() - see allocate_buffers/_can_reuse_buffers, which resets cuda_graph_instance to None whenever a buffer is actually reallocated). Skip the rebind loop once a graph is captured; only rebind on first capture or after a reset_cuda_graph() recovery. Verified live on the RTX 4090 td_config.yaml runtime path (the UNet engine loads with use_cuda_graph=True unconditionally via engine_manager.py's loader): 24-frame smoke run with real graph capture + replay shows no errors and frame timing (p50 32.10ms, mean 47.33ms) matching a pre-fix A/B run (p50 31.74ms, mean 47.00ms) - confirms correctness with the expected ~0 measured perf delta (the audit's own 5.0 baseline showed host-side region time is <0.01ms; this is a hygiene/false-dependency fix, not a throughput win). Suite 94/0; ruff clean; pyrefly 72 errors (baseline, no new). * perf: sync-free output/PIL/IPC paths (Sub-phase 5.2) Three output-side sync/allocation fixes, ordered by real-world impact on the TD path: - 5f (only hot-path site): _ipc_pack_rgba / _ipc_pack_unit_rgba now write into a persistent HWC x4 GPU buffer (alpha set once at allocation, B/G/R channels written in place) instead of allocating a fresh torch.full_like alpha + torch.cat every frame. Safe only because the IPC exporters are forced to blocking export (ADR-0001); comments updated at both _lazy_init_ipc_exporter and _lazy_init_cn_ipc_exporter to reflect the buffer is now persistent, not transient. - 5e: _tensor_to_pil_safe (preprocessing_orchestrator.py) reordered CPU-first, following the in-repo base.py:tensor_to_pil template -- the D2H transfer now happens before the tensor.min()/.max() range-check syncs instead of after, collapsing 3 GPU host syncs down to 1. - 5d (hygiene; TD's IPC output path never hits this): _tensor_to_pil_optimized now routes through the shared _output_pin_buf/_d2h_event pinned-buffer + Event machinery instead of a blocking, unpinned .cpu() into pageable memory. Documented the same buffer-reuse caveat as the "np" path (PIL images returned wrap a view of the pinned buffer; callers retaining them across frames must .copy()). Verified via tests/unit/test_sync_free_output_5_2.py: byte-identical parity against frozen pre-5.2 reference formulas for all three sites, plus persistent-buffer reuse/realloc and independent-buffer-identity checks (10 new tests, GPU-gated). Suite 104/0 (94 baseline + 10 new); ruff clean on touched files; pyrefly 72 errors (baseline, no new). * fix: batch-dynamic engines skip l2tc tiling, silencing benign VALIDATE FAIL build_engine() passed dynamic_shapes=build_dynamic_shape into Engine.build(), which tracks only dynamic resolution. On the default "Flexible" preset the UNet is resolution-static but batch-dynamic (build_static_batch=False), so dynamic_shapes was passed False, and _apply_gpu_profile_to_config's tiling branch ran against a graph that still has a symbolic batch dim -- TRT emitted "[l2tc] VALIDATE FAIL - Graph contains symbolic shape" as a no-op for every applicable layer, for the entire ~11.5 min production TD session analyzed this round. Fix: dynamic_shapes=build_dynamic_shape or not build_static_batch (one line, utilities.py:1372) -- True whenever any dim, resolution or batch, is symbolic, matching l2tc's actual requirement (ALL dims must be concrete). Fully-static engines (e.g. ControlNet: build_static_batch=True, build_dynamic_shape=False) are unaffected -- still get tiling as before. Tightened the _apply_gpu_profile_to_config docstring to match. Verified via new tests/unit/test_l2tc_dynamic_shapes.py: calls build_engine() with GPU detection/memory query/the real TRT build all monkeypatched out (no CUDA/TRT context required) and asserts the dynamic_shapes kwarg it wires into Engine.build() for the previously-broken case (batch-dynamic, resolution-static -> True), the unaffected fully-static case (-> False), and the already-working resolution-dynamic case (-> True). Suite 107/0 (104 baseline + 3 new); ruff clean; pyrefly 72 errors (baseline, no new). Impact is log-spam-only -- zero perf/numerical change, visible on the next engine rebuild. * perf: elide trt.input_staging DtoD copy for kvo/fio UNet cache inputs (Sub-phase 5.6) Engine.infer() copied every feed_dict tensor into its own persistent staging buffer each frame solely to give TensorRT a stable contiguous address to bind. nsys profiling (5.0/5.5) attributed ~9% of frame time (p95 3.1ms, ~161 DtoD copies/frame) to this region on the UNet call, dominated by the kvo/fio StreamV2V cache inputs. Those cache tensors are already persistent, address-stable, TRT-contiguous buffers (create_kvo_cache/create_fi_cache build them with layers_in_bucket as the outermost dim specifically for this) — the only event that moves their address is a batch-size change, which already forces a full buffer realloc+graph reset today. So copying them every frame is pure waste. Add an opt-in zero_copy_names parameter to Engine.infer() and a pure _staging_action() decision helper (copy/bind/bind_and_reset) that lets TensorRT bind directly to the caller's tensor instead of copying into self.tensors[name], guarded by contiguity and dtype-match fallbacks. Wire it in UNet2DConditionModelEngine for the kvo/fio input names only; every other caller (VAE, ControlNet, safety, preprocessing) passes nothing and keeps the original copy path byte-for-byte. * fix: lower ControlNet dynamic-shape floor 384->256 via shared min_image_shape (bug #6) ControlNet's dynamic-shape get_input_profile() hardcoded a 384px floor independent of the UNet's 256px floor (BaseModel.min_image_shape), so any resolution in [256, 384) hard-failed at Engine.allocate_buffers the moment ControlNet was enabled. Derive the ControlNet floor/ceiling from the same BaseModel.min_image_shape/max_image_shape the UNet already uses (SDXL inherits via super()), lower the matching engine_manager.py build-option default, and relabel the ControlNet dynamic-engine cache directory (--dyn-384-1024 -> --dyn-256-1024) so a re-floored build never collides with a stale 384-floored cache. Trade-off: widening the dynamic profile can slightly reduce TRT tactic specialization at the optimum resolution -- acceptable given the alternative is a hard failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: normalize blend weights on CPU + persistent ControlNet conditioning_scale scalar (Sub-phase 5.4) Two per-frame GPU creation syncs removed: - _normalize_weights built a CUDA tensor from a Python list every call, but every consumer (.item()/.tolist() in slerp/multi_slerp/cosine_weighted paths, float*cuda_tensor in the linear paths) immediately reads it back or treats it as a scalar operand. Build it on CPU float32 instead -- same consumers work unchanged (a 0-dim CPU tensor is treated as a wrapped scalar when multiplied against a CUDA tensor), the sync and readback are both gone, and float32 is more precise than the model's fp16 for the normalization divide. - ControlNetModelEngine rebuilt a conditioning_scale tensor via torch.tensor(...) on every __call__. Replace with one lazily-allocated persistent scalar updated via fill_() (an async kernel launch, no sync), mirroring the existing _fi_strength_tensor.fill_() idiom. Each active ControlNet has its own engine instance, so a per-instance buffer is correct; the binding rank (_cs_rank1) is fixed at load time so the shape never changes underneath it. Also drops three dead variable assignments and fixes one out-of-order import in stream_parameter_updater.py (pre-existing lint debt in a file this change already touches; the repo's commit gate lints the whole staged file, not just the diff). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: 1-frame-delayed async NSFW safety-checker readback (Sub-phase 5.3) Removes the cudaStreamSynchronize that NSFWDetectorEngine.__call__ forced on the hot path via probs[0,0].item() whenever use_safety_checker=True. The classifier now writes its GPU probability into a pinned host scalar via a non_blocking copy; _apply_safety_checker reads the *previous* frame's pinned verdict before launching this frame's classification, mirroring the existing SimilarImageFilter 1-frame-delayed readback pattern (image_filter.py). Edge cases: first frame passes through unconditionally (no prior verdict to act on); a 1-frame detection-leak window is possible before flagging lands. Both are acceptable for an opt-in, off-by-default, TensorRT-only feature. Updated tests/unit/test_safety_checker.py for the new (tensor, prob_pin) -> None checker contract and the delayed-readback semantics (10/10 passing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: add Claude review workflow to PR2 branch for head-ref validation * chore: widen Claude review allowedTools (Skill, python/pytest, /tmp Write) to unblock permission-denied max-turns failures * chore: raise Claude review max-turns to 60, discourage filesystem-wide find and output redirection * fix: correct NSFW verdict-to-frame attribution and guard pin_memory on CPU-only builds _apply_safety_checker previously read the prior frame's async verdict but applied it to the current frame's pixels, letting an isolated NSFW frame leak through and wrongly blanking the next clean frame. Buffer the raw frame in _pending_frame and emit it once its own verdict lands (+1 frame latency, first call now emits a black startup frame instead of raw pixels). Also guard .pin_memory() behind torch.cuda.is_available() so the CPU-only test contract in test_safety_checker.py holds. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of the stacked remediation PRs (PR 1 of 5). Squashes together four independent, previously-reviewed remediation passes into one cohesive branch, cut directly at the existing tip commit — no history rewrite, original commit hashes/messages preserved.
Test plan
pytest tests/unit -q— 94 passed, 0 failed (run against this branch's own source viaPYTHONPATHoverride, working around a shared-venv editable-install pointing at a sibling worktree)origin/SDTD_032_dev..HEAD= 6 commits, tip80c83ce)🤖 Generated with Claude Code