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

Add a more flexible credentials mechanism for chat_azure() #248

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

atheriel
Copy link
Contributor

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.

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Could you add a news bullet please?

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 atheriel force-pushed the azure-credentials-fn branch from 6247cfc to d1a08ad Compare January 13, 2025 17:44
@atheriel
Copy link
Contributor Author

Done.

@hadley hadley merged commit 587d627 into tidyverse:main Jan 13, 2025
11 checks passed
@hadley
Copy link
Member

hadley commented Jan 13, 2025

Thanks!

@atheriel atheriel deleted the azure-credentials-fn branch January 13, 2025 20:01
@SokolovAnatoliy
Copy link
Contributor

SokolovAnatoliy commented Jan 15, 2025

Hi @atheriel ,

My apologies on the delay testing this pr. I tested out this approach and wanted to make a couple of comments.

I was able to get the credential function working successfully by writing a custom function that takes into account the configuration that we are using.

In our configuration, we need both an api_key and a token. So my credential function needed to specify both. I also noticed that your default_azure_credentials could specify just an api_key. I think this is a bit confusing, since you would set the api_key of the chat object to NULL and then specify the key in the credentials function.

I would think that perhaps the api_key argument from the chat object should just be passed to the request automatically, without restrictions on using only one or the other. Are cases you are aware of, where you would only need a token without an api_key?

Was there a specific usage pattern that made you add this restriction to the pr?

In regards to the token argument. Have you observed a usage pattern where a user would want to pass a authentication token to the chat object as a string? From my understanding, you would need to have an extremely long lived token for this approach to be useful, which is not typical? I think this would also fail on Connect, when using the Integration approach, since Azure Integration would handle updating the token for you, but the chat object would not update.

I guess you could find the token in the chat environment and update it, but that doesn't seem like the right approach? Maybe it would be best to just remove the token argument, and just keep api_key and credentials.

This way, the api_key could be passed in, and then if there is a credentials function, you could use that to add a token, in whatever way you want, through a custom function.

The request could be structured like this, which would add the api_key, if provided, and add a credential function, if provided.

 
  if (!is.null(provider@api_key)) {
    req <- req_headers(req, `api-key` = provider@api_key, .redact = "api-key")
  }
  
  if (!is.null(provider@credentials)) {
  req <- req_headers(
    req,
    !!!provider@credentials(),
    .redact = c("Authorization")
  )
  }

So in my case, I would use a custom function like retrieve_dow_azure_token to retrieve my token object, manage refreshes, and the differences between Connect/Workbench, and then have that function provide an updated access_token to the credentials function.

I like this, since I would not need to deal with an api_key in two places.

dow_azure_credentials <- function(){
  
  function(){
    token <- retrieve_dow_azure_token()
    access_token <- token$credentials$access_token
    list( Authorization = paste("Bearer", access_token)) 
  }
}

Then my chat object could be create like this for Dow users.

ellmer::chat_azure(
  endpoint = Sys.getenv("AZURE_OPENAI_ENDPOINT"),
  deployment_id = Sys.getenv("AZURE_OPENAI_DEPLOYMENT_NAME"),
  api_version = Sys.getenv("AZURE_OPENAI_API_VERSION"),
  api_key = Sys.getenv("AZURE_OPENAI_API_KEY"),
  credentials = dow_azure_credentials(),
  system_prompt = "",
  echo = "none"
)

What do you think about this, and is this how you intended for users to use the credentials function?

@atheriel
Copy link
Contributor Author

Thanks for the testing!

Was there a specific usage pattern that made you add this restriction to the pr?

No, I just misunderstood Azure's documentation. It makes it seem like you use either API keys or Entra ID auth, but not both. I can certainly restore the previous treatment of API keys.

In regards to the token argument. Have you observed a usage pattern where a user would want to pass a authentication token to the chat object as a string? From my understanding, you would need to have an extremely long lived token for this approach to be useful, which is not typical? I think this would also fail on Connect, when using the Integration approach, since Azure Integration would handle updating the token for you, but the chat object would not update.

No, I largely left this intact because I didn't want to deprecate an parameter so soon in ellmer's life. I agree that folks are unlikely to use it, although I do think some Entra ID configurations can have tokens that last ~1h, which is long enough for many chat sessions. I'll mark it deprecated (unless @hadley objects).

@hadley
Copy link
Member

hadley commented Jan 16, 2025

@atheriel no concerns from my side.

atheriel added a commit to atheriel/elmer that referenced this pull request Jan 16, 2025
This implements the suggestion from testers of tidyverse#248, who rightly pointed
out that my assumption that API keys and Entra ID credentials are
mutually exclusive was incorrect.

Unit tests are included.

Signed-off-by: Aaron Jacobs <[email protected]>
hadley pushed a commit that referenced this pull request Jan 17, 2025
…256)

This implements the suggestion from testers of #248, who rightly pointed
out that my assumption that API keys and Entra ID credentials are
mutually exclusive was incorrect.

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

Successfully merging this pull request may close these issues.

3 participants