unsloth: pin the two AMD fixes so the ROCm nightlies carry them - #160
Merged
Conversation
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. |
ROCm APU user with no opt-in. #157 stops the unified-memory corruption for users who have GGML_CUDA_ENABLE_UNIFIED_MEMORY set. both would apply the same hunks twice and fail the resolve, so only one can be listed. Both entries come out once a base tag carries the upstream merge.
danielhanchen
force-pushed
the
unsloth/pin-amd-fixes
branch
from
August 31, 2026 07:40
6dbe421 to
a8033f5
Compare
danielhanchen
added a commit
that referenced
this pull request
Aug 31, 2026
…161) * unsloth: repin ggml-org#25731 to the commit that merges onto b10705 edee0e1 stopped applying to the current base tag, which failed the pin preflight. Because the preflight stops at the first conflict, it also meant the two AMD fixes pinned by #160 were never dry-run merged at all. 44eb88e is the head after the upstream conflicts were resolved; the PR is MERGEABLE upstream again as of 2026-08-31T08:06Z. * unsloth: repoint the AMD pins at commits based on an upstream ancestor #157 and #158 were branched from fork master, which is right for a small PR diff and wrong for a pin. Fork master has diverged from upstream (it deletes a dozen upstream workflow files), so merging those commits onto the base tag dragged the whole divergence in and conflicted modify/delete on twelve .github/workflows files. That is what exited the preflight with no message after 'ok #137'. The pins that work are branched from an upstream commit that is an ancestor of both master and the base tag, so they carry only their own delta. Both branches are now rebased onto 11cd988 on that pattern, and verified locally to merge onto b10705 for a net +35/-3 across 4 files, which is the two fixes and nothing else. --------- Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com>
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
1 participant
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.
Overview
Pins the two AMD fixes so the ROCm and Vulkan nightlies actually carry them. Two lines in
scripts/unsloth/pr-set.json.c7d87229)0ed235ea/ ggml-org#25057)GGML_CUDA_ENABLE_UNIFIED_MEMORYsetThe nightly tree is the upstream tag plus these pins, so merging either PR into fork master ships nothing on its own. That is the point of this change, and it is the trap that made #145 useless.
This supersedes #147
#147 pins ggml-org#25863; #158 is that same change carried in this fork. Pinning both would apply the same hunks twice and fail the resolve step, so only one can be listed. This uses ours, which keeps the code reviewable here. #147 should be closed if this merges, or this entry dropped if you would rather track upstream directly. Either works, but not both.
Why these two
#158.
c7d87229(ggml-org#24233) restoredprop.integratedon HIP builds, where CUDA keeps it off with the commentTemporarily disabled due to issues with corrupted output. The scheduler may then place a compute input in pinned host memory and an H2D write races a running graph. Two reporters bisected to that commit independently, and lemonade-sdk/lemonade#3160 is the same failure on our own Qwen3.8-27B UD-Q4_K_XL: coherent at first, then word salad under 8 concurrent requests, then////////even for sequential requests, recovering on reload. Same harness scores 78-82% against cloud endpoints and ~17% here. Measured at PPL 63.67 against a Vulkan reference of 64.10, where the base gives 872,006.#157. A release bisect over prebuilt ROCm tarballs put the corruption on
0ed235ea, b9826 clean and b9827 broken, direct parent and child. Verified on gfx1151 with both cells in one job on one host: the fixed build emitted identical tokens with the variable unset and set, while the stock build reproduced' Paris.,,, or or or is is is is,,,,,'.Verification
The preflight probes the merges; what I want to see is that plus green gfx1150 and gfx1151 ROCm legs.
Neither fix is measured for throughput, and I would rather say so than let the correctness results stand in for a benchmark. #158 changes buffer placement on a chip where both pools are the same DRAM. #157 makes the runtime resolve residency per pointer on a hot path; the comparable pair measured 47.7 against 48.2 tok/s, which is evidence against a large penalty rather than proof of none. Any benchmark here has to check output validity, because the corrupted build is not slower: in our own run the broken state generated at 45.0 tok/s.
Both entries come out once a base tag carries the upstream merge, per the rule in
_doc.