Repository navigation
Conversation
Assisted-by: Claude Opus 5.5
Assisted-by: Claude Opus 5.5
Contributor
Author
|
Closing for now to stay within the one-open-PR limit for new contributors. I'll reopen this once #29627 is done. |
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
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.
My name is Josh. I'm a software developer. I've been working a lot with AI tools, on an iOS app that lets people run models locally on their device. I really care about the security and sovereignty of individuals when they're using LLMs: if they want to interact with them without their interactions leaving their device, they should be able to. I think that's really cool.
The app I'm building is 100% free, and there are no in-app purchases. Long story short, I'm absolutely using AI to create everything that I'm doing at this point. While working on it, I seem to have come across something that would be a real benefit to get into the core infrastructure of llama.cpp, in that it would enable more models to be used better. If this is something that's not welcome, I will totally understand and take that message loud and clear: I'm not well-equipped enough to contribute, and that is actually a fair statement.
That being said, I've been working on this application for over a year now. I'm a huge fan of Georgi Gerganov, if anything as an idea: putting things out in the world that are good for people to use. This is just me trying to be a part of that. I don't mean to step on any toes here. Again, if it's not useful, I will not do this anymore. I'm just working with these tools so much that the agent I'm working with and I came across a few small changes that look like they could actually be of great benefit. For that reason, I'm submitting them, and I acknowledge 100% that this is on the line of a potential ban for contributing. Again, I will take the message loud and clear and never do this again. I just want to be helpful, and this is the way I've found that I can potentially do that.
Thank you for reading my PRs, and just know that there is absolutely a human in the loop, even though I am certainly using my agent to put this PR through.
-josh - this is a continuation off of another PR right before this same one as they're related...
The technical sections below were drafted by my agent from our testing, and I've reviewed them.
Overview
Builds on #29627: only the second commit is new here.
sentence-transformers CrossEncoder models saved with sentence-transformers v5, such as the
cross-encoder/ettin-reranker-*-v1family, keep the scoring head outside the transformer, as separate modules listed inmodules.json:Pooling→Dense(GELU) →LayerNorm→Dense. The converter only reads the mainmodel.safetensors, so these models convert without a head and their rank output isn't a relevance score.This PR:
config_sentence_transformers.jsonandmodules.json, and maps the head modules onto the existing ModernBERT rank head:Dense→cls,LayerNorm→cls_norm, the lastDense→cls_out1_Pooling, using the key added in model : support classifier_pooling for ModernBERT rerankers #29627cls_norm_btensor so the LayerNorm bias is applied. Folding the bias into the classifier bias would also work numerically, but only while the classifier directly follows the norm, and it would change the stored weights. Dropping the bias shifts the Ettin 68m scores by about 0.13.Densewith a bias, other module orders, a module withoutmodel.safetensors) with a clear error instead of writing a broken fileAdditional information
Scores for 6 query/passage pairs (f16,
llama-server --reranking, CPU) compared withCrossEncoder(...).predictin fp32 (sentence-transformers 5.4.1, transformers 5.7.0):cross-encoder/ettin-reranker-17m-v1cross-encoder/ettin-reranker-32m-v1cross-encoder/ettin-reranker-68m-v1cross-encoder/ettin-reranker-150m-v1cross-encoder/ettin-reranker-400m-v1On master the same models load, but without a head their outputs don't track the reference at all. Q8_0 of the 68m model stays within 0.22 and keeps the same ranking.
gte-reranker-modernbert-baseandgranite-embedding-reranker-english-r2convert to byte-identical GGUFs with and without this PR.These checkpoints'
tokenizer_config.jsonnames the transformers v5TokenizersBackendclass, so converting them needs transformers 5.x. The rope settings they keep inrope_parametersare already handled by the converter.Checks: the same as #29627 (warning-free build, flake8, editorconfig-checker, gguf-py tests,
test-llama-archs).Requirements