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

Azure chat object fails once token expires #195

Open
SokolovAnatoliy opened this issue Dec 5, 2024 · 0 comments
Open

Azure chat object fails once token expires #195

SokolovAnatoliy opened this issue Dec 5, 2024 · 0 comments

Comments

@SokolovAnatoliy
Copy link
Contributor

The current method for passing a token to the ProviderAzure class passes the token as a string. Once the chat object is created, there is no way to update the token string if the token expires. The consequence is that the entire chat must be re-created, losing all context.

I am suggesting a change to address this issue by passing a token object from the AzureAuth package. The token has built in validate and refresh methods. The chat can now check if the token is valid, and if that fails, attempt to refresh the token.

I wrote up these changes in a pull request.

I also noticed that the approach to validate the arguments to the ProviderAzure class uses the S7::as_class function. The issue is that there is not a way to validate an R6 class (what the AzureAuth package returns for the token) using the S7::as_class function.

I added a workaround in the pull request like this, but I don't love it because it will make the elmer package depend on the AzureAuth package. I used a blank token to retrieve the class name to ensure that the class "AzureToken" was not hard coded.

prop_azure_token <- function(allow_null = FALSE) {
  force(allow_null)
  ## The call to AzureAuth ensures that the class name of the Azure Token does not change
  class_name = AzureAuth::AzureToken$self$classname
  ## The returned token object is R6 - not currently supported by the S7::as_class()
  ## so I made a new S3 class to use validator
  AzureToken_class <- new_S3_class(class_name)

  new_property(
    class = if (allow_null) NULL | AzureToken_class else AzureToken_class,
    validator = function(value) {
      if (allow_null && is.null(value)) {
        return()
      }

      if (!inherits(value, class_name)) {
        paste0("must be an object of class <", class_name, ">, not ", obj_type_friendly(value), ".")
      }
    }
  )
}

I did not add the dependency in the pull request, because maybe there is a better way to do this? I added this code to the pull request, because without it, the change would not work, but any fixes would be very welcome.

SokolovAnatoliy added a commit to SokolovAnatoliy/elmer that referenced this issue Dec 5, 2024
atheriel added a commit to atheriel/elmer that referenced this issue Jan 10, 2025
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]>
atheriel added a commit to atheriel/elmer that referenced this issue Jan 10, 2025
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]>
atheriel added a commit to atheriel/elmer that referenced this issue Jan 13, 2025
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]>
hadley pushed a commit that referenced this issue Jan 13, 2025
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant