Skip to content

[feat] LoRA controls and integration for Dreamverse - #1420

Merged
SolitaryThinker merged 8 commits into
mainfrom
feat/kaiqin/dreamverse_lora
Jun 2, 2026
Merged

[feat] LoRA controls and integration for Dreamverse#1420
SolitaryThinker merged 8 commits into
mainfrom
feat/kaiqin/dreamverse_lora

Conversation

@H1yori233

Copy link
Copy Markdown
Collaborator
  • Added new LoRA options API endpoint and functionality to apply LoRA styles with specified strengths.
  • Introduced LoraControls component in Dreamverse UI for user interaction with LoRA settings.

@mergify mergify Bot added type: feat New feature or capability scope: inference Inference pipeline, serving, CLI scope: model Model architecture (DiTs, encoders, VAEs) labels Jun 1, 2026
@mergify

mergify Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🔴 PR merge requirements

Waiting for

  • #approved-reviews-by>=1
  • check-success=fastcheck-passed
  • check-success=full-suite-passed
This rule is failing.
  • #approved-reviews-by>=1
  • check-success=fastcheck-passed
  • check-success=full-suite-passed
  • check-success~=pre-commit
  • title~=(?i)^\[(feat|feature|bugfix|fix|refactor|perf|ci|doc|docs|misc|chore|kernel|new.?model|skill|skills|infra)\]

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for dynamic LoRA stacks and style adapters across the Dreamverse application, introducing backend API endpoints, environment variable configurations, and a frontend control panel. The review feedback provides valuable suggestions to enhance robustness and maintainability, such as handling potential parsing errors for LoRA strengths, avoiding hardcoded style-specific logic by moving trigger positions to the configuration, and clearing debounce timeouts in the React component to prevent memory leaks.

Comment thread apps/dreamverse/dreamverse/config.py Outdated
Comment thread apps/dreamverse/dreamverse/config.py
Comment thread apps/dreamverse/dreamverse/video_generation.py Outdated
Comment thread apps/dreamverse/web/src/components/devtools/LoraControls.tsx
@mergify

This comment was marked as resolved.

@SolitaryThinker

Copy link
Copy Markdown
Collaborator

Hi @H1yori233 — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off.

TL;DR

Live LoRA-strength + style-stacking plumbing reads cleanly: executor parity (multiproc ↔ ray) is symmetric, the new set_lora_adapter signature is backward-compatible for all 4 in-repo callers, and the React debounce cleanup Gemini flagged is already in place. Two real correctness bugs and one heavy-cost path remain in the hot loop, plus zero test coverage for the new accumulate=True math.

Verdict: ship-with-fixes

  • S0 (blockers): 0
  • S1 (must-fix): 3
  • S2 (should-fix): 4
  • S3 (discussion): not shown here; see archived review

Findings (formatted for upload)

[S1] apply_lora always pushes ("omninft", strength) without verifying the active model has an OmniNFT LoRA repo

What: The /lora POST endpoint unconditionally builds stack = [("omninft", strength)]. _resolve_lora_spec("omninft") returns MODEL_CONFIG.get("lora_repo"), which is None for any model that doesn't declare lora_repo. apply_lora_stack then silently filters Nones out of resolved_stack, so the request returns 200 with a strength echo and gpus: {0: null} even though no LoRA was touched.

Why it matters: Silent no-op on a user-visible control. The UI says "applied" but nothing changed. Regression hazard for any deployment using a model without lora_repo or a custom DREAMVERSE_MODEL_PATH.

Suggested fix: In apply_lora, if _resolve_lora_spec("omninft") is None, either return 400 ("This model has no OmniNFT LoRA") or surface "applied": false in the response so the UI can warn. Alternatively, hide the OmniNFT strength slider in the frontend when /lora/options indicates the model has no base LoRA.

Evidence: apps/dreamverse/dreamverse/main.py:130, apps/dreamverse/dreamverse/config.py:50-55, apps/dreamverse/dreamverse/video_generation.py:367-370


[S1] AVAILABLE_LORAS style entries are not gated by the active model — selecting "Pixar" on a non-fast-ltx23 model silently loads wrong-architecture weights

What: AVAILABLE_LORAS["pixar"] and ["transition"] declare "model": "fast-ltx23" in config.py, but no consumer reads that field. /lora/options returns all keys regardless of the running model, and the apply_lora allow-list also doesn't filter. If the runtime is fast-ltx2, selecting "Pixar" downloads the LTX-2.3 LoRA and feeds it into set_lora_adapter, which tries to merge weights trained for a different transformer architecture into LTX-2 layers.

Why it matters: At best the merge raises mid-loop (caught and re-raised at lora_pipeline.py:386) and the user sees a 500. At worst the shapes are similar enough to silently produce garbage video. Cross-model LoRA contamination is a canonical LoRA-tooling bug — gating on the declared "model" field is the standard fix.

Suggested fix: Add a helper _available_styles_for_active_model() returning [k for k, v in AVAILABLE_LORAS.items() if v["model"] == _active_model_key()] and use it in both /lora/options and the apply_lora allow-list. _active_model_key should derive from the same source as MODEL_CONFIG selection.

Evidence: apps/dreamverse/dreamverse/config.py:33-44, apps/dreamverse/dreamverse/main.py:115-116, apps/dreamverse/dreamverse/main.py:125-126


[S1] Zero test coverage for cross-cutting LoRA-core changes — merge_lora_weights(accumulate=True) and stacked composition unverified

What: The PR changes the math inside BaseLayerWithLoRA.merge_lora_weights in two ways: adds a strength multiplier (lora_delta *= scale * self.lora_strength) and adds an accumulate mode that skips the unmerge_lora_weights() reset so the second LoRA in a stack is merged on top of the first's already-merged weights. Both DTensor and non-DTensor branches were edited. The only existing LoRA test (test_lora_inference_similarity.py::test_merge_lora_weights) exercises only the old single-adapter path with default strength=1.0, accumulate=False. The PR's only test diff is fastvideo/tests/modal/pr_test.py, which is the Modal CI shim, not a LoRA test.

Why it matters: The accumulate path is the heart of the feature ("LoRA stacks" in the PR title) and is load-bearing for every Dreamverse deployment that ships with a 2-element default stack. If lora_delta *= scale * self.lora_strength introduces a dtype mismatch on certain backends, or if DTensor reshard interacts badly with merge-on-top, regressions will surface as silently-wrong video output, not a crash.

Suggested fix: Add one unit test (~50 lines): construct two trivial LoRA tensors with identity-like A·B, call set_lora_adapter twice (second with accumulate=True), assert merged weight equals base + delta_1 + delta_2. Plus a strength sweep asserting merge(strength=s) yields base + s * delta. Optional: a TestClient round-trip on /lora asserting 400 on unknown style (also gates the two S1 endpoint bugs above).

Evidence: fastvideo/layers/lora/linear.py:114-169, fastvideo/pipelines/lora_pipeline.py:296-297


[S2] DREAMVERSE_LORA_STRENGTH = float(os.getenv(...)) crashes app startup on malformed env

What: config.py:77 does a bare float(os.getenv("DREAMVERSE_LORA_STRENGTH", "1.0")). A non-numeric env value (e.g. "high") raises ValueError during dreamverse.config import, so the FastAPI app never starts. Inconsistent with _parse_lora_stack (a few lines above) which DOES catch ValueError.

Why it matters: Operational footgun. Bare traceback at process startup with no recovery.

Suggested fix:

try:
    DREAMVERSE_LORA_STRENGTH = float(os.getenv("DREAMVERSE_LORA_STRENGTH", "1.0"))
except ValueError:
    DREAMVERSE_LORA_STRENGTH = 1.0

Or a _env_float(name, default) helper mirroring _env_bool.

Evidence: apps/dreamverse/dreamverse/config.py:77


[S2] Per-tick re-merge cycle is heavy — full unmerge + registry reset + reload-from-disk for each 250ms slider tick

What: Every 250ms slider tick triggers apply_lora_stack, which (1) calls generator.unmerge_lora_weights(), (2) calls _reset_lora_registry via collective_rpc to clear pipeline.lora_adapters AND blank cur_adapter_path/cur_adapter_name, then (3) loops set_lora_adapter(...) per LoRA. Because cur_adapter_path was blanked, lora_path != self.cur_adapter_path is always true, so load_file(lora_local_path) re-reads the safetensors from disk every tick (HF cache is hit but the file read is real), then re-merges into every LoRA layer.

Why it matters: For 200-800 MB LoRAs this is multi-hundred-ms per LoRA per slider tick (×2 for the default stack). The slider visibly stutters and starves concurrent generations on the same GPU. The "live" framing in the slider label is misleading.

Suggested fix: Cheap: keep pipeline.lora_adapters populated across ticks (only blank cur_adapter_name so the merge-with-cached-weights branch runs). Better: factor out set_lora_strength(nickname, strength) that updates self.lora_strength on each BaseLayerWithLoRA and re-runs only merge_lora_weights — no re-load.

Evidence: apps/dreamverse/dreamverse/video_generation.py:351-382, fastvideo/pipelines/lora_pipeline.py:312-322


[S2] set_lora_adapter early-return at lora_pipeline.py:355 drops strength-only changes for future callers

What: The mixin's early-return (if not adapter_updated and self.cur_adapter_name == lora_nickname: return) skips re-merge when the same nickname is re-applied with the same path. With the new signature, callers can legitimately want to change strength with the same nickname+path. apply_lora_stack only sidesteps this by forcibly blanking cur_adapter_name via _reset_lora_registry (the source of the S2 above). A future direct caller of set_lora_adapter(nickname=X, path=X_path, strength=0.5) will silently get no strength change.

Why it matters: Footgun for any future caller of the new signature. The contract should be: "if strength changed, re-merge."

Suggested fix: Track self.cur_adapter_strength and include it in the early-return check:

if (not adapter_updated and self.cur_adapter_name == lora_nickname
        and self.cur_adapter_strength == strength and not accumulate):
    return
self.cur_adapter_strength = strength

Evidence: fastvideo/pipelines/lora_pipeline.py:355-357


[S2] Frontend STYLE_LABELS hardcodes the same style strings the backend used to hardcode — config drift

What: LoraControls.tsx:9-13 defines STYLE_LABELS = { none, pixar, transition }. The component fetches /lora/options for the dropdown options but uses local labels. Adding a fourth style backend-side will silently show its raw key (e.g. "watercolor") in the dropdown — there's no signal that the new style is "UI-supported" without a frontend rebuild.

Why it matters: Same anti-pattern Gemini flagged on the backend, now mirrored on the frontend. The STYLE_LABELS[option] ?? option fallback prevents a crash, so this is a smell more than a bug — kept at S2 because fixing it also tightens the data structure that the S1 model-gating fix will touch.

Suggested fix: Either return {styles: [{key, label}, ...]} from /lora/options (backend owns labels), or accept the fallback and document it.

Evidence: apps/dreamverse/web/src/components/devtools/LoraControls.tsx:9-13, fallback at line 96


— Gob (@SolitaryThinker's AI reviewer). Full review (including S3 items and verified-clean areas) is archived locally.

1 similar comment
@SolitaryThinker

Copy link
Copy Markdown
Collaborator

Hi @H1yori233 — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off.

TL;DR

Live LoRA-strength + style-stacking plumbing reads cleanly: executor parity (multiproc ↔ ray) is symmetric, the new set_lora_adapter signature is backward-compatible for all 4 in-repo callers, and the React debounce cleanup Gemini flagged is already in place. Two real correctness bugs and one heavy-cost path remain in the hot loop, plus zero test coverage for the new accumulate=True math.

Verdict: ship-with-fixes

  • S0 (blockers): 0
  • S1 (must-fix): 3
  • S2 (should-fix): 4
  • S3 (discussion): not shown here; see archived review

Findings (formatted for upload)

[S1] apply_lora always pushes ("omninft", strength) without verifying the active model has an OmniNFT LoRA repo

What: The /lora POST endpoint unconditionally builds stack = [("omninft", strength)]. _resolve_lora_spec("omninft") returns MODEL_CONFIG.get("lora_repo"), which is None for any model that doesn't declare lora_repo. apply_lora_stack then silently filters Nones out of resolved_stack, so the request returns 200 with a strength echo and gpus: {0: null} even though no LoRA was touched.

Why it matters: Silent no-op on a user-visible control. The UI says "applied" but nothing changed. Regression hazard for any deployment using a model without lora_repo or a custom DREAMVERSE_MODEL_PATH.

Suggested fix: In apply_lora, if _resolve_lora_spec("omninft") is None, either return 400 ("This model has no OmniNFT LoRA") or surface "applied": false in the response so the UI can warn. Alternatively, hide the OmniNFT strength slider in the frontend when /lora/options indicates the model has no base LoRA.

Evidence: apps/dreamverse/dreamverse/main.py:130, apps/dreamverse/dreamverse/config.py:50-55, apps/dreamverse/dreamverse/video_generation.py:367-370


[S1] AVAILABLE_LORAS style entries are not gated by the active model — selecting "Pixar" on a non-fast-ltx23 model silently loads wrong-architecture weights

What: AVAILABLE_LORAS["pixar"] and ["transition"] declare "model": "fast-ltx23" in config.py, but no consumer reads that field. /lora/options returns all keys regardless of the running model, and the apply_lora allow-list also doesn't filter. If the runtime is fast-ltx2, selecting "Pixar" downloads the LTX-2.3 LoRA and feeds it into set_lora_adapter, which tries to merge weights trained for a different transformer architecture into LTX-2 layers.

Why it matters: At best the merge raises mid-loop (caught and re-raised at lora_pipeline.py:386) and the user sees a 500. At worst the shapes are similar enough to silently produce garbage video. Cross-model LoRA contamination is a canonical LoRA-tooling bug — gating on the declared "model" field is the standard fix.

Suggested fix: Add a helper _available_styles_for_active_model() returning [k for k, v in AVAILABLE_LORAS.items() if v["model"] == _active_model_key()] and use it in both /lora/options and the apply_lora allow-list. _active_model_key should derive from the same source as MODEL_CONFIG selection.

Evidence: apps/dreamverse/dreamverse/config.py:33-44, apps/dreamverse/dreamverse/main.py:115-116, apps/dreamverse/dreamverse/main.py:125-126


[S1] Zero test coverage for cross-cutting LoRA-core changes — merge_lora_weights(accumulate=True) and stacked composition unverified

What: The PR changes the math inside BaseLayerWithLoRA.merge_lora_weights in two ways: adds a strength multiplier (lora_delta *= scale * self.lora_strength) and adds an accumulate mode that skips the unmerge_lora_weights() reset so the second LoRA in a stack is merged on top of the first's already-merged weights. Both DTensor and non-DTensor branches were edited. The only existing LoRA test (test_lora_inference_similarity.py::test_merge_lora_weights) exercises only the old single-adapter path with default strength=1.0, accumulate=False. The PR's only test diff is fastvideo/tests/modal/pr_test.py, which is the Modal CI shim, not a LoRA test.

Why it matters: The accumulate path is the heart of the feature ("LoRA stacks" in the PR title) and is load-bearing for every Dreamverse deployment that ships with a 2-element default stack. If lora_delta *= scale * self.lora_strength introduces a dtype mismatch on certain backends, or if DTensor reshard interacts badly with merge-on-top, regressions will surface as silently-wrong video output, not a crash.

Suggested fix: Add one unit test (~50 lines): construct two trivial LoRA tensors with identity-like A·B, call set_lora_adapter twice (second with accumulate=True), assert merged weight equals base + delta_1 + delta_2. Plus a strength sweep asserting merge(strength=s) yields base + s * delta. Optional: a TestClient round-trip on /lora asserting 400 on unknown style (also gates the two S1 endpoint bugs above).

Evidence: fastvideo/layers/lora/linear.py:114-169, fastvideo/pipelines/lora_pipeline.py:296-297


[S2] DREAMVERSE_LORA_STRENGTH = float(os.getenv(...)) crashes app startup on malformed env

What: config.py:77 does a bare float(os.getenv("DREAMVERSE_LORA_STRENGTH", "1.0")). A non-numeric env value (e.g. "high") raises ValueError during dreamverse.config import, so the FastAPI app never starts. Inconsistent with _parse_lora_stack (a few lines above) which DOES catch ValueError.

Why it matters: Operational footgun. Bare traceback at process startup with no recovery.

Suggested fix:

try:
    DREAMVERSE_LORA_STRENGTH = float(os.getenv("DREAMVERSE_LORA_STRENGTH", "1.0"))
except ValueError:
    DREAMVERSE_LORA_STRENGTH = 1.0

Or a _env_float(name, default) helper mirroring _env_bool.

Evidence: apps/dreamverse/dreamverse/config.py:77


[S2] Per-tick re-merge cycle is heavy — full unmerge + registry reset + reload-from-disk for each 250ms slider tick

What: Every 250ms slider tick triggers apply_lora_stack, which (1) calls generator.unmerge_lora_weights(), (2) calls _reset_lora_registry via collective_rpc to clear pipeline.lora_adapters AND blank cur_adapter_path/cur_adapter_name, then (3) loops set_lora_adapter(...) per LoRA. Because cur_adapter_path was blanked, lora_path != self.cur_adapter_path is always true, so load_file(lora_local_path) re-reads the safetensors from disk every tick (HF cache is hit but the file read is real), then re-merges into every LoRA layer.

Why it matters: For 200-800 MB LoRAs this is multi-hundred-ms per LoRA per slider tick (×2 for the default stack). The slider visibly stutters and starves concurrent generations on the same GPU. The "live" framing in the slider label is misleading.

Suggested fix: Cheap: keep pipeline.lora_adapters populated across ticks (only blank cur_adapter_name so the merge-with-cached-weights branch runs). Better: factor out set_lora_strength(nickname, strength) that updates self.lora_strength on each BaseLayerWithLoRA and re-runs only merge_lora_weights — no re-load.

Evidence: apps/dreamverse/dreamverse/video_generation.py:351-382, fastvideo/pipelines/lora_pipeline.py:312-322


[S2] set_lora_adapter early-return at lora_pipeline.py:355 drops strength-only changes for future callers

What: The mixin's early-return (if not adapter_updated and self.cur_adapter_name == lora_nickname: return) skips re-merge when the same nickname is re-applied with the same path. With the new signature, callers can legitimately want to change strength with the same nickname+path. apply_lora_stack only sidesteps this by forcibly blanking cur_adapter_name via _reset_lora_registry (the source of the S2 above). A future direct caller of set_lora_adapter(nickname=X, path=X_path, strength=0.5) will silently get no strength change.

Why it matters: Footgun for any future caller of the new signature. The contract should be: "if strength changed, re-merge."

Suggested fix: Track self.cur_adapter_strength and include it in the early-return check:

if (not adapter_updated and self.cur_adapter_name == lora_nickname
        and self.cur_adapter_strength == strength and not accumulate):
    return
self.cur_adapter_strength = strength

Evidence: fastvideo/pipelines/lora_pipeline.py:355-357


[S2] Frontend STYLE_LABELS hardcodes the same style strings the backend used to hardcode — config drift

What: LoraControls.tsx:9-13 defines STYLE_LABELS = { none, pixar, transition }. The component fetches /lora/options for the dropdown options but uses local labels. Adding a fourth style backend-side will silently show its raw key (e.g. "watercolor") in the dropdown — there's no signal that the new style is "UI-supported" without a frontend rebuild.

Why it matters: Same anti-pattern Gemini flagged on the backend, now mirrored on the frontend. The STYLE_LABELS[option] ?? option fallback prevents a crash, so this is a smell more than a bug — kept at S2 because fixing it also tightens the data structure that the S1 model-gating fix will touch.

Suggested fix: Either return {styles: [{key, label}, ...]} from /lora/options (backend owns labels), or accept the fallback and document it.

Evidence: apps/dreamverse/web/src/components/devtools/LoraControls.tsx:9-13, fallback at line 96


— Gob (@SolitaryThinker's AI reviewer). Full review (including S3 items and verified-clean areas) is archived locally.

@mergify mergify Bot added the scope: infra CI, tests, Docker, build label Jun 1, 2026
@SolitaryThinker

Copy link
Copy Markdown
Collaborator

Hi @H1yori233 — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off.

TL;DR

Re-review of 4e4d3c56 against prior b23da454: the single net-new "some fix" commit addresses all 7 surfaced prior findings (3×S1 + 4×S2) — model-active gating, safe env-float helper, slider re-merge no longer reloading from disk, strength-aware early-return, backend-supplied style labels, and a new test_merge_lora_math.py covering the previously-untested strength + accumulate math. No regressions, no new S1 issues.

Verdict: approve (was: ship-with-fixes)

  • S0 (blockers): 0
  • S1 (must-fix): 0
  • S2 (should-fix): 0
  • S3 (discussion): not shown here; see archived review

Prior findings status

# Prior finding Status at 4e4d3c5 Address commit Evidence
S1.1 apply_lora unconditional ("omninft", strength) push 4e4d3c56 main.py:138-148 — stack starts empty, ("omninft", strength) only appended if _resolve_lora_spec("omninft") is not None; empty-stack returns HTTP 400.
S1.2 AVAILABLE_LORAS styles not model-gated 4e4d3c56 config.py:208-211 adds _available_styles_for_active_model() filtering by v.get("model") == _active_model_key(); consumed by both /lora/options and allow-list.
S1.3 Zero test coverage for accumulate=True / strength!=1 4e4d3c56 New fastvideo/tests/inference/lora/test_merge_lora_math.py (53 lines): parametrized strength sweep, alpha-scale, and stacked-accumulate sum-of-deltas tests.
S2.1 DREAMVERSE_LORA_STRENGTH = float(os.getenv(...)) crash 4e4d3c56 config.py:240 now uses _env_float("DREAMVERSE_LORA_STRENGTH", 1.0); helper at config.py:106-114 wraps float(value) in try/except returning default.
S2.2 Per-tick re-merge reloads-from-disk 4e4d3c56 _reset_lora_registry no longer clears lora_adapters or cur_adapter_path; lora_pipeline.py:314 disk-reload now gated on per-nickname memo lora_adapter_paths.get(lora_nickname) != lora_path. Same-path slider ticks skip maybe_download_lora + load_file.
S2.3 set_lora_adapter early-return drops strength-only 4e4d3c56 lora_pipeline.py:366-368 early-return now includes self.cur_adapter_strength == strength and not accumulate; new cur_adapter_strength class attr at lora_pipeline.py:105.
S2.4 Frontend STYLE_LABELS hardcoded 4e4d3c56 Backend /lora/options now returns a labels map sourced from AVAILABLE_LORAS[k].get("label", k); frontend LoraControls.tsx merges server labels over the static STYLE_LABELS fallback.

Tally: 7/7 ✅.


New findings

(none at S1/S2)

Two minor S3-style observations are captured in the archived review:

  • _reset_lora_registry resets cur_adapter_strength = 1.0 rather than a sentinel — dormant footgun, not a runtime bug today.
  • lora_adapter_paths survives _reset_lora_registry; correct for the slider use-case but worth documenting as "valid iff the pipeline never retargets a different base model" if a runtime model-swap path is ever added.

Neither is blocking.


— Gob (@SolitaryThinker's AI reviewer). Full review (including S3 items) is archived locally.

1 similar comment
@SolitaryThinker

Copy link
Copy Markdown
Collaborator

Hi @H1yori233 — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off.

TL;DR

Re-review of 4e4d3c56 against prior b23da454: the single net-new "some fix" commit addresses all 7 surfaced prior findings (3×S1 + 4×S2) — model-active gating, safe env-float helper, slider re-merge no longer reloading from disk, strength-aware early-return, backend-supplied style labels, and a new test_merge_lora_math.py covering the previously-untested strength + accumulate math. No regressions, no new S1 issues.

Verdict: approve (was: ship-with-fixes)

  • S0 (blockers): 0
  • S1 (must-fix): 0
  • S2 (should-fix): 0
  • S3 (discussion): not shown here; see archived review

Prior findings status

# Prior finding Status at 4e4d3c5 Address commit Evidence
S1.1 apply_lora unconditional ("omninft", strength) push 4e4d3c56 main.py:138-148 — stack starts empty, ("omninft", strength) only appended if _resolve_lora_spec("omninft") is not None; empty-stack returns HTTP 400.
S1.2 AVAILABLE_LORAS styles not model-gated 4e4d3c56 config.py:208-211 adds _available_styles_for_active_model() filtering by v.get("model") == _active_model_key(); consumed by both /lora/options and allow-list.
S1.3 Zero test coverage for accumulate=True / strength!=1 4e4d3c56 New fastvideo/tests/inference/lora/test_merge_lora_math.py (53 lines): parametrized strength sweep, alpha-scale, and stacked-accumulate sum-of-deltas tests.
S2.1 DREAMVERSE_LORA_STRENGTH = float(os.getenv(...)) crash 4e4d3c56 config.py:240 now uses _env_float("DREAMVERSE_LORA_STRENGTH", 1.0); helper at config.py:106-114 wraps float(value) in try/except returning default.
S2.2 Per-tick re-merge reloads-from-disk 4e4d3c56 _reset_lora_registry no longer clears lora_adapters or cur_adapter_path; lora_pipeline.py:314 disk-reload now gated on per-nickname memo lora_adapter_paths.get(lora_nickname) != lora_path. Same-path slider ticks skip maybe_download_lora + load_file.
S2.3 set_lora_adapter early-return drops strength-only 4e4d3c56 lora_pipeline.py:366-368 early-return now includes self.cur_adapter_strength == strength and not accumulate; new cur_adapter_strength class attr at lora_pipeline.py:105.
S2.4 Frontend STYLE_LABELS hardcoded 4e4d3c56 Backend /lora/options now returns a labels map sourced from AVAILABLE_LORAS[k].get("label", k); frontend LoraControls.tsx merges server labels over the static STYLE_LABELS fallback.

Tally: 7/7 ✅.


New findings

(none at S1/S2)

Two minor S3-style observations are captured in the archived review:

  • _reset_lora_registry resets cur_adapter_strength = 1.0 rather than a sentinel — dormant footgun, not a runtime bug today.
  • lora_adapter_paths survives _reset_lora_registry; correct for the slider use-case but worth documenting as "valid iff the pipeline never retargets a different base model" if a runtime model-swap path is ever added.

Neither is blocking.


— Gob (@SolitaryThinker's AI reviewer). Full review (including S3 items) is archived locally.

@H1yori233

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for stackable LoRA adapters with adjustable strengths and intensities, including a new backend API, worker IPC commands, pipeline updates for weight accumulation, and a frontend control panel in the devtools. The review feedback highlights several critical issues: applying the LoRA stack globally across the GPU pool can overwrite settings for other active users in a multi-user environment; unmerging LoRA weights can raise a ValueError and leave layers in an inconsistent state; substring matching for style triggers can cause false matches; and the frontend slider controls are susceptible to race conditions from out-of-order asynchronous responses.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread apps/dreamverse/dreamverse/main.py
Comment thread apps/dreamverse/dreamverse/video_generation.py
Comment thread apps/dreamverse/dreamverse/video_generation.py
Comment thread apps/dreamverse/web/src/components/devtools/LoraControls.tsx
Comment thread apps/dreamverse/web/src/components/devtools/LoraControls.tsx
@SolitaryThinker

Copy link
Copy Markdown
Collaborator

/merge

@github-actions github-actions Bot added the ready PR is ready to merge label Jun 2, 2026
@SolitaryThinker

Copy link
Copy Markdown
Collaborator

Hi @H1yori233 — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off.

TL;DR

The delta 4e4d3c56..18fbb71c ships a meaningful API + UX upgrade beyond the prior review scope: /lora now accepts styles: dict[str, float] for multi-style stacking with per-style intensities (legacy style: str kwarg still honored), both /lora endpoints are now DEVTOOLS_ENABLED-gated (404 otherwise — nice hardening), trigger composition partitions all stack entries into prepend/append lists with regex word-boundary dedup, and 66 lines of TestClient tests cover stacking, validation, back-compat, clamping, and the devtools-disabled branch. All 7 prior closures stayed closed; the IPC contract change (LoraStackPayload.style dropped) is wired through all 4 callsites cleanly. Verdict drops one tier to ship-with-fixes for one cross-cutting change to shared FastVideo code that softens an existing invariant.

Verdict: ship-with-fixes (was: approve)

  • S0 (blockers): 0
  • S1 (must-fix): 0
  • S2 (should-fix): 1
  • S3 (discussion): not shown here; see archived review

Prior findings status

# Prior finding Status at 18fbb71c
S1.1 apply_lora unconditional ("omninft", strength) push ✅ unchanged
S1.2 AVAILABLE_LORAS styles not model-gated ✅ unchanged
S1.3 Zero test coverage for accumulate=True / strength!=1 ✅ unchanged
S2.1 DREAMVERSE_LORA_STRENGTH = float(os.getenv(...)) crash on bad env ✅ unchanged
S2.2 Per-tick re-merge reloads-from-disk ✅ unchanged
S2.3 set_lora_adapter early-return drops strength-only changes ✅ unchanged
S2.4 Frontend STYLE_LABELS hardcoded ✅ unchanged

Net-new findings

[S2-new-A] fastvideo/layers/lora/linear.py:179 softens unmerge_lora_weights invariant on shared code

The delta flips:

if not self.merged:
    raise ValueError("LoRA weights not merged. Please merge them first before unmerging.")

to:

if not self.merged:
    return

This file is shared FastVideo code, not Dreamverse-scoped. Audit of all 5 callers (grep -rn "unmerge_lora_weights"):

  • fastvideo/pipelines/lora_pipeline.py:437 — bulk iteration across all LoRA layers; previously surfaced any double-unmerge as a ValueError, now silently skips per-layer.
  • fastvideo/entrypoints/video_generator.py:1251 → executor RPC — same: now silently skips per worker.
  • examples/inference/lora/wan_lora_inference_from_ckpt.py:20 — example script, would previously raise on pre-merge call.
  • apps/dreamverse/dreamverse/video_generation.py:356 (your new caller) — already wraps the call in try/except Exception; the change makes the not-merged branch of that try/except dead-code.
  • linear.py:119 (merge_lora_weights self-call) — already guarded by if self.merged:, unaffected.

Only the Dreamverse caller benefits, and it already has its own try/except. The other 3 legacy paths lose a defensive invariant check that would have surfaced unmerge-before-merge / double-unmerge bugs immediately. No test asserts the new silent-return contract; grep -rn "LoRA weights not merged" returns only the (now-removed) line.

Suggested fix. Either revert linear.py:179 to the raise and rely on the existing try/except in video_generation.py:355-358 (it already swallows exceptions and prints a log), or keep the silent return but add a brief comment + a small pytest documenting the new no-op contract.


— Gob (@SolitaryThinker's AI reviewer). Full review (including S3 items) is archived locally.

Address review S2: unmerge_lora_weights silently returned when not merged,
softening an invariant on shared FastVideo LoRA code (legacy train +
inference). Option A adds an opt-in strict keyword while preserving the
current tolerant default for existing no-argument callers.
@SolitaryThinker
SolitaryThinker merged commit d3a821c into main Jun 2, 2026
14 of 21 checks passed
@SolitaryThinker
SolitaryThinker deleted the feat/kaiqin/dreamverse_lora branch June 2, 2026 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready PR is ready to merge scope: inference Inference pipeline, serving, CLI scope: infra CI, tests, Docker, build scope: model Model architecture (DiTs, encoders, VAEs) type: feat New feature or capability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants