update the default embd. #310
Merged
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested.
- Reviewed the entire pull request up to c9cba2e
- Looked at
365lines of code in14files - Took 4 minutes and 10 seconds to review
More info
- Skipped
1files when reviewing. - Skipped posting
4additional comments because they didn't meet confidence threshold of50%.
1. r2r/core/providers/embedding.py:8:
- Assessed confidence :
33% - Comment:
The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. This is a good practice as it makes the code more flexible and easier to extend in the future. However, the PR lacks a description explaining these changes. - Reasoning:
The PR changes the default embedding model fromall-MiniLM-L6-v2tomixedbread-ai/mxbai-embed-large-v1. The changes are consistent across the documentation and configuration files. The PR also updates thesentence-transformerslibrary version and adds thetokenizerslibrary in thepyproject.tomlfile. The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. Thetrust_remote_code=Trueargument is added when initializing theSentenceTransformermodel, which allows the execution of remote code for custom models. This could potentially be a security risk if untrusted models are used. The PR lacks a description, which should ideally explain the reason for these changes and their impact.
2. r2r/embeddings/openai/base.py:24:
- Assessed confidence :
33% - Comment:
The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. This is a good practice as it makes the code more flexible and easier to extend in the future. However, the PR lacks a description explaining these changes. - Reasoning:
The PR changes the default embedding model fromall-MiniLM-L6-v2tomixedbread-ai/mxbai-embed-large-v1. The changes are consistent across the documentation and configuration files. The PR also updates thesentence-transformerslibrary version and adds thetokenizerslibrary in thepyproject.tomlfile. The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. Thetrust_remote_code=Trueargument is added when initializing theSentenceTransformermodel, which allows the execution of remote code for custom models. This could potentially be a security risk if untrusted models are used. The PR lacks a description, which should ideally explain the reason for these changes and their impact.
3. r2r/embeddings/setence_transformer/base.py:10:
- Assessed confidence :
33% - Comment:
The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. This is a good practice as it makes the code more flexible and easier to extend in the future. However, the PR lacks a description explaining these changes. - Reasoning:
The PR changes the default embedding model fromall-MiniLM-L6-v2tomixedbread-ai/mxbai-embed-large-v1. The changes are consistent across the documentation and configuration files. The PR also updates thesentence-transformerslibrary version and adds thetokenizerslibrary in thepyproject.tomlfile. The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. Thetrust_remote_code=Trueargument is added when initializing theSentenceTransformermodel, which allows the execution of remote code for custom models. This could potentially be a security risk if untrusted models are used. The PR lacks a description, which should ideally explain the reason for these changes and their impact.
4. r2r/main/factory.py:51:
- Assessed confidence :
33% - Comment:
The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. This is a good practice as it makes the code more flexible and easier to extend in the future. However, the PR lacks a description explaining these changes. - Reasoning:
The PR changes the default embedding model fromall-MiniLM-L6-v2tomixedbread-ai/mxbai-embed-large-v1. The changes are consistent across the documentation and configuration files. The PR also updates thesentence-transformerslibrary version and adds thetokenizerslibrary in thepyproject.tomlfile. The changes in theEmbeddingProviderclasses are to accommodate the new configuration format where the provider, model, and dimensions are specified in a dictionary. Thetrust_remote_code=Trueargument is added when initializing theSentenceTransformermodel, which allows the execution of remote code for custom models. This could potentially be a security risk if untrusted models are used. The PR lacks a description, which should ideally explain the reason for these changes and their impact.
Workflow ID: wflow_IhRBb4LDbAa7v97B
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
| @@ -1132,7 +1132,7 @@ def __init__( | |||
| ) | |||
|
|
|||
| self.model = model | |||
| self._model = SentenceTransformer(self.model) | |||
| self._model = SentenceTransformer(self.model, trust_remote_code=True) | |||
There was a problem hiding this comment.
The trust_remote_code=True argument is added when initializing the SentenceTransformer model, which allows the execution of remote code for custom models. This could potentially be a security risk if untrusted models are used. It would be better to make this an optional configuration that can be turned on if necessary, rather than having it on by default.
| raise ValueError( | ||
| "Must set dimensions in order to initialize SentenceTransformerEmbeddingProvider." | ||
| ) | ||
| self.encoder = SentenceTransformer(model, truncate_dim=dimension, trust_remote_code=True) |
There was a problem hiding this comment.
The trust_remote_code=True argument is added when initializing the SentenceTransformer model, which allows the execution of remote code for custom models. This could potentially be a security risk if untrusted models are used. It would be better to make this an optional configuration that can be turned on if necessary, rather than having it on by default.
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 22, 2024
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 23, 2024
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 23, 2024
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 23, 2024
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 23, 2024
* Update local_rag.mdx * Update llms.mdx (#322) * update the default embd. (#310) * Feature/add agent provider (#317) * update pipeline (#315) * Update CONTRIBUTING.md * Delete CONTRIBUTOR.md * Adding agent provider * Feature/modify get all uniq values (#325) * refine * update * format * fix * dev merge
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 23, 2024
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 23, 2024
* Update local_rag.mdx * Update llms.mdx (#322) * update the default embd. (#310) * Feature/add agent provider (#317) * update pipeline (#315) * Update CONTRIBUTING.md * Delete CONTRIBUTOR.md * Adding agent provider * Feature/modify get all uniq values (#325) * refine * update * format * fix * dev merge
emrgnt-cmplxty
added a commit
that referenced
this pull request
Apr 24, 2024
* update the default embd. (#310) * Feature/add jina reranker rebased (#312) * Add jina reranker * fix oai * Revampt client & server approach. (#316) * Revampt client & server approach. * cleanups * tweak hyde prompt * Feature/add agent provider (#317) * update pipeline (#315) * Update CONTRIBUTING.md * Delete CONTRIBUTOR.md * Adding agent provider * Feature/add agent provider rebased v2 (#319) * modify prompt provider workflow, add agent * fix run qna client * add provider abstraction * tweaks and cleans * move text splitter config locale * Feature/modify get all uniq values (#325) * refine * update * format * fix * Feature/dev merge rebased (#329) * Update local_rag.mdx * Update llms.mdx (#322) * update the default embd. (#310) * Feature/add agent provider (#317) * update pipeline (#315) * Update CONTRIBUTING.md * Delete CONTRIBUTOR.md * Adding agent provider * Feature/modify get all uniq values (#325) * refine * update * format * fix * dev merge * Feature/fix dev merge mistakes rebased (#330) * Feature/add agent provider (#317) * update pipeline (#315) * Update CONTRIBUTING.md * Delete CONTRIBUTOR.md * Adding agent provider * Feature/modify get all uniq values (#325) * refine * update * format * fix * dev merge * cleanup * Feature/dev cleanup (#331) * final pub * final pub * json clean * fix sentence transformer issue * include rerank * fix llama cpp * rebase * fix rerank * small tweaks * rollbk config * cleanup
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.
Summary:
This PR updates the default embedding model, updates library versions, and refactors the
EmbeddingProviderclasses to accept a configuration dictionary.Key points:
all-MiniLM-L6-v2tomixedbread-ai/mxbai-embed-large-v1in various files.sentence-transformerslibrary version and addedtokenizerslibrary inpyproject.toml.EmbeddingProviderclasses to accept a configuration dictionary.SentenceTransformerEmbeddingProviderto initializeSentenceTransformerwithtrust_remote_code=True.get_embeddings_providerfunction inE2EPipelineFactoryto pass entire configuration dictionary toEmbeddingProviderclasses.Generated with ❤️ by ellipsis.dev