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

Determine whether we really need to support API keys and Entra ID tokens simultaneously in chat_azure() #274

Closed
atheriel opened this issue Jan 24, 2025 · 13 comments

Comments

@atheriel
Copy link
Collaborator

atheriel commented Jan 24, 2025

@SokolovAnatoliy reported in #248 (comment) that they use both API keys and Entra ID for authentication, so I ensured this was possible in #256.

But Azure's docs on this are unequivocal that one or the other is required, but not both:

Azure OpenAI provides two methods for authentication. You can use either API Keys or Microsoft Entra ID.

We should sort out which party is incorrect. The internals of chat_azure() would be much simpler if only one or the other is required.

@JamesHWade
Copy link
Contributor

Unfortunately, the only way we can authenticate given our internal configuration is both. Most other Azure OpenAI users will not require this, but I'd really like to still be able to use the package internally.

The reason we require both is that the token is coming from a different RG than the Azure AI Foundry instance. This is not a configuration I have the power to change.

@atheriel
Copy link
Collaborator Author

The reason we require both is that the token is coming from a different RG than the Azure AI Foundry instance. This is not a configuration I have the power to change.

It's a bit surprising that this wouldn't be mentioned in their docs, but I'm happy to accommodate here, and I suspect you won't be alone in having less-than-optimal Azure resource group arrangements!

@JamesHWade
Copy link
Contributor

@atheriel, looks like the check_exclusive(token, credentials, .require = FALSE) is back in the current version of ellmer. Was this intentional?

I installed the dev pkg version and noticed my code stopped working.

check_exclusive(token, credentials, .require = FALSE)

@atheriel
Copy link
Collaborator Author

@JamesHWade Yes, because I thought you'd probably be using the credentials function to pass the token but handle refreshing correctly. Is that not what you're doing?

@JamesHWade
Copy link
Contributor

@SokolovAnatoliy has a better handle on this than me. I'll defer to him. I'm using his contribution to our internal package anyway.

@SokolovAnatoliy
Copy link
Contributor

@atheriel - sorry I replied in a different thread here #262 (comment). Could you take a look at that line specifically.

@atheriel
Copy link
Collaborator Author

atheriel commented Jan 28, 2025

(Moved here from #262.)

I don't see this error with either pattern?

> chat <- chat_azure(
+   endpoint = "https://test.openai.azure.com/",
+   deployment_id = "gpt-4o-mini",
+   credentials = function() "aaron"
+ )
Using api_version = "2024-10-21".
> chat <- chat_azure(
+   endpoint = "https://test.openai.azure.com/",
+   deployment_id = "gpt-4o-mini",
+   token = NULL,
+   credentials = function() "aaron"
+ )
Using api_version = "2024-10-21".
Warning message:
The `token` argument of `chat_azure()` is deprecated as of ellmer 0.1.1.Support for the static `token` argument (which quickly expires) will be dropped in next release.
  Use ambient Azure credentials instead, or pass an explicit `credentials` argument.
> chat <- chat_azure(
+   endpoint = "[https://test.openai.azure.com/](vscode-file://vscode-app/usr/share/positron/resources/app/out/vs/code/electron-sandbox/workbench/workbench.html#)",
+   deployment_id = "gpt-4o-mini",
+   token = NULL,
+   api_key = "secret",
+   credentials = function() "aaron"
+ )
Using api_version = "2024-10-21".

@JamesHWade
Copy link
Contributor

Here's what I'm seeing:

chat <- ellmer::chat_azure(
   endpoint = "https://test.openai.azure.com/",
   deployment_id = "model",
   api_key = "abc123",
   credentials = function() "james",
)
#> Using api_version = "2024-10-21".
#> Error in `ellmer::chat_azure()`:
#> ! Exactly one of `token` or `credentials` must be supplied.

Created on 2025-01-28 with reprex v2.1.0

@SokolovAnatoliy
Copy link
Contributor

SokolovAnatoliy commented Jan 28, 2025

@atheriel I see the problem. The current version of ellmer that we are using is 0.1.0.9000, while your lifecycle function is looking for 0.1.1.

deprecate_warn(when = "0.1.1"

So, for us, it always fails. I don't see the 0.1.1 yet?

But I also think the approach will fail if you provide a token instead of a credentials function. Consider the scenarios below.

Scenario 1:

token <- lifecycle::deprecated()
creds <- function(x) "tony"
rlang::check_exclusive(token,  creds, .require = FALSE)

This works, since the token is a function, and not NULL, which is the reason you aren't seeing this in your test case.

Scenario 2:

token <- NULL
creds <- function(x) "tony"
rlang::check_exclusive(token,  creds, .require = FALSE)

This fails, since token <- NULL, which is what happens in our case, as described above.

Scenario 3:

token <- "tony"
creds <- NULL
rlang::check_exclusive(token,  creds, .require = FALSE)

This will fail, because the argument credentials is NULL at the point of this check when token is provided.

Scenario 4:

token <- lifecycle::deprecated()
creds <- NULL

rlang::check_exclusive(token,  creds, .require = FALSE)

This will work. This is why, for you, it works without a token or credentials function in your test. I think maybe there is a better way to check this? It seems to be very reliant on the deprecated() function returning nothing?

@atheriel
Copy link
Collaborator Author

atheriel commented Jan 28, 2025

I'm having a hard time understanding this still, probably due to the pseudocode -- can you provide a reprex?

Edit: missed the one James provided.

@atheriel atheriel reopened this Jan 28, 2025
@atheriel
Copy link
Collaborator Author

I see it now. There's some interaction with devtools::load_all() here that's masking the error on my end.

atheriel added a commit that referenced this issue Jan 28, 2025
This was somehow masked in my local testing due to
`devtools::load_all()` but reported in #274. This commit makes the
exclusive check before the deprecation check, which resolves the issue.

Signed-off-by: Aaron Jacobs <[email protected]>
@atheriel
Copy link
Collaborator Author

Fix in #284, I think.

atheriel added a commit that referenced this issue Jan 28, 2025
This was somehow masked in my local testing due to
`devtools::load_all()` but reported in #274. This commit makes the
exclusive check before the deprecation check, which resolves the issue.

Signed-off-by: Aaron Jacobs <[email protected]>
@SokolovAnatoliy
Copy link
Contributor

@atheriel this is working, thanks a lot for all your quick help!

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

3 participants