Skip to content

fix: address Ideogram review feedback - #20

Merged
gianni-cor merged 3 commits into
2026-07-03from
qvac/ideogram-review-fixes
Jul 13, 2026
Merged

fix: address Ideogram review feedback#20
gianni-cor merged 3 commits into
2026-07-03from
qvac/ideogram-review-fixes

Conversation

@aegioscy

Copy link
Copy Markdown

Summary

  • Register and apply optional Ideogram weight_scale tensors for linear layers so FP8-with-scale checkpoints are not silently run without their scales.
  • Use graph-reachable parameter staging for Ideogram non-segmented compute so cond/uncond passes only offload the active transformer weights.
  • Leaves the requested CFG/uncond-path enforcement comment out of scope per follow-up direction.

Test plan

  • git diff --check
  • cmake -S . -B build-ideogram-review -DSD_BUILD_EXAMPLES=OFF -DSD_BUILD_SHARED_LIBS=OFF -DGGML_NATIVE=OFF
  • cmake --build build-ideogram-review --config Release -j 8

Made with Cursor

aegioscy and others added 2 commits July 13, 2026 07:32
Support Ideogram linear weight_scale tensors and avoid staging inactive cond/uncond transformer params during non-segmented offload.

Co-authored-by: Cursor <cursoragent@cursor.com>
Fail Ideogram generation requests that need CFG when the standalone unconditional diffusion model was not successfully loaded.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread src/ideogram4.hpp Outdated
if (weight_scale->ne[0] == x->ne[0] && ggml_n_dims(weight_scale) == 1) {
weight_scale = ggml_reshape_3d(ctx->ggml_ctx, weight_scale, weight_scale->ne[0], 1, 1);
}
return ggml_mul(ctx->ggml_ctx, x, weight_scale);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This still applies weight_scale in the wrong order for biased linear layers. Linear::forward() has already added the bias, so this returns (xW + b) * weight_scale. For FP8 scaled weights the scale belongs to the weight product, not the bias; the expected computation is xW * weight_scale + b.

This matters for Ideogram because many of these linears have bias=true (input_proj, llm_cond_proj, timestep/adalan projections, final linear, etc.), so official FP8-with-scale checkpoints will still produce incorrect activations even though the scale tensor is now loaded. Please either teach Linear to handle weight_scale before bias, or have Ideogram4Linear call the underlying matmul without bias, multiply by weight_scale, then add bias afterward.

FP8 weight_scale belongs to the weight product, so compute the matmul without bias, multiply by weight_scale, then add bias: (xW)*scale + b instead of (xW + b)*scale.

Co-authored-by: Cursor <cursoragent@cursor.com>
@gianni-cor
gianni-cor merged commit 9f587ad into 2026-07-03 Jul 13, 2026
14 checks passed
@gianni-cor
gianni-cor deleted the qvac/ideogram-review-fixes branch July 13, 2026 09:04
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