Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Automatic access verification for AOAI services to develop and run on CAPI/managed AI resources #2764
base: main
Are you sure you want to change the base?
Automatic access verification for AOAI services to develop and run on CAPI/managed AI resources #2764
Changes from 18 commits
1702f14
09f9d04
4119ef8
9ed666f
da93eee
0264589
22af527
f9abbb7
5fc3ec2
2b18667
8a08eb2
d6c54fe
54ad50a
02bd29c
16e7169
e3680d2
8c41f6d
10ec3e8
e6ce450
b6d0848
91c85c7
8d03c7f
efd3c2e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Mmmm I suggest we keep the pattern that this file was using before (=> the
Set...
functions are just setting values, and then we check that the values are valid insideIsConfigured
).This saves us from possible bugs if some other part of the code is changing the authorization variables (and also delays checking the access and the validity of the values until they are actually needed).
Which means, here you just need to set the values for RecourceUtilization, ManagedResourceDeployment, and AOAIAccountName to some global variable. And then you can move the call to
VerifyAOAIAccount
to insideIsConfigured
instead, and you don't need the three boolean variables.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.
Would it make sense to handle this part of the request in platform? We handle other things there like ensuring the calls are aligned on api-version (I know we spoke about this previously so just wondering if there was a better reason for doing this in sys app)
Now we will have to remember to maintain both app and platform when moving to a newer api-version (and in app we would have to HF all minor releases).
Additionally, the verification log could be a system table so all the tracking and gating could be handled there
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.
I guess messages here are also for testing purposes. I think it's still a good idea to log this to telemetry though, so later you can add some
Session.LogMessage
instead of theMessage
s.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.
Minor: In case of parameters that are not super obvious, I like to add a sample of how they look like in the documentation comment. For example here you could do:
An Azure OpenAI account name. For example, if your Azure OpenAI endpoint is "contoso.openai.azure.com", use "contoso" for this parameter.