Fix FastSentenceTransformer Qwen embedding preprocessing - #6939
Conversation
|
@codex review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 885ebabaab
ℹ️ 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 more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
|
Reproduced this on the reported stack (transformers 5.5.0, sentence-transformers 5.5.0, torch 2.9.1+cu128, Root cause: in sentence-transformers 5.x, building the module with Tokenization of
Parity vs a stock
Also confirmed both paths load I pushed a small commit adding an inline note so the |
|
Pushed a follow-up to make this forwards and backwards compatible and to add regression coverage so it cannot silently come back. Hardening The previous gate required
It now prefers Tests
To exercise the parity test in CI, add a step that sets Verification (transformers 5.5.0, sentence-transformers 5.5.0, torch 2.9.1+cu128,
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0232d5ed36
ℹ️ 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".
| } | ||
| if not accepts_var_kw: | ||
| load_kwargs = {k: v for k, v in load_kwargs.items() if k in load_params} | ||
| transformer_module = Transformer.load(model_name, **load_kwargs) |
There was a problem hiding this comment.
Keep legacy Transformer.load off remote SentenceTransformer ids
When users have sentence-transformers 3.x/4.x installed and load a Hub SentenceTransformer model with modules.json, this branch still reaches Transformer.load(model_name, ...) after filtering the unsupported kwargs down for the legacy single-argument load(input_path) signature. That legacy loader reads sentence_bert_config.json from the supplied local directory, so a remote id like Qwen/Qwen3-Embedding-0.6B is treated as a filesystem path and fails before the previous Transformer(...) constructor fallback can run; pyproject.toml still leaves sentence-transformers unbounded. Fresh evidence in this revision is that the kwargs filter avoids the earlier TypeError but still invokes the single-argument loader for remote IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this is fixed in 291412d: the Hub-capable gate (accepts token/cache_folder/revision or **kwargs) means legacy sentence-transformers 3.x/4.x (single-arg Transformer.load(input_path)) no longer reaches the loader at all and falls back to the Transformer(...) constructor, so a remote id is never passed to the local-only legacy loader. That revision landed after the commit reviewed here (0232d5e).
…without Hub-capable load
|
@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: 291412da6b
ℹ️ 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".
| unsupported = [ | ||
| k | ||
| for k in ("token", "cache_folder", "revision", "trust_remote_code") | ||
| if not (accepts_var_kw or k in params) |
There was a problem hiding this comment.
Mirror the legacy Transformer.load fallback in the test
When the installed sentence-transformers is 3.x/4.x, Transformer.load has the legacy load(input_path) signature, which the production path deliberately treats as non-hub-capable and falls back to Transformer(...). Because sentence-transformers is still unpinned, this always-on test will fail in those supported environments before the fallback can be exercised; either skip this assertion for legacy signatures or mirror the hub_capable check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 629316f. The tripwire now mirrors the production hub_capable gate: it skips on a legacy load(input_path) signature (where the code falls back to Transformer(...)) and only asserts the kwargs on a Hub-capable load. Confirmed it passes on ST 5.5.0 and skips for the legacy single-arg signature.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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 |
|
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". |
…sformer requires CUDA
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1689577da
ℹ️ 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".
| } | ||
| if not accepts_var_kw: | ||
| load_kwargs = {k: v for k, v in load_kwargs.items() if k in load_params} | ||
| transformer_module = Transformer.load(model_name, **load_kwargs) |
There was a problem hiding this comment.
Honor transformer module paths when loading configs
When modules.json stores the Transformer module under a non-empty path such as 0_Transformer, _load_modules has that path but this call discards it and loads Transformer from the repo root. Transformer.load therefore looks for sentence_bert_config.json/modality_config at the root instead of in the module directory, so chat-template embedding checkpoints saved in that layout still fall back to inferred message preprocessing and lose the parity this change is meant to restore. Pass the module path through as the subfolder/load path when calling Transformer.load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in def20fb. The Transformer module's saved path from modules.json is now threaded into Transformer.load as subfolder, so sentence_bert_config.json and modality_config resolve from the module directory (for example 0_Transformer) like stock ST. A root path stays a no-op, so single-module checkpoints such as Qwen3-Embedding are unchanged (verified: identical tokenization, min cosine > 0.99).
| } | ||
| if not accepts_var_kw: | ||
| load_kwargs = {k: v for k, v in load_kwargs.items() if k in load_params} | ||
| transformer_module = Transformer.load(model_name, **load_kwargs) |
There was a problem hiding this comment.
Preserve lowercasing on the load path
For saved ST models on sentence-transformers >=5.4, this call sends do_lower_case through **transformer_kwargs, but Transformer.load only forwards its named load/model/processor/config kwargs and drops arbitrary constructor kwargs. The later transformer_module.do_lower_case = ... assignment does not update the tokenizer normalizer that ST 5.4+ relies on, so models that require the forced lowercasing path will tokenize differently than the old Transformer(..., do_lower_case=True) path. Ensure do_lower_case is applied after loading, not just stored as an attribute.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
On the load path do_lower_case is not lost. Transformer.load reads it from the saved sentence_bert_config.json (it is a Transformer.config_keys entry) and init applies it to the tokenizer backend normalizer on ST 5.4+ (transformer.py lines 679-696), so lowercasing is preserved and matches stock ST; the value passed through **kwargs there is redundant. The constructor fallback still receives do_lower_case and honors it. Decoder embedders like Qwen3-Embedding do not lowercase and use fast tokenizers with no basic_tokenizer, so no properly saved checkpoint tokenizes differently on this path.
modules.json records a path for the Transformer module (root for decoder embedders like Qwen3-Embedding, 0_Transformer for the classic layout). Pooling/Normalize already load from their saved path; thread the same path into Transformer.load as subfolder so config and tokenizer resolve like stock ST. stays a no-op, so single-module models are unchanged.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
fp16 overflows to NaN on bf16-native embedders such as EmbeddingGemma (Gemma3), producing a false parity failure. Prefer bf16 when the GPU supports it so the tripwire can guard the full documented embedding matrix (Qwen3-Embedding, EmbeddingGemma, BGE-M3, all-MiniLM, GTE-ModernBERT), not just fp16-safe models.
|
@codex review |
|
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". |
Summary
Fixes
FastSentenceTransformermodule construction for saved SentenceTransformers models such asQwen/Qwen3-Embedding-0.6B. The loader now preserves the saved text-only Transformer module config instead ofletting SentenceTransformers infer Qwen chat/message preprocessing.
Motivation
FastSentenceTransformer.from_pretrained("Qwen/Qwen3-Embedding-0.6B")was producing embeddings that diverged from a plainSentenceTransformerload. With SentenceTransformers 5.x, constructing the modulethrough
Transformer(...)can infer Qwen's chat template path, so plain embedding strings get encoded as chat messages.Fixes #6881.
Changes
unsloth/models/sentence_transformer.py, useTransformer.load(...)when the model hasmodules.json.token,cache_dir, andrevisioninto_create_transformer_module()so the module loader uses the same Hub/cache context as_load_modules().Transformer(...)fallback for non-SentenceTransformers models that do not havemodules.json.How to test
Run a parity check for Qwen3 embeddings:
Expected result: cosine values should be near 1.0. In the Colab check for this fix, mean_cos was 0.99999699 and max_abs_diff was 0.000305.