-
Notifications
You must be signed in to change notification settings - Fork 231
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
generator: support of azure openai #817
Conversation
DCO Assistant Lite bot All contributors have signed the DCO ✍️ ✅ |
I have read the DCO Document and I hereby sign the DCO |
Signed-off-by: eric-therond <[email protected]>
e358dbb
to
3d8a442
Compare
thanks! will take a look |
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.
This looks like a great start. To expand on consistency, I have suggested some refactor items and a few patterns to enable more flexible configuration.
Signed-off-by: eric-therond <[email protected]>
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.
A couple minor requests.
Signed-off-by: eric-therond <[email protected]>
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.
Testing shows the api_version must be specified.
The new class needs to be added to the list generators excluded from instance creation tests with a reason similar to # requires additional env variables tested in own test class
here: https://github.com/leondz/garak/blob/56d1a74cf7e4bd8a136e5f3fbbd248c9c7ec2529/tests/generators/test_generators.py#L169-L181
I have also provided some direction for how mock responses can be used to unit test generate
.
Signed-off-by: eric-therond <[email protected]>
Signed-off-by: eric-therond <[email protected]>
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.
This looks fairly complete end-to-end testing is in flight.
A minor tweak to the parameters for invocation might make it easier for users though.
From a user interactions perspective I think the utilization of --model_name
to refer to the underlying model behind the deployment may be confusing to first time users, as well as consumers of the reports, as the model_name
is part of the top level reference for the report run. Generally model_type + model_name
relates to the name of the application under test. For most users I suspect they think of the deployment name as the identifier of the Application
.
This line of thought seem to match the Azure documentation which adds a note about deployment name being the primary exposed reference:
When you access the model via the API, you need to refer to the deployment name rather than the underlying model name in API calls, which is one of the key differences between OpenAI and Azure OpenAI. OpenAI only requires the model name. Azure OpenAI always requires deployment name, even when using the model parameter. In our docs, we often have examples where deployment names are represented as identical to model names to help indicate which model works with a particular API endpoint. Ultimately your deployment names can follow whatever naming convention is best for your use case.
As currently configured the model_name
is used to determine the chat vs completion and context_lengths supported by in the target service. This seems more like metadata about the application than the name of the application itself.
I propose that --model_name
could be the azure deployment name and an accepted environment variable could set the underlying detail of what service model backs the deployment under test. Possibly something like AZURE_DEPLOYMENT_MODEL=gpt-4o
.
This would also reduce the complexity in _load_client()
and remove the need for name_backup
as self.deployment_model
could be resolved and held as the openai mapped value used for lookup of completion pattern and context_length without needing to be concerned about clobbering the value of self.name
consumed by _call_model()
.
Consider if I named a deployment fooAI
and backed it with gpt-4o:
The garak cli call for this would currently be something like (assuming correct env vars or config files are in place):
garak -m azure -n gpt-4o -g 1 -p continuation
The report would state generator: azure.gpt-4o
.
With the suggested revision:
garak -m azure -n fooAI -g 1 -p continuation
The report would state generator: azure.fooAI
.
Signed-off-by: eric-therond <[email protected]>
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.
@eric-therond, thanks for the latest refactor. Test failure are due to #837, the removals noted should get tests passing again.
Signed-off-by: eric-therond <[email protected]>
Signed-off-by: eric-therond <[email protected]>
Signed-off-by: Jeffrey Martin <[email protected]>
Signed-off-by: Jeffrey Martin <[email protected]>
update - we're waiting on azure acct to be provisioned; once this module is validated, if all goes well, it'll merge |
Testing complete, thank you for the contribution! |
following a previous PR comment that suggested to use OpenAI API compatibility class
#638 (comment)