Skip to content

Studio: add opt-in DSpark speculative decoding - #7968

Merged
danielhanchen merged 19 commits into
unslothai:mainfrom
oobabooga:studio-dspark-speculative-decoding
Aug 6, 2026
Merged

Studio: add opt-in DSpark speculative decoding#7968
danielhanchen merged 19 commits into
unslothai:mainfrom
oobabooga:studio-dspark-speculative-decoding

Conversation

@oobabooga

Copy link
Copy Markdown
Member

Adds opt-in DSpark speculative decoding for GGUF models in Studio and the managed CLI paths. DSpark stays out of Auto because it can substantially improve single-request speed, but adds significant VRAM use and may reduce concurrent throughput.

To test it, load unsloth/DeepSeek-V4-Flash-0731-GGUF and select DSpark. Studio downloads the recommended Q8_0 companion automatically and uses the tested default draft depth of n=3.

Performance

Tested with the DeepSeek V4 Flash 0731 Q3_K_M target and Q8_0 DSpark sidecar across coding-agent, QA, summarization, and mixed prompts.

Workload Target only DSpark Change
Single request, n=3 60.6 t/s 104.7 t/s +73%
Four concurrent, n=5 103.9 t/s 82.7 t/s -20%

For single requests, DSpark also improved throughput by 49% at n=2 and 35% at n=5. DFlash at n=5 was 55% slower for one request and 66% slower for four concurrent requests. Tested n-gram variants ranged from 19% slower to 13% faster.

Caveats and upstream status

  • DSpark should remain opt-in. The best single-request result was at n=3, while the only measured concurrent configuration, n=5, regressed.
  • The Q8_0 sidecar adds 10.90 GB and may displace target layers on smaller GPUs. DSpark requires fixed placement with --fit off; Studio falls back to llama.cpp's default n-gram mode if it cannot confirm that the target and sidecar fit. The model card also warns that heavy CPU offload can make DSpark slower.
  • Greedy output did not consistently match target-only output in testing. This remains tracked in llama.cpp #25618.
  • Core support is merged in llama.cpp #25173, and the required loader fix is merged in llama.cpp #26577. The current Unsloth b10265-mix-89aa77b build is gated because it aborts while loading DSpark, so standard installs still need a newer prebuilt containing the loader fix.

Implementation

  • Adds DSpark to the session-only Speculative Decoding selector and to unsloth studio run, unsloth inference, and unsloth chat through --speculative-type dspark and --spec-draft-n-max.
  • Finds local dspark-*.gguf companions or downloads the recommended Q8_0 sidecar only when DSpark is selected.
  • Includes the selected sidecar and draft KV cache in VRAM planning. Estimates were validated across Q3 and Q4 targets, Q8_0 and BF16 sidecars, 8K to 32K contexts, and draft depths 2 to 5, remaining 1.7% to 3.6% conservative.
  • Falls back safely when the sidecar, binary support, fixed placement, or speculative launch is unavailable.

Verification

  • Studio UI, OpenAI-compatible APIs, and all managed CLI paths loaded DSpark and completed coding, QA, and agent-style prompts consistently.
  • Reload deduplication, draft-depth changes, Off and DSpark transitions, companion discovery, VRAM planning, placement, and fallback paths were covered.
  • Focused backend suites passed 771 tests, all 843 CLI tests passed, and all 452 frontend tests plus type checking passed. ruff check and git diff --check also passed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 318a51cea8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +11549 to +11553
flags.extend(
[
"--model-draft", dspark_draft_path,
"--spec-type", "draft-dspark",
str(n_max_flag), str(draft_n_max),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Pin the DSpark sidecar to the selected draft device

When DSpark runs with a non-default placement (for example a Vulkan gpu_ids selection or a user --device), the caller already computes draft_device, but this DSpark branch never emits --spec-draft-device like the MTP drafter path does. Because DSpark is also launched as a separate --model-draft, llama.cpp can place the sidecar on its default device outside the planned/selected GPU pool, invalidating the VRAM fit and explicit GPU selection; pass the same draft-device flag for draft-dspark when draft_device is set.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not taking this one. Emitting --spec-draft-device for DSpark is the crash in ggml-org/llama.cpp#26475, which is still open.

The DSpark sidecar is converted without token_embd.weight and output.weight and borrows both from the target through ctx_other (see src/models/dflash.cpp, and PR ggml-org/llama.cpp#25173: "it shares the target's token-embeddings / lm_head"). Restricting the draft context to one device therefore leaves the target's pre-allocated output.weight unschedulable. I hit this directly while benchmarking the sidecar: pre-allocated tensor (output.weight) in a buffer (CUDA0) that cannot run the operation.

The candidate upstream fix, ggml-org/llama.cpp#26636, is still open and unmerged. Until it lands, the drafter has to span the same devices as the target, so stripping the device args is correct.

unslothai#7968

Three fixes for the failing CI on this branch:

- Drop the unused `Literal` import from unsloth_cli/commands/chat.py and
  inference.py. The options are typed with `SpeculativeType`, so the added
  import was a leftover and tripped the import-hoist blocker.

- Make test_the_startup_retry_drops_the_mtp_the_extras_and_the_env_carry
  whitespace insensitive. The guard now names both drafters, so formatting
  wrapped the call and the literal substring assertion no longer matched.
  It asserts on both `_extra_args_requests_mtp` and
  `_extra_args_requests_dspark` now, so the DSpark half is covered too.

- Gate DSpark on the whole broken build window instead of one tag. The
  reshape regression is ggml-org/llama.cpp#26531 and the fix is #26577, so
  every prebuilt based on b10259 through b10268 aborts on a DSpark load,
  not only b10265-mix-89aa77b. Matching the base build number keeps source
  builds unaffected, since those carry no install marker.
@danielhanchen

Copy link
Copy Markdown
Member

Pushed 22ce787 to clear the three failing checks on this branch.

Lint (import-hoist blocker)

Literal was added to the typing import in unsloth_cli/commands/chat.py and unsloth_cli/commands/inference.py, but both options are typed with SpeculativeType, so it was left unused. Removed from both.

test_the_startup_retry_drops_the_mtp_the_extras_and_the_env_carry

The retry guard now names both drafters, so formatting wrapped the call across lines and the literal substring assertion stopped matching. The behaviour was correct, only the source text assertion broke. It is now whitespace insensitive and asserts on _extra_args_requests_mtp and _extra_args_requests_dspark, so the DSpark half of the guard is covered too.

Broken build gate widened

_KNOWN_BROKEN_DSPARK_RELEASES matched the single tag b10265-mix-89aa77b. The underlying regression is ggml-org/llama.cpp#26531 and the fix is ggml-org/llama.cpp#26577, so the affected window is wider than one build. Checking both merge commits against the upstream tags:

b10258   break=n  fix=n
b10259   break=Y  fix=n     <- first broken
b10261   break=Y  fix=n
b10262   break=Y  fix=n
b10265   break=Y  fix=n     <- the tag that was listed
b10267   break=Y  fix=n
b10268   break=Y  fix=n     <- last broken
b10269   break=Y  fix=Y     <- first good

So it is now matched on the base build number over range(10259, 10269) rather than one exact tag. Source builds stay unaffected, since read_install_marker returns None for them and the gate only ever applies to a managed prebuilt. Added a small test for the window boundaries.

The gate is doing real work: b10265-mix-89aa77b is still the latest release on unslothai/llama.cpp, so DSpark falls back on a standard install until a prebuilt is cut from b10269 or newer. Worth noting that the previous prebuilt b10241-mix-89aa77b is not a way out either, since it predates ggml-org/llama.cpp#22789 and aborts on multi GPU with GGML_ASSERT(n_graph_inputs < GGML_SCHED_MAX_SPLIT_INPUTS). b10269 or newer is the first build where DSpark works both on load and across devices.

Verification

ruff check                       clean on all changed files
verify_import_hoist.py           OVERALL: PASS (no blockers), self-test all pass
test_llama_cpp_mtp_detection.py  510 passed (with the two other suites below)
test_metal_paravirtual_guard.py  passed, including the previously failing case
test_mtp_drafter_companion.py    passed
unsloth_cli/tests/test_inference_chat.py   59 passed

The remaining Windows Chat UI failure is unrelated to this branch. It is Voice model picker did not wheel-scroll, a Playwright timeout with nothing DSpark specific in it.

On the n=3 default

I benchmarked the sidecar separately on 4x B200 against UD-Q4_K_XL, using a 7 turn conversation with the reply appended each turn, greedy, 3 time blocked repetitions per config with the order reversed between blocks. Conversation level decode throughput, computed as total generated tokens over total generation time:

Config decode tok/s Speedup Acceptance
target only 61.09 (60.51 to 61.38) 1.000x n/a
DSpark n=2 95.73 (85.98 to 97.31) 1.567x 0.873
DSpark n=3 113.64 (109.72 to 113.89) 1.860x 0.847
DSpark n=5 95.13 (87.31 to 95.77) 1.557x 0.726

Your n=3 default is the right call, about 19 percent ahead of both neighbours. That also makes this PR's default better than what our own model card currently shows, which is --spec-draft-n-max 5. I will fix the card.

One difference worth flagging against your table. For 4 concurrent requests I am measuring DSpark ahead of target only on this hardware rather than behind it, which does not match the -20 percent you saw at n=5. Different GPUs and a different draft depth, so not a contradiction, but it suggests the concurrency regression may be setup specific rather than general. These B200s sit at 22 to 28 percent utilisation under a layer split, so there is idle headroom for the drafter to use that a busier box would not have. I will post the confirmed numbers once the second block finishes.

Keeping DSpark opt in still looks right to me regardless, given the VRAM cost.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22ce787629

ℹ️ 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".

Comment on lines +1627 to +1630
return (
bool(stem)
and weight.startswith(stem)
and (len(weight) == len(stem) or not weight[len(stem)].isalnum())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Require an exact family match before attaching a drafter

For prefix-related models in one local directory, this predicate still matches in the shorter-to-longer direction: dspark-DeepSeek-V4-Flash-Q8_0.gguf is accepted for DeepSeek-V4-Flash-Lite-Q4_K_M.gguf because the next target character is -. If the longer family lacks its own sidecar, Studio therefore launches the other model's drafter, which can fail startup or run an incompatible speculative model. Normalize the target's shard/quant suffix too and compare family identities rather than accepting any delimiter-bounded prefix.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The shorter-to-longer match reproduces, but an exact family match would break real published input, so leaving it.

That shape is exactly how every unsloth gemma-4 qat MTP drafter pairs. Checked against the live Hub, not just the docstring:

repo drafter weight
gemma-4-12B-it-qat-GGUF MTP/mtp-gemma-4-12B-it-BF16.gguf gemma-4-12B-it-qat-UD-Q4_K_XL.gguf
gemma-4-31B-it-qat-GGUF MTP/mtp-gemma-4-31B-it-F16.gguf gemma-4-31B-it-qat-UD-Q4_K_XL.gguf

The drafter stem gemma-4-12b-it prefixes the weight and the next character is -, which is the same delimiter-bounded prefix being flagged. Requiring exact family identity drops the drafter for all of those repos.

I ran the predicate on both directions. The dangerous one is already blocked by the boundary check this PR adds: dspark-DeepSeek-V4-Flash-Lite-Q8_0.gguf against DeepSeek-V4-Flash-Q4_K_M.gguf returns False. What remains is a local folder mixing two families where the longer one is missing its own sidecar, and detect_dspark_file already prefers the closer name when both are present. Real regression against a hypothetical, so keeping the current behaviour.

Comment on lines +7457 to +7461
files = sorted(
(name for name in candidates if _is_dspark_drafter_path(name)),
key = dspark_preference_key,
)
return files[0] if files else None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Download every shard of a split DSpark sidecar

When a Hub publishes the selected DSpark precision as split GGUF files, this picker returns only the first shard, and _download_companion_gguf downloads only that exact filename. On a fresh cache the sibling shards are therefore absent when llama-server opens --model-draft, so DSpark startup fails even though the repository contains a complete sidecar. Detect the split suffix and fetch the complete shard set before returning shard 1 as the launch path.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mechanically accurate but not reachable, so leaving it. There is no split DSpark sidecar published anywhere: our two are single-file (dspark-DeepSeek-V4-Flash-0731-Q8_0.gguf at 10.90 GB and dspark/dspark-DeepSeek-V4-Flash-0731-BF16.gguf at 11.31 GB), and a Hub sweep across unsloth plus third-party DSpark repos turned up no split sidecar and no split MTP drafter either.

It is also pre-existing rather than DSpark-specific: _pick_mtp behaves the same way on main. The local path already handles splits via _drafter_split_is_complete and _local_gguf_load_path. Worth revisiting if a split sidecar is ever published.

Comment thread studio/backend/routes/inference.py Outdated
Comment on lines 5006 to 5010
drafter_attr = "gguf_dspark_file" if dspark_requested else "gguf_mtp_file"
for attr in ("gguf_mmproj_file", drafter_attr):
f = getattr(config, attr, None)
if f and Path(f).is_file():
total_bytes += Path(f).stat().st_size

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Include DSpark draft KV in the training admission estimate

When training is active and DSpark is requested, this path adds the sidecar weights but still returns only the target model's KV estimate. The launch-time fitter additionally reserves the DSpark draft KV and draft compute buffers, with the draft KV scaling with both context length and n_parallel; at large contexts or multiple slots this can exceed the generic margin and make the coexistence guard admit a chat load that OOMs or disrupts the training run it is intended to protect. Reuse the launch-time DSpark overhead calculation in this admission estimate.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The omission is real but pre-existing and deliberately compensated, so leaving it. The reasoning is already at the call site, and git show main:studio/backend/routes/inference.py puts that comment at line 5466, so it predates this PR:

MTP is deliberately NOT clamped here even though the launch clamps it to one slot. _estimate_gguf_required_gb counts the drafter file and the main KV, but not the draft KV, the duplicated target context MLA keeps, or the draft compute reserve, all of which load_model does budget. Sizing for one slot would drop the slot KV without replacing it with those, and a guard that under-sizes evicts the training run it exists to protect: the spare slots stand in for what is not modelled.

This PR only picks which drafter file to count (drafter_attr = "gguf_dspark_file" if dspark_requested else "gguf_mtp_file"), so DSpark inherits the same already-reasoned treatment as MTP. Tightening the admission model is a fair follow-up, but it is not a defect introduced here.

Comment on lines 3729 to 3735
if (
(self._speculative_type == "draft-mtp" or self._spec_fallback_reason == "runtime_error")
(
self._speculative_type in ("draft-mtp", "draft-dspark")
or self._spec_fallback_reason == "runtime_error"
)
and intent.spec_draft_n_max is not None
and intent.spec_draft_n_max != (compared_draft_n_max or 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Compare DSpark draft depth when clearing it or after fit fallback

A loaded DSpark session with an explicit draft depth is treated as an exact match when the caller clears spec_draft_n_max, because the comparison only runs when the new value is non-null. In addition, after a dspark_fit_required fallback, all draft-depth changes are ignored because neither side of the outer condition is true; lowering the depth can change the fit result, but Apply is deduplicated and never retries DSpark. Compare the requested normalized depth, including None, against the saved intent/runtime for every DSpark fallback where depth can affect a retry.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Both halves are pre-existing and asserted on main, so leaving them. The guard at llama_cpp.py:3706-3711 on main is byte-identical apart from this PR adding "draft-dspark" to the tuple:

(self._speculative_type == "draft-mtp" or self._spec_fallback_reason == "runtime_error")
and intent.spec_draft_n_max is not None

Clearing to None is the documented contract, pinned by test_already_in_target_state_matches_when_draft_n_max_unset on main: "None on the request means platform default; matches any backend." The fallback case has its own sibling test for binary_no_mtp.

For dspark_fit_required specifically the refusal is not depth-dependent at all: dspark_fit_allowed = not use_fit, so a different draft depth re-derives the same refusal and a reload would change nothing observable.

Conflict was in studio/backend/utils/models/model_config.py, in
detect_mtp_file. Both sides touched the same helpers: this branch hoists
_pairing_stem, _drafter_launch_path, the split-completeness check and the
shard-size sum to module level so the DSpark path can share them, while main
kept them as local closures and rewrote their comments.

Resolved in favour of the shared helpers, since main's changes to that
function were comment only and carry no behaviour. Kept main's tightened
wording for _smallest_first and its note that split copies collapse to shard
1, and kept this branch's note that MTP prefers Q4_0 where DSpark prefers
Q8_0.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

shimmyshimmer and others added 3 commits August 6, 2026 12:28
The ~11 GB DSpark sidecar was fetched at llama_cpp.py:8664 while the first
supports_dspark consumer sat ~480 lines later, so a binary that cannot run
draft-dspark paid for the whole download and then fell back without ever
opening the file. probe_server_capabilities is already called just above for
supports_kv_unified, so the answer is in scope and cached and the check costs
nothing.

This is the default path right now, not an edge case: the shipped
unslothai/llama.cpp prebuilt b10265-mix-89aa77b sits inside the known-broken
b10259..b10268 window, so supports_dspark is False on a standard install.

Also swaps the order of the first two DSpark fallbacks. Now that the fetch is
gated on the same answer, a gated binary leaves no sidecar, and checking the
drafter first reported "no matching dspark-*.gguf sidecar was found" and told
the user to place a file that was never the problem, while re-loading on every
Apply through the drafter_not_found dedup branch.

Adds three regression tests, all of which fail without this change.
…ct the hint

Three fixes from the latest review round.

Extras that own --spec-type return from _build_speculative_flags before
_speculative_type is set, so the --fit strip keyed only on that field never
fired for a pass-through DSpark launch and a user --fit on survived. DSpark's
layout cannot be reshaped, so that aborts the load. The strip now also reads
the accumulated spec types, which covers both the flag and the env.

The training coexistence estimate sized the drafter with a bare stat(), while
the main weight beside it already used the split-aware helper. Discovery hands
back shard 1, so a split sidecar was counted at one shard and the guard could
admit a load that evicts the training run it exists to protect.

The Speculative Decoding hint promised "no accuracy hit" unconditionally, which
DSpark does not meet: on a quantized target its greedy output can differ from a
non speculative run (ggml-org/llama.cpp#25618). Measured here on
DeepSeek-V4-Flash-0731 UD-Q4_K_XL, where the same greedy conversation produced
10570 tokens without a drafter and 14687 with one. The claim now stays with
Auto, and DSpark carries its own caveat.

Both backend fixes have regression tests that fail without them.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

@danielhanchen

Copy link
Copy Markdown
Member

Correction to my earlier reply on the --fit thread: --fit on is not incompatible with DSpark, so 4876666 was fixing the wrong thing. I checked it on the real model and reverted it in 960fabf.

Run, 4x B200, UD-Q4_K_XL target plus the published Q8_0 sidecar:

llama-server -m DeepSeek-V4-Flash-0731-UD-Q4_K_XL-00001-of-00005.gguf \
  -md dspark-DeepSeek-V4-Flash-0731-Q8_0.gguf \
  --spec-type draft-dspark --spec-draft-n-max 3 --fit on -fa on -c 8192 --jinja

It loads and the drafter engages:

E llama_init_from_model: failed to initialize the context: dflash requires ctx_other to be set (this warning is normal during memory fitting)
W srv    load_model: [spec] failed to measure draft model memory: failed to create llama_context from model
I common_speculative_impl_draft_dflash: adding speculative implementation 'draft-dspark'
I common_speculative_impl_draft_dflash: - n_max=3, n_min=0, p_min=0.00
I srv  llama_server: model loaded

and generates normally: 196 tokens at 105.7 tok/s with draft_n=177, draft_n_accepted=137.

The mechanism is narrower than "the layout cannot be reshaped". DSpark ships no token_embd/output and borrows the target's, so it needs ctx_other. The fit step tries to build a standalone draft context purely to measure its memory (server-context.cpp:1160-1193), that throws (llama-context.cpp:153-160, upstream labels the message "normal during memory fitting"), llama-server catches it and continues. The real context is created later with ctx_other set and succeeds. The only consequence is that the sidecar's ~11 GB is missing from fit_params_target, so it is not reserved.

What changed in 960fabf:

  • Dropped the dspark_fit_required fallback. This was the significant one: it disabled DSpark outright whenever the placement estimator fell back to auto-fit, which is the default Manual + Auto path, so the feature silently did not engage for the users most likely to need fitting.
  • Dropped the forced --fit off on pass-through extras, and strip_fit from strip_shadowing_flags with it. A caller's --fit is now left alone.
  • Removed the DSpark exclusion from the --fit off -> --fit on startup-crash retry. That retry is exactly the recovery a DSpark load wants when the unreserved sidecar pushes it over.
  • Replaced the hard refusal with an info log naming the unreserved size and pointing at Manual GPU Layers, plus the dspark_fit_required string in the API schema and the chat settings sheet.

Tests updated in place rather than deleted: test_dspark_composed_argv_respects_placement_fit_decision now asserts the drafter is emitted under both fit values, test_build_speculative_flags_dspark_engages_under_auto_fit replaces the declines-when-fit-required case, and test_dspark_keeps_a_user_fit_flag pins that a caller's --fit on survives. 1055 backend tests pass.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 6, 2026
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@danielhanchen
danielhanchen merged commit 2e593b3 into unslothai:main Aug 6, 2026
34 of 57 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44dfe67e54

ℹ️ 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".

# MTP fetch above, so this costs one listing and no download.
if (
not dspark_draft_path
and _spec_canon in ("auto", "dspark")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep DSpark out of Auto mode

When speculative_type is omitted, _spec_canon resolves to auto, so this branch automatically downloads the roughly 11 GB DSpark sidecar and the promotion block below launches it. That violates the advertised opt-in behavior and makes an ordinary default load of a repository such as DeepSeek V4 Flash consume substantial disk/VRAM and potentially change greedy output or reduce concurrent throughput without the user selecting DSpark; only the explicit dspark mode should trigger this fetch and promotion.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Disagreeing on this one: Auto reaching DSpark is deliberate, and it is the behaviour this repo now wants. The opt-in wording the comment cites was the old design, and it has been updated along with the change.

The trade-offs listed are real, so they are disclosed rather than hidden. The picker hint now reads:

Faster generation. Auto picks the best strategy for the model and platform: DSpark when the model ships a drafter sidecar, otherwise MTP / ngram. Pick a strategy to force it, or Off to disable. DSpark downloads a sidecar of about 11 GB and trades VRAM for speed; on quantized targets its greedy output can differ from a non speculative run. MTP and ngram do not change output.

On the merits, measured on this exact target (unsloth/DeepSeek-V4-Flash-0731-GGUF UD-Q4_K_XL, 7-turn conversation, greedy, decode rate as sum(predicted_n)/sum(predicted_ms)):

4x B200 1x B200
target only 61.0 tok/s 62.6 tok/s
DSpark n=3 112.4 tok/s (1.84x) 119.7 tok/s (1.91x)

Without the promotion this architecture gets no drafter at all from Auto, so the alternative to 1.84x is 1.0x. Full write-up, plots and CSVs: https://huggingface.co/datasets/danielhanchen/minimax-2.7-analysis/tree/main/dspark_deepseek_v4_flash_0731_2026_08_06

The narrower points are handled:

  • Disk / VRAM. The fetch is capability-gated, so a binary that cannot run draft-dspark never opens it, and the training-coexistence guard charges the sidecar under Auto too, so a load is refused rather than silently evicting a training run.
  • Greedy output. Documented in the hint above and on the model card. It reproduces Eval bug: Speculative decoding (draft-mtp / draft-dspark): greedy output diverges from vanilla on quantized targets ggml-org/llama.cpp#25618 and is not specific to this integration.
  • Concurrency. Measured at 4-way on this hardware it still helps, 150.4 against 137.0 tok/s aggregate at n=3. Reports of a regression exist on other machines, which is why the writeup calls concurrent throughput hardware dependent.

Anyone who wants the old behaviour has two supported ways to get it: pick a mode explicitly, or set it to Off.

)
# Distinguishes a repo that ships no sidecar (the permanent state for
# every repo but one) from a fetch that failed and could yet succeed.
self._dspark_sidecar_absent = outcome.get("listed") is False

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Retry DSpark after an inconclusive repository listing

When the main model is cached but the Hub is temporarily offline or all list_repo_files attempts fail, _download_companion_gguf leaves target unset and records outcome["listed"] = False, so this assignment incorrectly marks the repository as definitively lacking a sidecar. After the resulting drafter_not_found fallback, _runtime_matches_intent uses this flag to deduplicate every subsequent Apply, meaning DSpark is never fetched when connectivity returns unless the user explicitly unloads first; track whether a listing actually completed separately from whether it contained a matching file.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct, and my own comment on that line claimed the opposite ("left unset ... where the answer is genuinely unknown"). It is only unset on the early returns; a listing that exhausts its three attempts falls through with target still None, so an unreachable Hub was recorded as a definitive absence and the reuse check then deduped every later Apply.

Fixed in #8037 (4de1647) by tracking the two facts separately:

target: Optional[str] = None
# Whether the question was actually answered. A listing that never
# completed (offline, transient Hub failure) leaves target None for a
# reason that says nothing about the repo's contents.
listing_answered = False
...
        target = pick(list_repo_files(hf_repo, token = hf_token))
        listing_answered = True

and the record is written only when listing_answered or target is not None, so a cache hit still counts as an answer while a failed listing leaves the flag absent and the retry alive.

test_an_unreachable_hub_is_not_recorded_as_a_missing_sidecar drives list_repo_files into ConnectionError with no cache snapshots and asserts "listed" not in outcome. It fails against the merged condition.

Comment on lines +268 to +269
speculative_type: payload.speculative_type ?? null,
spec_draft_n_max: payload.spec_draft_n_max ?? null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Pass speculative settings through background validation

These fields only help when the caller includes them, but the background canAutoLoad payload type and both calls in chat-adapter.ts omit speculative_type and spec_draft_n_max even though the subsequent loadModel call sends the resolved per-model/global values. While training is active, a saved Off or Ngram choice is therefore preflighted as Auto and may be rejected after charging an unused DSpark sidecar, while a forced MTP configuration that Auto would suppress can be admitted using too small an estimate; pass the same effective speculative settings to the validation request as to the load.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Confirmed. I added the fields to validateModel but not to canAutoLoad, whose payload type carried a comment explicitly saying the spec knobs are deliberately omitted. That was true while the estimate was mode-independent; making it mode-dependent invalidated it.

Fixed in #8037 (4de1647). The type gains speculative_type / spec_draft_n_max, the stale sentence is gone, and both call sites send exactly what their loadModel sends: the remembered-config sweep passes effectiveSpeculativeType / effectiveSpecDraftNMax (already computed a few lines above for the load), and the default-model path passes specSettings.

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.

3 participants