[staging CI] unslothai/unsloth#7968 - #777
Closed
danielhanchen wants to merge 16 commits into
Closed
danielhanchen wants to merge 16 commits into
danielhanchen wants to merge 16 commits into
Conversation
for more information, see https://pre-commit.ci
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.
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.
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.
for more information, see https://pre-commit.ci
…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.
…claim them on delete
for more information, see https://pre-commit.ci
…cker contract marker
for more information, see https://pre-commit.ci
…reload loops it exposed
for more information, see https://pre-commit.ci
![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: f331b8728e
ℹ️ 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
+7485
to
+7489
| files = sorted( | ||
| (name for name in candidates if _is_dspark_drafter_path(name)), | ||
| key = dspark_preference_key, | ||
| ) | ||
| return files[0] if files else None |
There was a problem hiding this comment.
When the preferred remote DSpark sidecar is split (for example, dspark-model-Q8_0-00001-of-00002.gguf), this picker returns only the first filename, and _download_companion_gguf subsequently calls hf_hub_download_with_xet_fallback for that single file. llama-server discovers sibling shards by filename, so a fresh remote load receives an incomplete drafter and falls back without DSpark. Group split candidates and download the complete selected shard set rather than one entry.
Useful? React with 👍 / 👎.
Comment on lines
+4976
to
+4979
| if dspark_candidates: | ||
| # Same preference order the download uses, so the budget sizes the | ||
| # file the launch will actually fetch. | ||
| total += min(dspark_candidates, key = lambda c: dspark_preference_key(c[0]))[1] |
There was a problem hiding this comment.
For a remote DSpark sidecar published as multiple GGUF shards, this adds only the size of the minimum-ranked individual shard. A complete split set already present in the cache is loaded in full by llama-server, so validation can underestimate VRAM by the remaining shards and admit a chat load that evicts or OOMs the training job this guard protects. Rank sidecar sets by precision and sum every shard in the chosen set.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Disposable CI run for unslothai#7968. Do not merge; closed after CI.