diff --git a/AUDIT_REPORT.md b/AUDIT_REPORT.md deleted file mode 100644 index 20f1eef..0000000 --- a/AUDIT_REPORT.md +++ /dev/null @@ -1,393 +0,0 @@ -# MiniMax H3 Power LoRA Stack — Technical Audit - -Audit date: 2026-08-08 -Audited revision: `400770a` -ComfyUI revision used for compatibility review: `dd79c643` (2026-08-08) - -## Executive assessment - -The project has a strong technical core. It identifies three real H3-specific -problems—quantized-weight merging, adaLN basis changes, and inconsistent LoRA -key conventions—and addresses them with a sensible module split. The plain-LoRA -runtime branch and the dense-to-curve math both held up under targeted numerical -checks. The key normalizer also mapped all recognized keys in the 46 locally -installed H3 LoRAs to a representative H3 checkpoint with zero unmatched keys. - -The node should still be considered **experimental rather than production-ready**. -The main blockers are not syntax or basic mathematics; they are edge-path -correctness, unsupported adapter behavior on quantized models, incomplete -accounting of what was actually applied, and the complete absence of automated -tests. Several README claims are stronger than the implementation can currently -guarantee. - -Recommended release posture: keep the current version marked beta/experimental -until the high-priority findings below are fixed and covered by tests. - -## Scope and evidence - -The audit covered: - -- Python node registration and ComfyUI integration. -- Dynamic frontend widgets, picker, serialization, and auto-balance behavior. -- Key normalization and adapter-format handling. -- Quantized runtime branches and stack fusion. -- Dense/curve and curve/curve adaLN conversion. -- Per-modality adaLN scaling. -- Gain measurement, HTTP route, diagnostics, packaging, and documentation. -- The installed H3 library: 46 safetensors files totaling about 29.7 GB. - -Checks performed: - -- All 11 Python files passed AST parsing. -- The package imported against the installed ComfyUI and registered all three - nodes successfully. -- `web/h3_power_lora_stack.js` passed `node --check`. -- All 46 local H3 safetensors headers were readable: 32 comfy, 5 kohya, and 9 - bare-format files. -- The local set contains 14 curve-adaLN files, 8 dense-adaLN files, and 24 files - without adaLN pairs. -- All recognized keys from all 46 files normalized against the sampled H3 model - with zero unmatched keys. -- Synthetic dense→curve conversion had maximum absolute error around `1.5e-5`. -- Synthetic two-LoRA fusion had maximum absolute error around `2.0e-6`. -- The discovered silu grid is `[1025, 2688]`; the two local curve tables both - fitted at approximately `1.68e-3` relative residual. -- A CUDA table-to-table fit reproduced the device mismatch described in finding - H5 below. - -Not covered by an existing test harness: full H3 sampling, VRAM/offload stress, -workflow round-trip testing in the browser, older ComfyUI versions, and every -quantization layout. No test suite exists in this repository. - -## Priority findings - -| ID | Severity | Area | Summary | -|---|---|---|---| -| H1 | High | adaLN | A shared dense-target context caches the first source LoRA's curve basis and can use it for later LoRAs from a different bake. | -| H2 | High | Loading | A local combined LoRA/ALS file contains 102 full adaLN tensors that are silently ignored by the apply path. | -| H3 | High | Quantization | LoKr, LoHa, DoRA, LoCon, and other non-plain adapters are merged into quantized weights, defeating the node's main correctness guarantee. | -| H4 | High | Reporting | “Merged” and “unmatched” counts do not prove that ComfyUI accepted or applied the patches. | -| H5 | High | Device support | Curve-to-curve fitting fails when model tables are on CUDA. | -| M1 | Medium | Formats | PEFT/default and alternate LoRA suffixes normalize but are not handled consistently by adaLN porting, modality scaling, detection, and gain measurement. | -| M2 | Medium | adaLN | Curve→dense computes the same pseudoinverse once per adaLN layer instead of once per basis. | -| M3 | Medium | Reproducibility | Grid selection is implicit and first-match; the chosen grid path/fingerprint is not reported or user-selectable. | -| M4 | Medium | Auto-balance | Calibration is a collection-specific heuristic presented too much like an exact measurement. | -| M5 | Medium | Input handling | Missing/ambiguous files and non-finite strengths can be skipped or propagated without a useful node report. | -| M6 | Medium | Frontend | The LoRA list is cached for the whole browser session and balance state has edge-case drift when replacing the last balanced row. | -| M7 | Medium | Release quality | There are no tests or CI, compatibility is undeclared, and repository metadata/legal files are incomplete. | - -## Detailed findings - -### H1 — Dense-target multi-LoRA basis cache can corrupt later adapters - -`apply_stack()` creates one `AdalnContext` and reuses it for every LoRA. On a -dense target, `AdalnContext.basis(curve_table=...)` must use the source table -shipped by each curve-trained LoRA. Instead, `_basis` and `_failed` are stored -once on the shared context. After the first source table is fitted, later source -tables receive that first fit. If the first LoRA lacks a table and marks the -context failed, later convertible LoRAs are also prevented from fitting. - -This is a latent but serious numerical error: it produces plausible-shaped -tensors and therefore may not raise an exception. - -Relevant code: `h3lora/apply.py:128`, `h3lora/apply.py:145`, -`h3lora/adaln.py:190`, and `h3lora/adaln.py:200`. - -Required work: - -- Cache dense-target bases by a stable source-table identity/fingerprint, not - once per stack. -- Keep target-table basis caching separate; sharing that case is valid. -- Do not let failure for one LoRA poison later LoRAs. -- Add a test stacking two curve LoRAs with different source tables onto one - dense target, plus a “missing table then valid table” ordering test. - -### H2 — Full adaLN passenger tensors are ignored - -`keymap.normalize()` deliberately passes full replacement tensors through and -says the caller will handle them. `apply_stack()` only removes -`adaln_t_table`; it then sends the remaining dictionary to -`comfy.lora.load_lora()`, which does not interpret plain -`blocks.N.adaln_proj.linear.weight` or `.bias` tensors as patches. - -This is not hypothetical. The local -`minimax_h3_pruned_turbo_ckpt850_loraWithALS.safetensors` contains: - -- 208 A tensors and 208 B tensors for ordinary LoRA layers. -- One `adaln_t_table`. -- 51 full adaLN weights and 51 full adaLN biases. - -The 102 full tensors are currently omitted while the ordinary layers still -report success. For a file whose schedule behavior depends on those ALS tensors, -that changes the adapter's meaning. - -Required work: - -- Decide and document whether full replacements are supported. -- If supported, canonicalize them and apply them with explicit `set` semantics, - including a clear policy for strength values other than 1.0 and for stacking. -- If not supported, reject the file with an actionable error rather than - partially applying it. -- Include passenger/applied/rejected counts in the node report. - -### H3 — Unsupported adapter types merge into quantized weights - -Only a plain 2-D `LoRAAdapter` is eligible for the runtime branch. Everything -else is put in `merge`, including LoKr, LoHa, DoRA, LoCon, and full-delta -adapters. On w4a8/int4 this uses the same destructive merge path that the node -exists to avoid. The README lists this as a limitation, but the runtime report -only says “merged”; it does not warn that accuracy may be lost. - -The local collection already contains one LoKr file, so this affects a current -user path rather than only a future format. - -Required work: - -- Preferred: integrate or adapt ComfyUI's bypass-adapter machinery for supported - non-plain adapters, retaining stack fusion where it is mathematically safe. -- Minimum safe behavior: detect a quantized target and reject or prominently - warn before merging a non-plain adapter. -- Report forced INT8 `mlp.fc2` merges separately for the same reason. -- Test plain LoRA, LoKr, LoHa/LoCon, DoRA, and full-diff behavior on plain, INT8, - and w4a8 targets. - -### H4 — The report overstates what was applied - -There are three accounting gaps: - -1. `unmatched` only covers failures in key normalization. Passenger tensors, - orphaned A/B components, and suffixes Comfy does not consume are not included. -2. `patcher.add_patches()` returns the keys accepted by the model state dict, - but the return value is ignored. `len(merge)` is reported instead. -3. Branch shape failures increment `report.skipped`, but that count is never - included in the per-LoRA detail or final summary. - -As a result, “0 unmatched” and “N merged” are not equivalent to “everything was -applied,” despite the README promising an account for every LoRA. - -Required work: - -- Track normalized, consumed-by-Comfy, accepted-by-patcher, branched, and - rejected keys independently. -- Capture and surface shape mismatch reasons. -- Include missing files from `_collect()` in the returned report instead of - logging them before report creation. -- Add a strict mode that fails if any material tensor is unconsumed. - -### H5 — Table-to-table fitting is not device-safe - -`fit_table_to_table()` converts source and target tables to float64 on their -current device, then creates the intercept column on CPU. If a model table is on -CUDA, `torch.cat()` fails with a CPU/CUDA mismatch. This was reproduced on the -installed CUDA runtime. - -The usual pre-sampling path may keep model buffers offloaded on CPU, so the bug -will not trigger in every workflow. It remains a real failure under resident, -already-loaded, or nonstandard patcher configurations. - -Required work: explicitly move both tables to CPU for fitting, as `fit_basis()` -already does, or create every temporary on `tgt.device`. CPU is preferable for -predictable memory use and cache behavior. - -### M1 — Format support is broader in key mapping than downstream handling - -The key mapper recognizes `.lora_A.default.weight`, `.lora_B.default.weight`, -`.lora_A`, and `.lora_B`, but `port_adaln_pairs()` only pairs standard -`lora_A.weight`/`lora_B.weight` and kohya down/up names. Modality scaling also -omits the default and bare B suffixes. The inspector misses those rank forms. -Other suffixes supported by current ComfyUI, such as `_lora.up.weight`, -`lora_linear_layer.up.weight`, and `reshape_weight`, are not uniformly -canonicalized when they arrive without the canonical prefix. - -This means a PEFT file may load ordinary layers while silently losing adaLN -porting or modality control. - -Required work: define one adapter-suffix registry and reuse it in key mapping, -adaLN pairing, modality scaling, inspection, and gain measurement. Add a fixture -for every advertised convention. - -### M2 — Curve→dense repeats an expensive pseudoinverse - -`torch.linalg.pinv(v)` is inside the per-module loop. A typical H3 LoRA has 51 -adaLN projections, so the same `[2688, 8]` pseudoinverse may be computed 51 -times. Compute and cache it once with the fitted basis. - -Also add caching to table-to-table maps and a residual policy. At present a -high table-to-table residual is logged at info level and the mapping is still -applied. A bad fit should produce a visible warning or fail under strict mode. - -### M3 — Grid discovery is convenient but not reproducible - -The implementation uses the first generic -`h3_silu_temb_grid.safetensors` found across several locations. `grid_path` -exists in the Python API but is not exposed by the node. The report prints the -fit residual but not which file was selected. - -This matters because the README correctly notes that bakes differ. Two users -can run the same workflow and select different grids based on filesystem order. - -Required work: - -- Expose auto/explicit grid selection, or bind known grid fingerprints to model - metadata. -- Report the selected path, tensor shape, checksum, and residual. -- Search all configured LoRA/model paths, not only index zero. -- Invalidate the grid cache on mtime/size changes. - -### M4 — Auto-balance is useful, but its claims need tighter boundaries - -The low-rank Frobenius identity is implemented correctly for ordinary LoRA, but -the final `rel` is not an exact measurement against the loaded model. It uses -RMS constants from one INT8 checkpoint, excludes adaLN, is calculated before -modality/port transformations, and anchors to one local collection median. -Those are defensible product choices, but they make the result a calibration -heuristic rather than a universal unit. - -Additional correctness/reporting issues: - -- The “exact duplicate fingerprint” is only layer count plus aggregate energy; - different adapters can collide. -- Tucker LoKr is not measured according to ComfyUI's reconstruction, and one - decomposed-side alpha layout is scaled incorrectly. -- The backend report calls `factor` the strength auto-balance “would use.” It is - a multiplier; if the user's manual strength was not 1.0, that suggestion is - wrong. -- Header-only inspection and auto-balance only support safetensors, while the - picker can expose other LoRA file types. - -Required work: label the feature as heuristic calibration, report both factor -and resulting strength where the manual value is known, use a real content hash -for duplicate detection, and either implement or explicitly disable unsupported -measurement formats. - -### M5 — File and numeric input handling can mislead - -`_resolve_lora()` falls back to a case-insensitive basename match and returns -the first candidate. If two directories contain the same basename, a moved -workflow can silently select the wrong file. Missing files are logged and -removed before report generation, so a node can return “no LoRAs enabled” when -the real problem is that every requested file is missing. - -Strength parsing accepts arbitrary workflow data. `NaN` and infinity are not -rejected and can propagate into bank weights or model patches. - -Required work: reject ambiguous basename matches, preserve unresolved rows in -the report, require finite strengths, and bound them consistently with the UI. - -### M6 — Frontend lifecycle and feedback need hardening - -- `loraListPromise` caches the folder listing for the entire browser session, - with no refresh action; newly installed LoRAs require a page reload. -- When the only balanced row is repointed, its balance metadata is cleared - before `isBalanced()` is checked, so the replacement is not automatically - balanced and the node exits balanced mode. -- Balance failures are console-only and the button has no busy/error state. -- Strengths are rounded to two decimals; small intentional strengths can round - to zero after calibration. -- Serialization has recently been repaired, but there is no automated workflow - save/load/clone regression test to protect it. - -Required work: add refresh/invalidation, store node-level balance intent rather -than inferring it solely from rows, show user-visible request status, preserve -more numeric precision, and test format-1 migration plus format-2 round trips. - -### M7 — Release engineering and repository hygiene are incomplete - -- No unit, integration, frontend, or numerical regression tests. -- No CI configuration. -- No minimum ComfyUI revision/API capability check despite imports of recent - `quant_ops` and `weight_adapter` APIs. -- No `requires-python` declaration. -- `pyproject.toml` declares MIT, but there is no tracked `LICENSE` file. -- Python bytecode and `__pycache__` files are committed, and there is no - `.gitignore`. -- `pyproject.toml` points to the `doggeddalle` repository/publisher while the - configured Git origin is `cicalooo`; ownership and release metadata should be - made consistent. -- The README says key mapping was verified against 37 LoRAs, while later - sections and the current library use 46. Numerical benchmark claims have no - checked-in reproduction script or fixture manifest. - -Required work: add the legal file and ignore rules immediately, then establish a -small CI matrix and document supported ComfyUI/Python revisions before public -release. - -## What is already working well - -- The module boundaries are clear: normalization, application, branching, - adaLN conversion, modality control, gain, detection, and server handling are - independently understandable. -- Key normalization resolves underscore conventions against the actual model - key set instead of guessing token boundaries. The 46-file local scan supports - this design. -- The fused branch identity is correct and avoids one branch call per LoRA. -- Branch parameters are registered as an additional model for ComfyUI memory - accounting rather than hidden in closures. -- The special INT8 `mlp.fc2` fused path is explicitly recognized. -- Dense→curve conversion includes the required bias delta and passed a targeted - synthetic test. -- Modality scaling happens before porting, which correctly carries scaling into - the generated bias delta. -- The frontend escapes LoRA names before inserting highlighted HTML. -- The node provides unusually useful human-readable diagnostics; the remaining - task is making those diagnostics authoritative. - -## Recommended implementation sequence - -### Phase 1 — Correctness gate - -1. Fix per-source adaLN basis caching and compute `pinv(V)` once. -2. Make full passenger tensor handling explicit: support or reject, never - silently partially apply. -3. Prevent destructive non-plain merges on quantized weights, with explicit - warnings for forced fused-layer merges. -4. Rebuild reporting around consumed and accepted keys. -5. Make table fitting device-safe and add residual thresholds/strict mode. -6. Centralize suffix handling across every subsystem. - -### Phase 2 — Regression suite - -Add CPU-fast tests for: - -- Every advertised key convention and alternate suffix. -- Dense→curve, curve→dense, and curve→curve output equivalence, including alpha - and bias deltas. -- Two source bakes in one dense-target stack and failure isolation. -- Modality scaling before and after basis conversion. -- Branch fusion with positive, zero, and negative strengths. -- Quantized routing decisions for each layout and adapter type. -- Passenger, orphan, malformed shape, missing file, ambiguous file, `NaN`, and - infinity behavior. -- Gain formulas against explicitly materialized small matrices. -- Frontend create/save/load/clone/migrate and async balance behavior. - -Use tiny synthetic modules and safetensors fixtures; the core suite should not -need a 20 GB checkpoint or GPU. Add one optional GPU/offload smoke workflow. - -### Phase 3 — Product and release polish - -1. Expose/report grid provenance and compatibility. -2. Add LoRA-list refresh and visible balance status. -3. Document installation, compatibility, grid acquisition, example workflows, - and adapter-type limitations near the top of the README. -4. Add reproducible benchmark scripts and a fixture manifest for numerical - claims. -5. Add CI, license, ignore rules, version policy, and consistent repository - metadata. - -## Suggested release gates - -Do not call the node stable until all of the following are true: - -- No material LoRA tensor can be silently unconsumed. -- Quantized mode never silently sends an unsupported adapter through a known - destructive merge. -- Multi-LoRA adaLN conversion is correct regardless of row order or source bake. -- The report is derived from patches actually accepted by the model patcher. -- Format, math, serialization, and failure behavior have automated coverage. -- A documented ComfyUI version range passes CI and one real H3 smoke workflow. - -With those gates met, the project would move from an impressive specialist -prototype to a credible production loader. The underlying approach is worth -continuing; the largest remaining need is to make every fallback explicit and -testable. diff --git a/README.md b/README.md index 8f0e7cf..0266a83 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,8 @@ head. | **MiniMax H3 LoRA Schedule** | Varies selected stack rows' strength over denoising steps or normalized sigma | | **MiniMax H3 LoRA Inspector** | Reports a LoRA's format, rank and adaLN basis without loading it | +The stack row UI matured from [rgthree-comfy](https://github.com/rgthree/rgthree-comfy)'s Power Lora Loader. + ## Good to know - **AdaLN port — 8 Aug 2026, commit `28ac439`.** This stack rebases dense↔curve @@ -132,9 +134,9 @@ alibaba-pai Acc LoRAs ship a 32-interval output-head bank as `final_layer.video_out.set_weight` of shape `[3072, 5376]` (`32 × 96`) plus the audio twin. Stock Comfy `copy_`s that onto the native `[96, 5376]` head and crashes. This node peels those tensors before the stock `set` path and blends -the heads the sampler step spans with Core's rule (PR 15908): row 0 is the -full head and later rows are offsets from it. Original `proj_out` banks are -full heads, so those are converted to that layout first. Row strength +the heads the sampler step spans: row 0 is the full head and later rows are +offsets from it. Original `proj_out` banks are full heads, so those are +converted to that layout first. Row strength interpolates between native and blended heads; schedules multiply that strength. Use `simple` at 8 steps with shifts 12/3 so steps land on the trained grid. Later stack rows that also carry a bank replace the earlier one. diff --git a/h3lora/nodes.py b/h3lora/nodes.py index 52d5c0d..b0df283 100644 --- a/h3lora/nodes.py +++ b/h3lora/nodes.py @@ -130,7 +130,7 @@ class H3PowerLoraStack: rebased onto a pruned checkpoint's curve, instead of being skipped; * Acc/PDD head banks -- ``video_out.set_weight`` of shape ``[N*96, hidden]`` is peeled off before the stock ``set`` path and blended per sampler step - as row 0 plus offsets, matching Core; + as row 0 plus offsets; * key conventions -- ai-toolkit, kohya, lycoris, peft and bare-prefix layouts all resolve against the model's own key set. """ diff --git a/h3lora/pdd.py b/h3lora/pdd.py index 2fe8b66..582f538 100644 --- a/h3lora/pdd.py +++ b/h3lora/pdd.py @@ -2,15 +2,14 @@ alibaba-pai Acc LoRAs store a 32-interval bank as ``final_layer.video_out.set_weight`` of shape ``[N * 96, hidden]`` (and the audio twin). Stock Comfy ``copy_``s that -onto the native ``[96, hidden]`` head and crashes. Core PR 15908 teaches -``FinalLayer`` to blend those rows; this module does the same inside the Power -LoRA Stack so those files load without a core change. +onto the native ``[96, hidden]`` head and crashes, so this module peels the bank +and blends it at sample time. -Core's layout is not a stack of full heads. Row 0 is the full head and every -later row is an offset from it. A step is ``rows[0] + dt-weighted mean of the -offsets it spans``. Comfy ``set_weight`` files are already in that layout. -Original ``proj_out`` / ``audio_proj_out`` banks are full heads, so those are -converted to offsets before the same blend. +The layout is not a stack of full heads. Row 0 is the full head and every later +row is an offset from it. A step is ``rows[0] + dt-weighted mean of the offsets +it spans``. ``set_weight`` files are already in that layout. Original +``proj_out`` / ``audio_proj_out`` banks are full heads, so those are converted +to offsets before the same blend. The bank is kept beside the model (object patch + DIFFUSION_MODEL wrapper) and is registered as an additional model so ComfyUI can move and account for it. @@ -55,8 +54,8 @@ _AUDIO_BIAS = ( "final_layer.audio_out.set_bias", "audio_proj_out.bias", ) -# Original alibaba files store one absolute head per interval. Comfy's -# set_weight files already use Core's row-0-plus-offsets layout. +# Original alibaba files store one absolute head per interval. +# set_weight files are already row 0 plus offsets. _FULL_HEAD_KEYS = frozenset(( "proj_out.weight", "proj_out.bias", @@ -201,10 +200,10 @@ def peel(sd, video_dim, audio_dim, hidden): def _blend(weight, bias, h, n, start, stop, flow_shift): - """Core PR 15908 head: row 0 is the full head, later rows are offsets. + """Row 0 is the full head, later rows are offsets. - Matches ``comfy.ldm.minimax.model._pdd_head``. Interval 0's dt weight is - not applied to an offset; adding row 0 unweighted is that interval's share. + Interval 0's dt weight is not applied to an offset; adding row 0 unweighted + is that interval's share. """ start = max(0, min(n - 1, int(start))) stop = max(start + 1, min(n, int(stop))) diff --git a/web/h3_power_lora_stack.js b/web/h3_power_lora_stack.js index 1f355c9..0f03b7f 100644 --- a/web/h3_power_lora_stack.js +++ b/web/h3_power_lora_stack.js @@ -1,3 +1,4 @@ +// Stack row UI matured from rgthree-comfy's Power Lora Loader. import { app } from "../../scripts/app.js"; import { api } from "../../scripts/api.js";