[staging CI] unslothai/unsloth#4222 - #134
Closed
danielhanchen wants to merge 22 commits into
Closed
danielhanchen wants to merge 22 commits into
danielhanchen wants to merge 22 commits into
Conversation
- Fix #3544: Add revision parameter to AutoConfig, AutoModelForCausalLM, AutoModelForSequenceClassification, and load_correct_tokenizer calls in FastLlamaModel.from_pretrained. This enables loading specific model revisions/branches from HuggingFace Hub. - Fix #3667: Escape single quotes in system messages before substituting into Jinja2 templates. This prevents TemplateSyntaxError when system messages contain apostrophes (e.g., "user's" in Vicuna templates). Signed-off-by: majiayu000 <1835304752@qq.com> (cherry picked from commit b0a6e4154b1bca9ed9bc06bdd1a83da1007dd6bf)
- Add revision to load_vllm_kwargs in llama.py to fix config/weights mismatch - Add revision to PEFT AutoConfig calls in loader.py (FastLanguageModel & FastModel) Addresses reviewer feedback from @chatgpt-codex-connector and @Datta0 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> (cherry picked from commit 14f89e453173f3279c885ed37d5f2a9cf10793df)
Propagate revision parameter to all from_pretrained calls in vision.py to ensure consistent version pinning for vision models. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> (cherry picked from commit c5aa4ec92783345eaa0286bd876386cb2767d990)
The branch has drifted nine months. Resolving the merge to main's content
everywhere, because every hunk it carried is now either landed, superseded
or actively harmful:
- chat_templates.py escaping: superseded. main no longer uses re.sub, and
_escape_jinja_literal already covers backslash, both quotes and CR.
- loader.py AutoConfig(revision = ...): already on main.
- vision.py adding revision to the FastBaseModel.from_pretrained signature:
harmful. revision arrives via **kwargs there, and binding it as a named
parameter drops it from the weight load and from kwargs.get("revision").
- llama.py load_vllm(revision = ...): raises TypeError. load_vllm has no
revision parameter and load_vllm_kwargs is not filtered.
The revision fix itself follows in the next commit.
FastLlamaModel.from_pretrained took a `revision` argument and never read it, so the config, the weights and the tokenizer all came from the repo's default branch while the caller believed they had pinned a ref. Reported in #3544 by someone versioning their fine-tunes with branches, which makes it a silently wrong base checkpoint rather than an error. Forward it in llama.py (both AutoConfig loads, the three model loads, the tokenizer, the prefetch warm and the fp8 scale restore), plumb it through load_correct_tokenizer, and read it from kwargs in vision.py for the four AutoConfig, two processor and two tokenizer loads plus the VLM processor fallback. vision.py must not bind it as a named parameter: the weight load there forwards **kwargs, so binding it would drop it from that load. model_name is not always the repo the caller named. get_model_name can swap in a pre-quantized mirror, _offline_quantize_to_fp8 an fp8 temp dir, ModelScope a local snapshot, and fast_inference_setup a -bnb-4bit variant, and use_exact_model_name only gates the first of those. A ref from the original repo does not exist on the substitute, so _revision_for_resolved_repo drops it with a warning naming both repos when the resolution changed the name. The adapter load keeps the caller's revision, since that one really is for old_model_name. Supersedes the earlier attempt on this branch, whose chat-template hunk is handled by #7731 and #7746, whose vision.py signature change caused the drop described above, and whose load_vllm(revision = ...) raised TypeError because load_vllm has no such parameter. Fixes #3544
for more information, see https://pre-commit.ci
Four fixes from review: - The gate ran after the AutoConfig and PeftConfig probes, which already used the raw revision against the resolved name, so a pinned load_in_4bit load failed against the mirror instead of warning. Gate right after the resolution block and point both probes at the gated value, then re-gate before dispatch for the later fast_inference_setup remap. Feeding the second call the first result keeps the warning to one. - On a PEFT load model_name is necessarily the base model, so the late gate warned "Ignoring revision" for every versioned adapter and told the caller to pass use_exact_model_name, which cannot stop an adapter resolving its base. Skip the late gate for PEFT; PeftModel.from_pretrained already loads the adapter with the caller's revision. - load_vllm takes no revision, so vLLM fetches the default branch. Pinning only the config and the tokenizer put two refs in one model, which is worse than the old behaviour of ignoring the revision outright. Drop the pin with a warning before the config load whenever vLLM owns the weights. - _hub_repo_or_local_path resolved a cached snapshot without the revision, so an offline or local_files_only tokenizer load silently got the default ref: a revision handed to from_pretrained cannot re-point a local directory. Thread it into _resolve_hub_repo_local_dir and both call sites. Five new tests, one per fix, all failing before it.
for more information, see https://pre-commit.ci
The vLLM guard sat at the end of the same block that turns fast_inference off when vLLM is missing or the GPU is older than sm70. In that case the load falls through in-process and can honour the revision, but the guard dropped it anyway. Re-check fast_inference in the condition.
Three more from review: - A num_labels load goes through AutoModelForSequenceClassification in-process no matter what fast_inference says, so the vLLM guard was discarding a revision the load could have used. Condition it on the same `fast_inference and num_labels is None` predicate the prefetch warm already uses. - use_exact_model_name only gates the mapper substitution. The ModelScope download, the ALLOW_PREQUANTIZED_MODELS strip and fast_inference_setup ignore it, so the warning was sending callers round the same loop. Record whether the mapper is what moved the name and only offer the remedy then. - The tokenizer does not always come from the base model's repo. Loading a PEFT repo with an explicit tokenizer_name pointing at the adapter dropped the pin for the tokenizer while PeftModel loaded the adapter from the requested ref, mixing two refs. _revision_for_tokenizer_repo now resolves it where the repos are known and both dispatches carry it, replacing the tokenizer_name == model_name guess in llama.py and vision.py. vision.py pops it from kwargs, since the weight load forwards **kwargs and transformers has no such argument. Seven new tests, all failing before this.
for more information, see https://pre-commit.ci
Three more from review, all fallout from splitting tokenizer_revision out: - Skipping the late gate for PEFT leaves base_revision naming the adapter, and a remote PEFT load without an explicit tokenizer_name reads its tokenizer from the base repo, so that ref was handed to the wrong repository. Both dispatches now derive one model_revision and pass it to the base load and to the tokenizer resolution alike, so the base tokenizer can only ever get the base model's ref. - FastLlamaModel is exported, and the architecture wrappers forward `revision` through **kwargs without the new internal tokenizer_revision, so a direct call pinned the config and weights while the tokenizer read the default branch. Fall back to `revision` when the tokenizer repo is the model repo, before the warm so it does not fetch the wrong ref either. - The vLLM guard cleared only the model pin, leaving vLLM on the default branch with the tokenizer still on the requested ref. Clear both, in llama.py and in the parallel FastBaseModel block. Seven new tests.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
for more information, see https://pre-commit.ci
Four ways a pin could still land on the wrong ref: - A plain load that names its own repo as tokenizer_name kept the caller's revision even after a remap had already dropped it off the config and weights, so mirror weights paired with a pinned tokenizer. Only a PEFT adapter is a genuinely separate repo, so only it keeps that ref now. - FastModel probes the config before dispatching and FastBaseModel skips its own load while that config is set, so the vLLM path received a config read at the pinned ref alongside the default-branch weights vLLM fetches. The probed config is now withheld there; a caller's own config still goes down. - The get_auto_processor fallback under AutoProcessor ran unpinned. - _offline_quantize_to_fp8 read the default branch and cached under a name that ignored the revision, so load_in_fp8 with a revision quantized the wrong ref and could reuse another ref's artifact.
danielhanchen
force-pushed
the
pr-4222-ci
branch
from
August 2, 2026 16:43
650ee6d to
d2cfd7e
Compare
for more information, see https://pre-commit.ci
FastModel withheld the probed config from FastBaseModel on the vLLM path, but model_types, auto_model and the text-only decision had already been derived from it, so default-branch weights could load with pinned-ref dispatch. The drop now happens before the probe instead, using the same predicate FastBaseModel does, which makes that guard a no-op on this path and lets the config go down untouched again. FastLanguageModel keeps its drop inside llama.py: that one also turns fast_inference off on pre-Volta GPUs and for a num_labels load, and the loader cannot see either without duplicating the device checks, so gating early there would discard a pin llama.py would have honoured. The fp8 cache name sanitized the ref by replacing every unsafe character with the same one, so release/v1 and release.v1 shared a directory and the second load reused the first ref's artifact. A digest of the raw ref now rides along with the readable form.
danielhanchen
force-pushed
the
pr-4222-ci
branch
from
August 2, 2026 17:14
d2cfd7e to
f4fe086
Compare
for more information, see https://pre-commit.ci
…he saved ref FastLanguageModel still probed the config at the pinned ref while llama.py dropped that same ref for its vLLM load, so model_types could pick the architecture class off one ref and load weights from another. It now drops the pin before the probe like FastModel does, through _vllm_will_load_weights in llama.py, which llama.py itself now calls: the language path also falls back in-process on pre-Volta GPUs and for a num_labels load, so the predicate has to live where those checks are rather than be guessed at by the loader. That drop runs before is_peft is known, and it was zeroing the ref the PeftConfig probe reads. An adapter is loaded in-process by peft, so it keeps the ref: adapter_revision holds the value from before the vLLM drop. Pinning the tokenizer also desynced the save path, which restores tokenizer.model from tokenizer.name_or_path and so had no idea which branch to read. The loaded ref is now stamped on the tokenizer the way local_files_only and cache_dir already are, and the sentencepiece probe, its memo key and the restore all use it.
danielhanchen
force-pushed
the
pr-4222-ci
branch
from
August 2, 2026 17:49
f4fe086 to
85a90eb
Compare
for more information, see https://pre-commit.ci
FastBaseModel builds its processor without going through load_correct_tokenizer, so the stamp save.py reads was only being applied on the text path and a pinned FastVisionModel load still restored tokenizer.model from the default branch. Stamped at the return rather than at each of the processor branches, so the AutoTokenizer fallback that runs when patch_tokenizer raises cannot lose it either.
danielhanchen
force-pushed
the
pr-4222-ci
branch
from
August 2, 2026 18:11
85a90eb to
c29a941
Compare
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
2 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/unsloth#4222. Do not merge; closed after CI.