-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Azure provider improvements #1763
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
base: main
Are you sure you want to change the base?
Azure provider improvements #1763
Conversation
@microsoft-github-policy-service agree |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/extension/byok/vscode-node/azureProvider.ts:1
- Temperature assignment on line 301 occurs after
delete body.temperatureon line 295. If a thinking model has a temperature value already set in the body, it will be deleted and then potentially re-added ifthis._temperatureis defined. The condition on line 300 should prevent this, but the logic is confusing. Consider moving the temperature application before the thinking model check or ensuring the temperature is only set when not a thinking model to avoid the delete-then-potentially-add pattern.
/*---------------------------------------------------------------------------------------------
src/extension/byok/vscode-node/azureProvider.ts:1
- The no-op function is unnecessarily wrapped in parentheses and uses 'any' type assertion. This should be simplified to
return (() => {}) as any;or better yet, use a more specific type assertion based on the return type ofChatResponseStream[K].
/*---------------------------------------------------------------------------------------------
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
package.json:3170
- The
customOAIModelsschema is missing thetemperatureproperty definition. While the TypeScript types inconfigurationService.ts(line 815) includetemperature?: number, the JSON schema lacks this field, which means VS Code won't provide autocomplete or validation for this property. Add atemperatureproperty with typenumber, description, and min/max constraints (0-2) similar to theazureModelsschema.
},
"required": [
"name",
"url",
"toolCalling",
"vision",
"maxInputTokens",
"maxOutputTokens",
"requiresAPIKey"
],
"additionalProperties": false
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
package.json:3159
- The
temperaturefield is missing from thecustomOAIModelsschema in package.json, but it's present in theazureModelsschema and implemented throughout the codebase (lines 814-815 in configurationService.ts). This inconsistency means the temperature feature won't have JSON schema validation or autocomplete for customOAIModels. Add the temperature field to the customOAIModels schema after line 3152 with the same definition as in azureModels (type: number, minimum: 0, maximum: 2, with appropriate description).
"requestHeaders": {
"type": "object",
"description": "Additional HTTP headers to include with requests to this model. These reserved headers are not allowed and ignored if present: forbidden request headers (https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_request_header), forwarding headers ('forwarded', 'x-forwarded-for', 'x-forwarded-host', 'x-forwarded-proto'), and others ('api-key', 'authorization', 'content-type', 'openai-intent', 'x-github-api-version', 'x-initiator', 'x-interaction-id', 'x-interaction-type', 'x-onbehalf-extension-id', 'x-request-id', 'x-vscode-user-agent-library-version'). Pattern-based forbidden headers ('proxy-*', 'sec-*', 'x-http-method*' with forbidden methods) are also blocked.",
"additionalProperties": {
"type": "string"
}
}
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
|
@bryanchen-d, I have completed with the changes to support these two additions. |
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
package.json:3087
- Missing property in schema: The
requiresAPIKeyproperty is missing from the azureModels schema, but it's present in the TypeScriptAzureModelConfiginterface (line 33 in azureProvider.ts) and in the documentation (line 32 of azure-provider-settings.instructions.md). This property should be added to the schema to provide IntelliSense support for users configuring Azure models. Add it after thevisionproperty:
"requiresAPIKey": {
"type": "boolean",
"description": "Whether the model requires an API key for authentication",
"default": true
} "requestHeaders": {
"type": "object",
"description": "Additional HTTP headers to include with requests to this model. These reserved headers are not allowed and ignored if present: forbidden request headers (https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_request_header), forwarding headers ('forwarded', 'x-forwarded-for', 'x-forwarded-host', 'x-forwarded-proto'), and others ('api-key', 'authorization', 'content-type', 'openai-intent', 'x-github-api-version', 'x-initiator', 'x-interaction-id', 'x-interaction-type', 'x-onbehalf-extension-id', 'x-request-id', 'x-vscode-user-agent-library-version'). Pattern-based forbidden headers ('proxy-*', 'sec-*', 'x-http-method*' with forbidden methods) are also blocked.",
"additionalProperties": {
"type": "string"
}
}
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.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
|
@eleanorjboyd I had to correct a few things based on Copilot suggestions, as it seems that AzureProvider, CustomOpenAIProvider and OpenAIProvider are dependent in this implementation. |
|
@lramos15 just checking in — I’d love feedback on this PR whenever you get a chance. Thanks for all your work on the project! |
For Provider "Azure" there are two improvements:
https://learn.microsoft.com/en-us/azure/ai-foundry/openai/how-to/responses
https://learn.microsoft.com/en-us/azure/ai-foundry/openai/how-to/reasoning
Next Steps:
What fixes this PR provides:
This changes the format of settings needed in settings.json
From this azure setting:
To this azure setting:
Here are some more information about implementation: