feat(diffusion): add LoRA support to addon run config#1474
Conversation
edab08d to
e8b54bc
Compare
jesusmb1995
left a comment
There was a problem hiding this comment.
Missing version and CHANGELOG update
There was a problem hiding this comment.
Thanks for this — the core change is small, correct, and safe (the PreparedLoras ownership pattern mirrors the pinned sd.cpp fork's CLI flow, and NRVO/vector-move preserves the captured c_str() across the return, so the current code is fine as a v0).
Requesting changes on two points before merge:
1. Validate params.lora in index.js, or document the silent-no-op
The JSDoc promises "Absolute path to a LoRA adapter", but index.js doesn't enforce it:
assertAbsolute(...)is only applied tofiles.*in the constructor.params.lorais never checked — a relative path from JS silently becomes CWD-relative in sd.cpp.- More importantly:
apply_loras_immediatelyin the fork silently skips LoRAs that fail to load (stable-diffusion.cpp:1082:if (!lora || lora->lora_tensors.empty()) continue;). A typo, a wrong format, or a missing file produces a clean "successful" generation with no LoRA applied and no error returned to the caller.
Please do one of:
- Preferred: add a one-shot validation in
_runInternalnext to the existinginit_imagecheck, e.g.:Fails fast with a clear error and is consistent with howif (params.lora != null) { if (typeof params.lora !== 'string' || params.lora.length === 0) { throw new TypeError('lora must be a non-empty absolute path string') } if (!path.isAbsolute(params.lora)) { throw new TypeError(`lora must be an absolute path (got: ${params.lora})`) } }
files.modelis validated. - Acceptable alternative: leave the validation for a follow-up but update the JSDoc in
index.jsand the TS doc inindex.d.tsto explicitly state the silent-no-op behaviour — something like "If the file cannot be loaded, generation proceeds without the LoRA and no error is raised; callers must verify the file exists and is a valid adapter before callingrun()."
Either is fine — the goal is that callers aren't surprised by a working-looking call that didn't actually apply anything.
2. Regenerate test/mobile/integration.auto.cjs
The new test/integration/lora-bridge.test.js is missing from the auto-generated mobile wrapper. It's labelled "AUTO-GENERATED FILE. Run npm run test:mobile:generate to update." and the repo's own test:integration:generate script re-runs that generator, so the tree currently has drift.
Since the new test has skip = isMobile || noGpu, this isn't a functional problem on mobile, but any "no uncommitted changes after generate" check in CI will flag it.
Please either:
- Run
npm run test:mobile:generateand include the regenerated file in this PR, or - Confirm with CI (and mention it in a comment) that the drift is tolerated and the next PR will pick it up.
Non-blocking follow-up material (flagging so it doesn't get lost):
- The surface is single-LoRA, hardcoded
multiplier = 1.0f, nois_high_noiseexposure.docs/architecture.mdalready advertises "multiple simultaneous LoRAs with configurable weights", so the JS layer now lags the doc — worth a follow-up ticket (array of{ path, multiplier?, is_high_noise? }, or reuse the upstream<lora:path:mul>prompt syntax). - Foot-gun if ever extended: a second
paths.push_backinprepareLoraswill reallocate the vector and invalidate every previously-captureditems[i].path. Safe today because there's exactly one element, but aprepared.paths.reserve(n)and/or a short comment there would future-proof it. - The integration test asserts only "valid PNG produced". Combined with the silent-no-op above, it would pass even if LoRA loading failed. A cheap upgrade is a second generation with the same seed but without
lora, thent.ok(!bytesEqual(a, b)). Matches the PR description's "bridge forwarding" intent a lot more tightly. - Minor nit:
"lora"is placed under the "Prompt" section inSdGenHandlers.cpp, and the unit test fixture isSdGenHandlers_Prompt. A dedicated"LoRA"section +SdGenHandlers_Lorafixture will age better.
|
/review |
Summary
Adds end-to-end LoRA support to the diffusion addon.
Changes
lorafield to JS run configlorathrough JS → native bridgeloraPathlorasd_img_gen_params_t.lorasNotes
lorais not setValidation