-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add support for token object for ProviderAzure. Fixes #195 #196
Add support for token object for ProviderAzure. Fixes #195 #196
Conversation
R/provider-azure.R
Outdated
@@ -58,7 +58,7 @@ ProviderAzure <- new_class( | |||
parent = ProviderOpenAI, | |||
properties = list( | |||
api_key = prop_string(), | |||
token = prop_string(allow_null = TRUE), | |||
token = prop_azure_token(), |
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.
Since we're only using this in a single place, there's no need to pull out into a separate function. And indeed, since we expect that pretty much all ProviderAzure
objects are created by chat_azure()
, I'd just do any validation there and not worry about it in the class.
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.
Agreed. That is a good idea. I added a local version of is_azure_token
from the AzureAuth package to utils, and removed the class validation from utils-S7. Now the validation is done in the chat_azure
function and is validated as a string in the ProviderAzure class.
I also renamed the argument token to azure_token, because I thought it was confusing with the term "token" being used for LLM tokens. Once the access_token is extracted from the object, I called it access_token to make it clear that it was no longer the azure_token object.
But I think we'd prefer not to take a dependency on AzureAuth, so we might need to do this from first principles. |
@SokolovAnatoliy What form of authentication are you using from |
@atheriel - we mainly use Azure Active Directory authentication via authorization_code (thanks to you) and device_code flows. We also use managed identities as well. It depends on the use case. I am not aware of us using service principals or Azure CLI, but that just may be a limitation of my knowledge based the practices of the users I interact with. |
@SokolovAnatoliy If you're using auth code & device code flows with |
@atheriel - for For us, they all use the exact same structure of creating an R6 object token of the |
@SokolovAnatoliy Thank you, that's very helpful. Great to hear that you're using Connect's OAuth integrations! I'm hoping to get them working with |
It's exciting to see all the continued progress on the package! @hadley, are you waiting on any more code changes before merging? I'm eager to use the new token handling for Azure calls. Currently, any app using |
@JamesHWade I'm waiting for @atheriel to say it's ok |
@JamesHWade I don't want to take this PR in its current form, not least because it doesn't actually solve the refresh issue. We're also hesitant to add However, I do have an alternative way to add an escape hatch for |
@atheriel - your solution sounds good, and I'd like to check it out, but I just want to comment that my updated code no longer has a dependency on AzureAuth at all. I updated it to allow So in the short-medium term, this does not add any dependencies to the ellmer package, but does allow us to use the code with a provided token and keep our chat_azure object from dying every 30 minutes. |
In lieu of adding support for Azure authentication packages directly, this commit adds a mechanism that at least allows them to be used and refreshed manually (see tidyverse#195 and tidyverse#196): a `credentials` argument that takes a function, similar to what we have for `chat_cortex()` today. The `credentials` function is called on every request to Azure, making it possible to refresh tokens that have expired prior to their use. I also did some internal refactoring of the `ProviderAzure` class in the process, and removed the need to set `api_key = ""` to use token-based authentication. Unit tests are included. Signed-off-by: Aaron Jacobs <[email protected]>
In lieu of adding support for Azure authentication packages directly, this commit adds a mechanism that at least allows them to be used and refreshed manually (see tidyverse#195 and tidyverse#196): a `credentials` argument that takes a function, similar to what we have for `chat_cortex()` today. The `credentials` function is called on every request to Azure, making it possible to refresh tokens that have expired prior to their use. I also did some internal refactoring of the `ProviderAzure` class in the process, and removed the need to set `api_key = ""` to use token-based authentication. Unit tests are included. Signed-off-by: Aaron Jacobs <[email protected]>
See #248. |
In lieu of adding support for Azure authentication packages directly, this commit adds a mechanism that at least allows them to be used and refreshed manually (see tidyverse#195 and tidyverse#196): a `credentials` argument that takes a function, similar to what we have for `chat_cortex()` today. The `credentials` function is called on every request to Azure, making it possible to refresh tokens that have expired prior to their use. I also did some internal refactoring of the `ProviderAzure` class in the process, and removed the need to set `api_key = ""` to use token-based authentication. Unit tests are included. Signed-off-by: Aaron Jacobs <[email protected]>
In lieu of adding support for Azure authentication packages directly, this commit adds a mechanism that at least allows them to be used and refreshed manually (see #195 and #196): a `credentials` argument that takes a function, similar to what we have for `chat_cortex()` today. The `credentials` function is called on every request to Azure, making it possible to refresh tokens that have expired prior to their use. I also did some internal refactoring of the `ProviderAzure` class in the process, and removed the need to set `api_key = ""` to use token-based authentication. Unit tests are included. Signed-off-by: Aaron Jacobs <[email protected]>
This pull request addresses issue #195. The problem solved is that the token passed to the chat object can expire, and then the chat can no longer be continued.
I propose that the AzureAuth token object be passed instead, which allows for the validate method to determine if the token is expired and the refresh method to refresh the token.
Help is requested on the
prop_azure_token()
function, to eliminate the dependency on the AzureAuth package.