Repository navigation
Sync with upstream master-951-f89d9b1 - #24
Conversation
Co-authored-by: leejet <leejet714@gmail.com>
Co-authored-by: leejet <leejet714@gmail.com>
Co-authored-by: leejet <leejet714@gmail.com>
Co-authored-by: leejet <leejet714@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af486f1678
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| float attn_scale; // Override flash-attention K/V scaling; 0 keeps the model default | ||
| const char* tokenizer; // tokenizer.json path or main=FILE,clip-l=FILE,clip-g=FILE assignments; required for PiD and Lens | ||
| bool sage_attn; | ||
| int conditioning_cache_size; // Maximum cached conditioning entries per context; 0 disables caching (default: 4) |
There was a problem hiding this comment.
When an application compiled against the previous public header loads the updated shared library, its sd_ctx_params_t storage ends at sage_attn, but the new sd_ctx_params_init() uses the library's larger sizeof(sd_ctx_params_t) and overwrites beyond the caller's allocation; callers that skip the initializer still have this new field read out of bounds. The appended image_preprocess fields cause the same problem for the image/video generation structs, so these additions need a size/version mechanism or versioned types before they can be exposed safely.
AGENTS.md reference: AGENTS.md:L69-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. The new fields are upstream's (leejet#2028, leejet#2034, leejet#2059); this PR only merges them. The fork has no shared-library consumer to break: the prebuilt bundles ship statically linked sd-cli and sd-server (no libstable-diffusion), and Studio runs those executables, so nothing loads a newer library against an older header. Adding a size/version scheme to upstream's structs here would also diverge from upstream's API on every later sync.
| const sd_lora_t* loras, | ||
| int lora_count); |
There was a problem hiding this comment.
Existing binaries call the exported C symbol convert_with_components with 11 arguments, but this change keeps the same symbol while requiring two more. On an in-place shared-library upgrade, the implementation therefore interprets unspecified register/stack contents as loras and lora_count; a non-null garbage pointer can be dereferenced immediately while logging or applying LoRAs. Preserve the old signature as a wrapper and add a separately named/versioned entry point for the LoRA arguments.
AGENTS.md reference: AGENTS.md:L69-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. The two LoRA arguments come from upstream (leejet#2079, merging LoRA weights on conversion); this PR only merges them. The fork distributes statically linked sd-cli and sd-server only, so no existing binary resolves convert_with_components from an upgraded shared library. A fork-only wrapper and renamed entry point would diverge from upstream's API on every later sync.
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0d47fc2af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Each batch item has a base pass and, when enabled, a hires pass. | ||
| g_preview.job_count = static_cast<int64_t>(std::max(1, params.batch_count)) * (params.hires.enabled ? 2 : 1); | ||
| g_preview.job_no = 0; | ||
| sd_set_preview_callback(preview_callback, PREVIEW_PROJ, 1, true, false, nullptr); |
There was a problem hiding this comment.
When the loaded model lacks a PREVIEW_PROJ mapping, the preview callback is never invoked, so /sdapi/v1/progress remains at step/progress zero for the entire request. This affects the newly supported PixArt family: it uses a 4-channel VAE, but preview_image() only recognizes SD1/SD2/SDXL in its 4-channel projection branch and returns before calling this callback. Track progress independently of latent projection, or add an appropriate PixArt preview path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this here. This is upstream's server code (the SDAPI preview/progress endpoints from leejet#2093 and leejet#2096, and PixArt from leejet#2047), merged unchanged; the fork does not touch examples/server or preview_image(). Studio does not use /sdapi/v1/progress or PixArt through sd.cpp. A fix belongs upstream, where it would reach this fork on the next sync.
| for (int i = 0; i < lora_count; i++) { | ||
| ModelManager::LoraSpec spec; | ||
| spec.path = loras[i].path != nullptr ? loras[i].path : ""; | ||
| spec.multiplier = loras[i].multiplier; | ||
| spec.is_high_noise = loras[i].is_high_noise; | ||
| spec.required = true; |
There was a problem hiding this comment.
When the array contains at least one nonzero multiplier, has_active_loras() selects the in-memory path, but this loop also turns every zero-multiplier entry into a required source. Consequently, a caller that keeps a missing or optional LoRA entry disabled with multiplier 0 alongside an active LoRA fails in prepare_lora_sources(), even though the active check explicitly treats that entry as inactive. Omit zero-multiplier entries from the generated specs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this here. build_lora_specs() and has_active_loras() are upstream's code from leejet#2079 (LoRA merging on conversion), merged unchanged; the fork does not modify src/convert.cpp, and Studio does not run sd.cpp conversion with LoRAs. Diverging from upstream in this file for it would cost a conflict on every later sync; the fix belongs upstream.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0d47fc2af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return true; | ||
| }; | ||
| if (!load_image("init_image", 0, init_image) || |
There was a problem hiding this comment.
Preserving the source channel count here allows grayscale and grayscale-alpha PNGs to enter generation with 1 or 2 channels. For ordinary 3-channel models, ensure_image_tensor_channels() in src/pipeline/image.cpp only handles >=3 -> 3, so these valid inputs reach its GGML_ABORT and terminate the CLI/server process; 2-channel inputs also abort for 4-channel models. Convert 1/2-channel images to the requested RGB/RGBA layout before passing them to the generation pipeline, or extend that helper to handle both layouts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reproduced. On this branch (CPU build of d0d47fc), Z-Image img2img with a grayscale (L) and a grayscale+alpha (LA) PNG completes through both entry points: sd-cli -i, and sd-server POST /sdcpp/v1/img_gen with init_image as base64 (the from_json_str path this comment points at). The log shows the IMG2IMG branch and a successful encode_first_stage for both, with no abort. This loader is also upstream's code (leejet#2021, leejet#2028), merged unchanged.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0d47fc2af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (int c = 0; c < channels; ++c) { | ||
| float fill = rgba[channels == 1 ? 0 : c] / 255.f; |
There was a problem hiding this comment.
When a C API caller supplies a two-channel grayscale-alpha image with mode=fit-pad, the second output channel reads rgba[1] (green) rather than rgba[3] (alpha). Consequently, even the default #000000ff creates transparent padding instead of opaque black, and any explicitly requested padding alpha is ignored; map channel 1 to the RGBA alpha component when channels == 2.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this here. src/runtime/image_preprocess.cpp is upstream's (leejet#2028), merged unchanged, and the case needs a C API caller passing a 2-channel image with fit-pad: the fork's sd-cli and sd-server decode inputs to RGB/RGBA before preprocessing, and Studio sends RGB/RGBA. The fix belongs upstream.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
19 participants
Syncs the fork with upstream
master-951-f89d9b1(2026-10-09). The last sync, #15 on 2026-09-21, brought the tree up tomaster-889-c678dfe, so this adds 62 upstream commits. Among them are fixes Studio users need:f583f39->d25b121).Merge this with "Create a merge commit", not squash or rebase. #15 was squashed, so git never recorded it as a merge. Git still thinks the last sync was the 2026-08-07 merge, and a plain
git mergere-raised every conflict #15 had already resolved. The first commit here (e739cd3) isgit merge -s ours c678dfe. It records #15's sync point without changing a file: the tree is identical to master, andc678dfe..mastercontains only fork files. The real merge (8a48526) therefore usedc678dfeas its base. A squash would throw that away again.What needed more than a textual merge
Seven files conflicted. Beyond those, these upstream changes overlap in meaning with fork code:
scripts/unsloth/ggml-patchesstopped applying, so the prebuilt pipeline would have failed at its patch step. They are rebased ontod25b121withgit am -3. Each patch keeps its author and message, and applying them to a cleand25b121reproduces the rebased tree exactly.SOL_ATTNandROPE_APPLY(GGML_OP_COUNT108).sd_tiling_seam_safe_tile_sizeworked around. The thin case still happens with the new planner: on 62 latents with 32-latent tiles, two tiles overlap by only 2 latents. The guard is reimplemented on top ofsd_tiling_plan_axisand is unchanged in intent. It shrinks the tile first; it can widen it to at most 2x; every interior overlap is at least half the target and at least 2 latents.--vae-tiling, so nothing it sends changes meaning. MiniMax-H3 resolves its tiling through upstream'sresolve_tiling_params, andSD_H3_VAE_TILE=Nstill means N latents. The H3 batched decode now handles circular axes the same way asVAE::tiled_compute.SD_H3_VAE_KEEP_RESIDENT, so the fork's decode loop is kept as is.close-organization-fork-prs.ymlis a new upstream workflow that closes PRs opened from organization-owned forks. It is deleted here: that is upstream's contribution policy, not this repository's.Behaviour that changes with upstream
f89d9b1that this PR ran is pixel-identical (Z-Image 1024 untiled/tiled and 1008 tiled on CUDA, SD1.5 and Z-Image on Metal), so the change comes from upstream.--vae-tiling: the H3 video VAE now uses--vae-tile-overlap(default 0.5) instead of a forced 0.25. Studio passes--vae-tilingonly under its low-VRAM offload policies.Verification
Arms are master
4a98febvs this branch, and pure upstreamf89d9b1where noted.CUDA, local (RTX 6000 Ada sm89, RTX 3090 sm86, cuDNN on)
test-backend-ops, full suite: 16597/16597 on both GPUs.sd_tiling_seam_safe_tile_sizeinto an emulation of the loop: all 2382 cases (dims 4-400, overlap 0.5/0.25/0.1, scale 8/16) finish within 6 attempts.AMD Strix Halo gfx1151 (DevLab runners)
Builds on HIP, Linux Vulkan and Windows Vulkan. All op tests pass: FLASH_ATTN_EXT, CPY, MUL_MAT, RMS_NORM, ROPE, the fork's fused ops, ADD, MUL, CONV_2D_DW.
H3 at 640x384x56:
CPU reference check (2 steps, each arm vs its own CPU-backend render):
HIP OOM retries:
Metal (M1, 8 GB)
Prebuilt pipeline legs, replayed from this branch's
unsloth-sd-prebuilt.ymlon GitHub-hosted runners (signing skipped)After merging
The prebuilt workflow names a release after the highest
master-NNN-shatag reachable in this repository. Upstream's tags aftermaster-813were never copied here, so releases would still be calledmaster-813-.... Pushing upstream's tag fixes the name:Not covered
f89d9b1fails the same way at 9 and 8 GiB free. The new ggml reports the iGPU's real free memory, where the old one reported system memory (about 59 GB), so the DiT now switches to segments and runs out partway. This comes from upstream, not from the merge.