Add New Integration for DeepInfra Embedding Model #1
Conversation
ovuruska
changed the title
|
I don't see the code that does the auto-batching (the one you added for lang-chain). Other than that it looks like a standalone project, which is probably how this is organized. Other than that looks good. |
|
|
||
| async def _aget_query_embedding(self, query: str) -> List[float]: | ||
| """ | ||
| Async get query embedding. |
There was a problem hiding this comment.
Async here? We do some sharing of sync and async code by computing headers, url and body in methods and calling those in sync and async requests/HTTP client APIs
|
Also implement sync and async. Also what is the difference between text and query? Actually some models do make a difference (normally you prefix the text with |
| @@ -0,0 +1,3 @@ | |||
| poetry_requirements( | |||
| name="poetry", | |||
There was a problem hiding this comment.
Are you sure you need this? I mean if you already have poetry_requirements that would require poetry to read, no?
| from dotenv import load_dotenv, find_dotenv | ||
| from llama_index.embeddings.deepinfra import DeepInfraEmbeddingModel | ||
|
|
||
| _ = load_dotenv(find_dotenv()) |
There was a problem hiding this comment.
Maybe a comment to explain what are you loading exactly, not obvious to me.
| print(response) | ||
| ``` | ||
|
|
||
| ### Use with text prefix |
There was a problem hiding this comment.
There should be one example which passes both query and text prefixes, with a short description on why that would be useful. I don't see how having two examples is better.
| """ | ||
| Add query prefix to queries. | ||
| """ | ||
| return [self._query_prefix + query for query in queries] |
There was a problem hiding this comment.
You can do [self._query_prefix + query for query in queries] if self._query_prefix else queries so it doesn't slow things down without query prefix
| print(response) | ||
| ``` | ||
|
|
||
| ### Asynchronous requests |
There was a problem hiding this comment.
I guess if all others have these verbose examples then fine, but the setup is the same, only the method call is different, and even that is the API of the BaseEmbedding (i.e nothing deepinfra specific). So you decide if you keep this or shorten it a bit (maybe a single setup + multiple fn calls).
There was a problem hiding this comment.
Dealt with it. Decreased the code repetition.
| """ | ||
| Chunk items into batches of size MAX_BATCH_SIZE. | ||
| """ | ||
| return [items[i : i + MAX_BATCH_SIZE] for i in range(0, len(items), MAX_BATCH_SIZE)] |
There was a problem hiding this comment.
MAX_BATCH_SIZE can be a constructor arg, and this function can be a method
There was a problem hiding this comment.
Isn't this parameter related with deepinfra's backend?
|
Looks good, send it upstream! |
Description
This pull request introduces a new integration for the DeepInfra Inference API.
Motivation and Context
Users are able to use DeepInfra's models using llama_index.
Fixes
New Package?
If yes, I have filled in the
tool.llamahubsection in thepyproject.tomland provided a detailedREADME.mdfor my new integration.Version Bump?
Type of Change
How Has This Been Tested?
Suggested Checklist:
make format; make lintto appease the lint gods