Skip to content
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

Fixes: community: fix AzureSearch Oauth with azure_ad_access_token #26995

Conversation

clairebehue
Copy link
Contributor

@clairebehue clairebehue commented Sep 30, 2024

Description:
AzureSearch vector store: create a wrapper class on azure.core.credentials.TokenCredential (which is not-instantiable) to fix Oauth usage with azure_ad_access_token argument

Issue: the issue it fixes

Dependencies: None

  • Lint and test

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Sep 30, 2024
Copy link

vercel bot commented Sep 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Dec 16, 2024 5:54am

@dosubot dosubot bot added community Related to langchain-community Ɑ: vector store Related to vector store module 🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature labels Sep 30, 2024
@levalencia
Copy link
Contributor

your fix seems more stable than mine, so go ahead and merge

@clairebehue
Copy link
Contributor Author

@efriis since you reviewed #24330 any chance you could take a look at this quick fix ? 🙏 (do not hesitate to indicate if i should follow a different process to get approval for this PR)

Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this doesn't work anyways, should we just remove it?

The original justification was that this was an auth format that AzureOpenAI supports, and I'm starting to think it's an antipattern based on the difficulty to use it from the azure sdk

I think it's ok that Azure OpenAI service supports some different auth formats than azure vectorsearch given they're configured differently.

What are your thoughts?

@clairebehue
Copy link
Contributor Author

since this doesn't work anyways, should we just remove it?

The original justification was that this was an auth format that AzureOpenAI supports, and I'm starting to think it's an antipattern based on the difficulty to use it from the azure sdk

I think it's ok that Azure OpenAI service supports some different auth formats than azure vectorsearch given they're configured differently.

What are your thoughts?

what do you mean by it doesn't work anyway? this fix allows to use oauth token successfully.

@clairebehue
Copy link
Contributor Author

@efriis can we proceed on this PR? we'd love to reuse it on our side (Dataiku company) to allow our customers to use oauth per user using the oauth tokens.

@SirSiLves
Copy link

@efriis is there any chance to get this PR merged soon? We have to use azure_ad_access_token cause our security too.

@efriis
Copy link
Member

efriis commented Dec 16, 2024

sorry for the delay. I still don't love these wrapper classes on tokens that are auto-computing expirations - hopefully the azure team has a better auth mechanism in store for some of the langchain-azure integrations!

@efriis efriis enabled auto-merge (squash) December 16, 2024 05:55
@efriis efriis merged commit fb44e74 into langchain-ai:master Dec 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature community Related to langchain-community size:S This PR changes 10-29 lines, ignoring generated files. Ɑ: vector store Related to vector store module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants