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
partners[openai]: add tests for secret_str for keys #20982
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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.
Thank you!
monkeypatch.setenv("AZURE_OPENAI_API_KEY", "secret-api-key") | ||
monkeypatch.setenv("AZURE_OPENAI_AD_TOKEN", "secret-ad-token") | ||
chat_model = AzureChatOpenAI( | ||
openai_api_key="secret-api-key", |
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.
Should we remove the secrets from the constructor in this case, to enforce that they're read from the env vars?
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.
Yes. I was a tad too aggressive with the copy-paste.
# azure openai | ||
def test_azure_openai_api_key_is_secret_string() -> None: | ||
"""Test that the API key is stored as a SecretStr.""" | ||
llm = AzureOpenAI( |
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.
(nit) Are we able to parametrize these at all? e.g.,
from typing import Type
import pytest
@pytest.mark.parametrize(
"model_class", [AzureChatOpenAI, AzureOpenAI, AzureOpenAIEmbeddings]
)
def test_api_key_is_secret_string(model_class: Type) -> None:
model_instance = model_class(
openai_api_key="secret-api-key",
azure_endpoint="endpoint",
azure_ad_token="secret-ad-token",
api_version="version",
)
assert isinstance(model_instance.openai_api_key, SecretStr)
...
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.
Yes. Thank you - I'd overlooked this simplification.
Description: Add tests to check API keys and Active Directory tokens are masked
Issue: Resolves #12165 for OpenAI and Azure OpenAI models
Dependencies: None
Also resolves #12473 which may be closed.
Additional contributors @alex4321 (#12473) and @onesolpark (#12542)