cosmosdbnosql: Added Cosmos DB NoSQL Semantic Cache Integration with tests and jupyter notebook #24424
Conversation
gsa9989
commented
Jul 19, 2024
gsa9989
commented
- Added Cosmos DB NoSQL Semantic Cache Integration with tests and jupyter notebook
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dosubot
bot
added
size:XXL
|
@ccurme can you please review this PR |
dosubot
bot
added
size:L
…a9989/langchain into users/garagundi/cosmosdbnosql
|
Friendly ping @ccurme for a review here |
There was a problem hiding this comment.
Thanks for the ping.
See our contributing guidelines for how to format and lint code locally for future commits. I ran the formatter myself for now but there are still some type issues, would you mind taking a look? https://github.com/langchain-ai/langchain/actions/runs/10582689581/job/29323028501?pr=24424
The PR summary mentions a notebook, but I don't see any updates to documentation here. I think this would be good to add, otherwise the new cache will not be discoverable. If you edit docs/docs/integrations/llm_caching.ipynb, it will get built into the docs preview here. Let me know if you have any questions!
libs/community/tests/integration_tests/cache/test_azure_cosmosdbnosql_cache.py
Outdated
Show resolved
Hide resolved
|
Friendly ping @ccurme @baskaryan @isahers1 for a review here. |
| "%%time\n", | ||
| "# The first time, it is not yet in cache, so it should take longer\n", | ||
| "llm.invoke(\"Tell me a joke\")" |
There was a problem hiding this comment.
(nit) Could be worth running these cells to demonstrate the speedup.
| @@ -2102,7 +2106,7 @@ def __init__( | |||
| ef_construction: int = 64, | |||
| ef_search: int = 40, | |||
| score_threshold: Optional[float] = None, | |||
| application_name: str = "LANGCHAIN_CACHING_PYTHON", | |||
| application_name: str = "LangChain-CDBNoSQL-SemanticCache-Python", | |||
There was a problem hiding this comment.
- Couldn't this break user workflows?
- What is the motivation for this change? We have
AzureCosmosDBSemanticCacheandAzureCosmosDBNoSqlSemanticCacheseparately, but then we are specifyingNoSQLhere.
There was a problem hiding this comment.
It's just a name change, this should not break any workflows. This user agent is being used for tracking purposes only. We want to have it in this pattern, as Azure CosmosDB is integrated in various AI frameworks (microsoft internal and external), and we want to create dashboards to track the usage using these user agents.
There was a problem hiding this comment.
Yes but if someone is using the default application_name, after this update the name will change, right? Even if it's just telemetry, I'm not sure we want to change this for users without warning.
It's OK to make such a change if important, but users are already free to specify application_name however they like-- why is it important to change the default here? Let me know if I'm misunderstanding.
There was a problem hiding this comment.
Because we have seen that a lot of users are not specifying the application_name, and we can only see default value in our telemetry. So, that's the reason for this update as we can keep the application_name consistent across all our integrations.
| from azure.cosmos import ContainerProxy, CosmosClient | ||
| from azure.identity import ClientSecretCredential | ||
|
|
||
| USER_AGENT = ("LlamaIndex-CDBNoSql-VectorStore-Python",) |
libs/community/langchain_community/vectorstores/azure_cosmos_db_no_sql.py
Outdated
Show resolved
Hide resolved
…ngchain into users/garagundi/cosmosdbnosql
|
@ccurme Hey, apologies for the delay on the PR—it fell through the cracks. I've addressed all the comments, and it's ready for review. Please take a look when you have a chance. Thanks! |
|
Friendly ping @ccurme @baskaryan @isahers1 for a review here. |
|
Friendly ping @efriis @ccurme @baskaryan for a final review here. |
|
@aayush3011 It looks like there are some merge conflicts, are you able to take a look? |
dosubot
bot
added
the
lgtm
