Enable AzureOpenAI model testing #514
Conversation
.github/workflows/unit_tests.yml
Outdated
| @@ -27,6 +28,13 @@ jobs: | |||
| pip install -e .[test] | |||
| if [ -f requirements.txt ]; then pip install -r requirements.txt; fi | |||
| - name: Test with pytest | |||
| env: # Configure endpoints | |||
| AZUREAI_CHAT_ENDPOINT: ${{ secrets.AZUREAI_CHAT_ENDPOINT }} | |||
There was a problem hiding this comment.
These work in my fork, but of course would have to be updated in the guidance-ai/guidance repo.
There was a problem hiding this comment.
And I've just discovered that the environment variable will exist even if the secret doesn't. So my _env_or_skip() method doesn't quite work :-/
|
Tag @Harsha-Nori etc. |
riedgar-ms
changed the title
riedgar-ms
changed the title
|
The best way of handling the secrets in the build is not completely clear to me. If we're accepting PRs, then the PR could run and leak the secrets; hence a CI build |
|
In order for the tests to work, the six environment variables will need to be set as GitHub secrets. I do not have permissions to do this. |
|
I added the secrets into the env, but it looks like they don't get passed on a fork/PR. Since these are just extra tests, merging in for now so we can see if the secret-passing works properly. |
| def __init__( | ||
| self, | ||
| model: str, | ||
| azure_endpoint: str, | ||
| azure_deployment: str, | ||
| azure_deployment: str = None, | ||
| azure_ad_token_provider=None, | ||
| api_key: str = None, | ||
| tokenizer=None, |
There was a problem hiding this comment.
@riedgar-ms I assume there's no way to do auto-tokenizer detection logic on Azure OpenAI models?
There was a problem hiding this comment.
It will - it ultimately uses the model string, just like with the OpenAI class. Users to have to set this for themselves, though, since it's not part of the URI which AzureAI provides.
| @@ -77,55 +66,61 @@ def __init__( | |||
| azure_deployment=azure_deployment, | |||
| tokenizer=tokenizer, | |||
There was a problem hiding this comment.
similar to above, I imagine we may need to make initializing and passing a tokenizer a required arg if we can't autodetect? seems like the default behavior is to pass through None here
There was a problem hiding this comment.
Users should be able to pass through a particular tokeniser if they want, but otherwise, this should just use the same model argument as the OpenAI implementation.
There was a problem hiding this comment.
Yeah, it's just that the AzureOpenAI model deployments can be named anything, which would likely break the tiktoken detection logic we use in the OpenAI base classes. But that's OK - this is as good as our current behavior, so we can deal with cleaning it up later. It may be worth investigating if there is some way in Azure APIs to detect what the underlying model powering any deployment is (so that we get a tiktoken friendly string).
There was a problem hiding this comment.
Not if the user has passed in the model argument set to the name of the model used by the deployment. We can certainly investigate if there's a way of getting that info, short of querying the Azure API (to which the user might not have access)
Working to enable AzureOpenAI model testing. This:
models/_azure_openai.pyafter recent adjustments left it in a non-working state