refactor: remediation phases 1-4 (perf, exception hygiene, correctness, TRT atomicity)#30
Open
forkni wants to merge 1 commit into
Open
Conversation
…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
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
1 participant
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
acceleration/tensorrt/utilities.py: performance, exception hygiene, correctness, and TensorRT atomicity fixes.forkni/StreamDiffusion@SDTD_040_beta_release(internal PR chore(installer): overhaul install scripts — portable paths, TRT verification, pin alignment #10).Test plan
SDTD_040_beta_releasetip, no conflicts.src/streamdiffusion/acceleration/tensorrt/utilities.py).