-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add embeddings & reranking via Sentence Transformers #2381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
LightRAG/lightrag/api/config.py
Lines 242 to 246 in 23e7ffb
| choices=[ | |
| "lollms", | |
| "ollama", | |
| "openai", | |
| "azure_openai", |
Even though create_app adds logic for a sentence_transformers embedding binding, the CLI still restricts --embedding-binding to the list shown here, so passing --embedding-binding sentence_transformers results in an argparse "invalid choice" error. Because of this omission the new embedding provider cannot be enabled from the server’s documented configuration interface.
LightRAG/lightrag/api/config.py
Lines 254 to 257 in 23e7ffb
| "--rerank-binding", | |
| type=str, | |
| default=get_env_value("RERANK_BINDING", DEFAULT_RERANK_BINDING), | |
| choices=["null", "cohere", "jina", "aliyun"], |
Similarly, the argparse definition for --rerank-binding still lists only null|cohere|jina|aliyun even though the server now maps a sentence_transformers reranker (lightrag/api/lightrag_server.py lines 943‑998). Attempting to run the API with --rerank-binding sentence_transformers is rejected before the app starts, making the newly added reranker unreachable via the supported CLI/environment configuration.
ℹ️ 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".
| sentence_transformers_embed.func | ||
| if isinstance(sentence_transformers_embed, EmbeddingFunc) | ||
| else sentence_transformers_embed | ||
| ) | ||
| return await actual_func(texts, embedding_dim=embedding_dim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wire sentence_transformers binding with required model
Selecting the new sentence_transformers embedding binding crashes immediately. The server block calls actual_func(texts, embedding_dim=embedding_dim) without ever constructing or passing a SentenceTransformer instance, yet sentence_transformers_embed (lightrag/llm/sentence_transformers.py lines 13‑32) requires a model positional argument and does not accept embedding_dim. As soon as the binding is chosen the API raises TypeError (missing model / unexpected embedding_dim), so the embedding provider cannot be used at all.
Useful? React with 👍 / 👎.
Description
Related Issues
None
Changes Made
sentence_transformers_embedwith autoembedding_dimandmax_token_size, allowing you to use any model from https://huggingface.co/models?library=sentence-transformerssentence_transformers_rerank, allowing you to use any model from https://huggingface.co/models?pipeline_tag=text-ranking&library=sentence-transformerslightrag_hf_demo.pyChecklist
Additional Notes
I wasn't able to get
lightrag_hf_demo.pyto run nicely, so I couldn't extensively test the new demolightrag_sentence_transformers_demo.pyeither, but the embedding and reranking components do work on their own.