-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
LUIS runtime changes #5424
LUIS runtime changes #5424
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,52 @@ | |
"version": "2.0" | ||
}, | ||
"x-ms-parameterized-host": { | ||
"hostTemplate": "{Endpoint}/luis/v2.0", | ||
"useSchemePrefix": false, | ||
"hostTemplate": "{AzureRegion}.api.cognitive.microsoft.{AzureCloud}/luis/v2.0", | ||
"parameters": [ | ||
{ | ||
"$ref": "#/parameters/Endpoint" | ||
"name": "AzureRegion", | ||
"description": "Supported Azure regions for Cognitive Services endpoints", | ||
"x-ms-parameter-location": "client", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason these are supposed to be client level parameters? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nebadr @AmirGeorge - client level params? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tsuwandy for possible input. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dsgouda We are not aware exactly why but is it causing problem? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not necessarily a problem. We want to ensure your intent is to tie each client instance to a particular Azure region. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dsgouda Yes, this is our intent. Confirmed. |
||
"required": true, | ||
"type": "string", | ||
"in": "path", | ||
"x-ms-skip-url-encoding": true, | ||
"x-ms-enum": { | ||
"name": "AzureRegions", | ||
"modelAsString": false | ||
}, | ||
"enum": [ | ||
"westus", | ||
"westeurope", | ||
"southeastasia", | ||
"eastus2", | ||
"westcentralus", | ||
"westus2", | ||
"eastus", | ||
"southcentralus", | ||
"northeurope", | ||
"eastasia", | ||
"australiaeast", | ||
"brazilsouth", | ||
"virginia" | ||
] | ||
}, | ||
{ | ||
"name": "AzureCloud", | ||
"description": "Supported Azure Clouds for Cognitive Services endpoints", | ||
"x-ms-parameter-location": "client", | ||
"required": true, | ||
"type": "string", | ||
"in": "path", | ||
"x-ms-skip-url-encoding": true, | ||
"x-ms-enum": { | ||
"name": "AzureClouds", | ||
"modelAsString": false | ||
}, | ||
"enum": [ | ||
"com", | ||
"us" | ||
] | ||
} | ||
] | ||
}, | ||
|
@@ -35,6 +76,7 @@ | |
"name": "appId", | ||
"in": "path", | ||
"type": "string", | ||
"format": "uuid", | ||
"required": true, | ||
"description": "The LUIS application ID (guid)." | ||
}, | ||
|
@@ -74,7 +116,7 @@ | |
{ | ||
"name": "bing-spell-check-subscription-key", | ||
"in": "query", | ||
"description": "The subscription key to use when enabling bing spell check", | ||
"description": "The subscription key to use when enabling Bing spell check", | ||
"type": "string" | ||
}, | ||
{ | ||
|
@@ -115,6 +157,7 @@ | |
"name": "appId", | ||
"in": "path", | ||
"type": "string", | ||
"format": "uuid", | ||
"required": true, | ||
"description": "The LUIS application ID (Guid)." | ||
}, | ||
|
@@ -155,7 +198,7 @@ | |
{ | ||
"name": "bing-spell-check-subscription-key", | ||
"in": "query", | ||
"description": "The subscription key to use when enabling bing spell check", | ||
"description": "The subscription key to use when enabling Bing spell check", | ||
"type": "string" | ||
}, | ||
{ | ||
|
@@ -327,7 +370,7 @@ | |
}, | ||
"additionalProperties": { | ||
"type": "object", | ||
"description": "List of additional properties. E.g.: score and resolution values for pre-built LUIS entities." | ||
"description": "List of additional properties. For example, score and resolution values for pre-built LUIS entities." | ||
}, | ||
"required": [ | ||
"entity", | ||
|
@@ -406,14 +449,5 @@ | |
} | ||
}, | ||
"parameters": { | ||
"Endpoint": { | ||
"name": "Endpoint", | ||
"description": "Supported Cognitive Services endpoints (protocol and hostname, for example: https://westus.api.cognitive.microsoft.com).", | ||
"x-ms-parameter-location": "client", | ||
"required": true, | ||
"type": "string", | ||
"in": "path", | ||
"x-ms-skip-url-encoding": true | ||
} | ||
} | ||
} |
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 change will prevent the users from using customized domain/containers.
Please revert this change.
If you believe this change is a must, we can kickstart an email thread to discuss abt this.
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.
@diberry has this been addressed?
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.
@nebadr @AmirGeorge Is this something you can address? I don't think I introduced this issue.
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.
@dsgouda @diberry this change has been reverted in the second commit.
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.
@nebadr - should I close this PR?
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.
@diberry We want to merge this PR. I meant the change has been reverted here in this PR but in the second commit (this change appears to be outdated).
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.
@nebadr Do I need to pull something into this PR to fix or it is good to go as it is now?
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.
@diberry No, it is good. Thanks.