Skip to content

fix: close 34 findings from the PR 171-194 review - #241

Merged
Pfannkuchensack merged 3 commits into
mainfrom
fix/merge-errata-171-194
Sep 11, 2026
Merged

fix: close 34 findings from the PR 171-194 review#241
Pfannkuchensack merged 3 commits into
mainfrom
fix/merge-errata-171-194

Conversation

@Pfannkuchensack

Copy link
Copy Markdown
Member

Follow-up to PRs #171, #172, #173, #174 and #194. An adversarial review of that merge window turned up 33 findings; this closes all of them, plus one pre-existing bug of the same class that the review exposed in SD1/SDXL.

The five that ship a wrong result rather than a crash

Tiling a diffusers VAE returned a wrong-sized image. scoped_vae_tiling set tile_sample_min_size and tile_latent_min_size independently, but diffusers derives its latent loop step from one and its output crop from the other — they only agree when the latent tile is a multiple of 4. The node field allows any multiple of 8. Measured on a 4-block AutoencoderKL at a 768px target, sweeping all 84 legal values: 24 correct, 59 wrong-shaped, 1 crash. The same defect is pre-existing in latents_to_image.py / image_to_latents.py: at a 512px target, 12 correct, 35 wrong-shaped, 2 crashes, and it affects encode as well as decode. Both now derive the latent tile first and the sample size from it, so the pair cannot disagree.

Overlapping SDPA windows permanently corrupted process-global state. torch.nn.attention.sdpa_kernel saves and restores at::globalContext() non-atomically, so two threads whose windows overlap leave it changed for the process lifetime — reproduced: priority order [FLASH, EFFICIENT, MATH, CUDNN][FLASH, CUDNN, EFFICIENT, MATH], and with an exclusive override, three backends left disabled. Both the Krea-2 attention processor and the Anima denoise loop open such windows, and InvokeAI runs one session per GPU concurrently, so it leaks into every other model in the process. New invokeai/backend/util/sdpa_scope.py gives the globals one owner: the outermost window is the only one that writes them.

cast_unquantized widened integer payloads. The function the int8 loaders switched to lacked the float guard whose own comment warns that "a loader that switches to cast_state_dict does not silently lose the guard". An index buffer installed as a float raises inside the forward; a bool mask changes meaning silently.

The int8 branch ignored each model's declared precision-sensitive modules. ZImageTimestepEmbedder reads self.mlp[0].weight.dtype to pick its activation dtype; on an Int8ConvrotLinear that is torch.int8, and the fallback looks for a compute_dtype attribute neither class defines — so the timestep branch would run in float32 and hand a float32 embedding to a bf16 model.

db_synchronous: normal was applied without WAL. PRAGMA journal_mode = WAL reports the mode it actually established and the return value was discarded. On a rollback journal, normal can corrupt the database — the opposite of the documented guarantee. It is now read back, and normal refused for a database file that did not get WAL.

Verified against real checkpoints

Every int8 fixture in the original PR was synthetic. Both published Comfy-Org int8 builds were loaded through the production loaders:

Z-Image Turbo int8 Krea-2 Turbo int8
checkpoint 5.78 GiB 12.57 GiB
resident after load 5.75 GiB 12.57 GiB
make_room reserved 5.75 GiB 12.57 GiB
Int8ConvrotLinear / dense 270 / 6 224 / 40
integer payloads widened none none

A real 1024×1024 forward through all 270 swapped Z-Image layers: 580 ms, 494 MiB activation peak, output finite, std 1.27 — the RAM prediction is exact on real files, and the layers compute.

Behaviour changes

  • force_tiled_decode now applies to FLUX.1, which ignored it. A tiled decode is not pixel-identical to a single-pass one — the decoder's GroupNorms and mid-block attention are global, so the difference is spread across the image (measured 0.11 at 1024px on a ±2.7 range), not confined to the seams. The setting's description now says so.
  • INVOKE_KREA2_SDPA_BACKEND=priority-cudnn is now spelled default. It never ranked cuDNN first — the shipped list is flash-first. An old exported value now fails at startup rather than mid-generation.
  • A checkpoint mixing int8_tensorwise with scaled fp8 is refused at load instead of loading the fp8 weights unscaled. Scoped to modules the built model actually consumes, so an all-in-one export bundling a foreign submodel still loads.
  • An OOM-triggered tiled retry now logs a warning, not only a transient progress event, so a support log records that the image was tiled.

Verification

  • pytest -n logical: 7556 passed, 169 skipped, 9 xfailed. 9 failures are environmental on the dev box (HF_ENDPOINT points at a private-network mirror the SSRF guard refuses); none of the failing paths are touched by this diff.
  • ruff check and format --check clean; openapi.json and schema.ts regenerated.
  • Three independent read-only review passes (correctness; architecture/safety/performance; test value/product quality). Every material finding is resolved in this branch.

Known limitations

  • The precision-sensitive-layer fix is not exercised by either published checkpoint — both leave those layers dense and unmarked. The fix is defensive against repacks that do not.
  • peak_int8_dequant_transient_bytes is exact for most shapes and ~2 MiB short on the two where cuBLAS takes a workspace, against a 3 GiB reservation floor. Documented at the function.
  • CPU and CUDA only; no ROCm, MPS or XPU path exercised. The WAL refusal is tested through a transient database, not a real network share.
  • MiniMax H3 does not get the skip-pattern split, so a "quantize everything" repack is refused there — with a named error now, rather than an unattributed crash.

Tiling a diffusers VAE assembled a wrong-sized image for most legal tile sizes —
in Z-Image, and pre-existing in SD1/SDXL decode and encode. Overlapping SDPA
windows permanently corrupted torch's process-global backend selection. The int8
loaders cast integer payloads, left precision-sensitive layers quantized, and
accepted mixed-format checkpoints that then loaded unscaled. `db_synchronous:
normal` applied even without WAL, the one condition that makes it safe.

Behaviour changes:
- `force_tiled_decode` now applies to FLUX.1, which ignored it; a tiled decode is
  not pixel-identical to a single-pass one.
- `INVOKE_KREA2_SDPA_BACKEND=priority-cudnn` is now `default` — it never ranked
  cuDNN first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pfannkuchensack and others added 2 commits September 11, 2026 17:28
`force_tiled_decode`'s description changed; `settings.json` is generated from it
by a different script than the OpenAPI artefacts and was missed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Pfannkuchensack
Pfannkuchensack enabled auto-merge (squash) September 11, 2026 16:20
@Pfannkuchensack
Pfannkuchensack merged commit b8fa42f into main Sep 11, 2026
19 checks passed
@Pfannkuchensack
Pfannkuchensack deleted the fix/merge-errata-171-194 branch September 11, 2026 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant