Skip to content

Fix issues found in code review - #6

Merged
BobsBlazed merged 1 commit into
mainfrom
claude/node-update-improve-2fygeb
Jul 29, 2026
Merged

BobsBlazed merged 1 commit into
mainfrom
claude/node-update-improve-2fygeb

Conversation

@BobsBlazed

Copy link
Copy Markdown
Owner

Eight findings from a review of 1.2.0. None changes behaviour for a correctly-paired LoRA and model — the first two matter when the pairing is wrong.

1. The dot-boundary fix never reached the FLUX/SDXL classifiers

The Universal loader matches block stacks on the raw dotted key, because normalising . → _ erases the separator that distinguishes an outermost stack from one nested inside a block. The two dedicated classifiers never got that fix, so an SDXL key:

input_blocks.4.1.transformer_blocks.0.attn1.to_q.weight

matched the inner transformer_blocks and was read as double-stream block 0.

Anchoring on . alone turned out to be insufficient — the nested stack is also dot-preceded. The patterns are now anchored to the start of the module path after an optional known prefix, which is the property that actually distinguishes them. Verified against 782 authentic FLUX and 1696 authentic SDXL keys: unchanged.

keys → node before after
SDXL → FLUX all 1696 in Early Downsampling 1674 Other Tensors, 22 Final Output Layer
FLUX → SDXL all 782 in Other Tensors unchanged

2. Wrong loader for a model now warns instead of failing silently

Picking the wrong node was invisible: the keys still match — ComfyUI builds the key map from the model, so the LoRA loads — they just all land in one bucket, and the sliders quietly stop meaning what they say.

A mismatch check now fires when ≥90% of UNet tensors land in a single block, on both the info output and the log. A threshold rather than "all of them", because cross-family names overlap slightly: real SDXL keys through the FLUX classifier leave 98.7% in the catch-all and send the rest to the shared proj_out token, which exact equality would wave through.

Measured against real key sets:

MISMATCHED (must warn)
  SDXL keys -> FLUX node : WARNING: 99% of UNet tensors landed in 'Other Tensors'...
  FLUX keys -> SDXL node : WARNING: 100% of UNet tensors landed in 'Other Tensors'...
CORRECT (must stay silent)
  FLUX->FLUX, SDXL->SDXL, SD15->SDXL, and SDXL/SD15/FLUX->UNIVERSAL : all None

3. Import-order dependency in the shared registries

bobs_universal mutated four of bobs_blocks's dicts at import time. Confirmed consequence: after that import, this line in tests/test_bobs_blocks.py raises KeyError: 'UNIVERSAL':

blocks = {"FLUX": ALL_FLUX_BLOCKS, "SDXL": ALL_SDXL_BLOCKS}[family]

It passed only because the two test files happened to load bobs_blocks under different module names, so they got separate instances — luck, not design. Families now register through an explicit register_family(), and the test drives off ALL_BLOCKS.

4–8. Smaller items

  • Per-family tooltips. FLUX/SDXL and Universal each define a block named "Text Encoder"; BLOCK_TOOLTIPS.update() meant the Universal wording overwrote the other. Tooltips are now stored per family.
  • Output Head over-matching. The _head_ substring also caught multi_head_attention and head_dim_proj. Now anchored to a real head module (head, head.head, out.<n>).
  • Unknown preset names log a warning instead of silently behaving as Custom (only reachable from hand-edited workflow JSON).
  • FLUX negative block indices accepted, matching the Universal loader's handling of SD3's joint_blocks.-1.
  • Dropped a 1.01 bucket boundary guarding a fraction that cannot occur — depth_fraction maxes at (n-0.5)/n.
  • Bonus, found while writing the tests: _mismatch_warning was keying off len(grouped), which is dense in production but sparse in a unit test. It now uses its blocks argument, which was otherwise unused.

Testing

71 tests, up from 55, still with no ComfyUI or torch dependency. New tests/test_node_behaviour.py covers node registration, widget order, output signature, report formatting and the mismatch warning — including the exact 1674/22 distribution from finding 1, so the threshold can't silently regress to exact-equality.

The real-ComfyUI integration harnesses were re-run: 8,000+ authentic keys across 11 architectures, still nothing falling through to Other Tensors, and the node end-to-end run through a real ModelPatcher is byte-identical (block isolation still exactly 384/1933).

pyproject.toml bumped to 1.2.1.


Generated by Claude Code

Eight findings from a review of 1.2.0. None changes behaviour for a correctly
paired LoRA and model; the first two matter when the pairing is wrong.

The FLUX and SDXL classifiers never received the dot-boundary fix the Universal
loader got. They matched on the underscore-normalised key, which erases the
separator distinguishing an outermost block stack from one nested inside a
block, so an SDXL key

  input_blocks.4.1.transformer_blocks.0.attn1.to_q.weight

was read as double-stream block 0. Anchoring on "." alone is not enough either,
since the nested stack is also dot-preceded, so the stack patterns are anchored
to the start of the module path after an optional known prefix. Real FLUX and
SDXL keys are unaffected -- verified against 782 and 1696 authentic keys.

Choosing the wrong loader for a model was silent: the keys still match, because
ComfyUI builds the key map from the model, so the LoRA loads and every tensor
lands in whichever single bucket the classifier reaches. A mismatch check now
warns, on the info output and in the log, when 90% or more of the UNet tensors
land in one block. A threshold rather than "all of them" because cross-family
names overlap slightly: real SDXL keys through the FLUX classifier leave 98.7%
in the catch-all and send the rest to the shared proj_out token. Measured at
99% and 100% for the two mismatched pairings, silent for all six correct ones.

Also:

- Families register through register_family() instead of mutating bobs_blocks'
  tables at import time. The old approach made behaviour depend on import order
  and left test_bobs_blocks one import away from a KeyError on "UNIVERSAL".
- Tooltips are stored per family. FLUX/SDXL and Universal each define a block
  named "Text Encoder", and the Universal wording overwrote the other.
- The "_head_" output token also matched multi_head_attention and head_dim_proj;
  it is now anchored to a real head module, along with out.<n>.
- An unknown preset name logs a warning rather than silently acting as Custom.
- FLUX block indices accept the negative form, matching the Universal loader.
- Dropped a 1.01 bucket boundary guarding a fraction that cannot occur.
- _mismatch_warning uses its blocks argument rather than the sparse grouped
  dict, which is dense in production but not in tests.

Tests grow from 55 to 71, adding tests/test_node_behaviour.py for node
registration, widget order, report formatting and the new warning. The real
ComfyUI integration harnesses still classify 8000+ authentic keys across 11
architectures with nothing falling through.

Bumps version to 1.2.1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TUT2wQ8UsjNgosRWu6jvpj
@BobsBlazed
BobsBlazed marked this pull request as ready for review July 29, 2026 14:48
@BobsBlazed
BobsBlazed merged commit e7f867a into main Jul 29, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants