[CognitiveServices] LUIS Runtime API Spec by pcostantini · Pull Request #2051 · Azure/azure-rest-api-specs

pcostantini · 2017-11-27T20:11:24Z

This PR includes the API Spec for the LUIS Runtime endpoint (Predictions):
https://westus.dev.cognitive.microsoft.com/docs/services/5819c76f40a6350ce09de1ac/operations/5819c77140a63516d81aee78

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

pcostantini · 2017-11-27T20:41:20Z

+    "hostTemplate": "{AzureRegion}.api.cognitive.microsoft.com",
+    "parameters": [
+      {
+        "$ref": "../../../Common/ExtendedRegions.json#/parameters/AzureRegion"


For GA, what is the region list that should be included?
Should the BasicRegion or ExtendedRegion sets be used instead?

If neither of these fit LUIS' profile, feel free to create your own AzureRegion definitions within the spec.

Thanks, still awaiting confirmation.

@pcostantini when you've got confirmation, ping and we can merge this.

Hi @fearthecowboy ,
Just got confirmation, the ExtendedRegions.json is the one to be used, so no further changes are needed.
Thanks!

fearthecowboy · 2017-11-27T22:59:16Z

+    "/{appId}": {
+      "get": {
+        "description": "Gets the published endpoint predictions for the given query.",
+        "operationId": "Prediction_GetPredictionsFromEndpointViaGet",


We use operationId to represent the group/method that gets generated for the SDKs.

The format should be <method-group>_<method-name> .

Ideally, something like Predictions_List (and let the description and parameters speak for themselves)

Updated to Predictions_Resolve

fearthecowboy · 2017-11-28T21:31:57Z

+        },
+        "entities": {
+          "type": "array",
+          "x-nullable": true,


Is there a reason you're explicitly marking things x-nullable: true ?

That affects code-generation in some specific ways, but shouldn't be used so liberally without a definite reason.

Most of what you've marked here would be nullable anyway.

Removed all x-nullable: true

fearthecowboy · 2017-11-28T21:35:00Z

+      },
+      "post": {
+        "description": "Gets the published endpoint prediction for the given long query.",
+        "operationId": "Prediction_GetPredictionsFromEndpointViaPost",


Are there any fundamental differences between using the GET and POST endpoints?

again this should be Predictions_List or something. Although, you can't have two methods with the same operation id, so if they are functionally the same, call the GET one Predictions_List2 and call this one Predictions_List and we'll give you the configuration to suppress generating the code for the GET (since the SDK caller won't ever care which one it's using)

If there are fundamental differences between GET and POST , we should examine them and see what we can do.

ACK. The API allows using GET or POST, but the end-result is the same and the parameters constraints are the same too.
For the SDK generation the POST can be used instead. Do you have the configuration required to disable an operation? (Please take a look at the current C# generation @ https://github.com/southworkscom/azure-sdk-for-net/blob/cognitiveservices-luis-runtime-sdk/src/SDKs/CognitiveServices/dataPlane/Language/LUIS-Runtime/Generated/PredictionExtensions.cs#L50 )

fearthecowboy

Overall, this spec still needs descriptions on all properties, parameters and model classes.

After that, we can generate the code and see if it looks like a usable SDK; after looking at this, I'm not exactly sure what the API is actually supposed to do...

fearthecowboy · 2017-11-28T21:38:50Z

+      "type": "object",
+      "properties": {
+        "query": {
+            "type": "string",


You need description fields on all your properties.

Added description to all parameters and attributes

fearthecowboy · 2017-11-28T21:39:47Z

+          "type": "string"
+        },
+        "startIndex": {
+          "type": "number"


It would be a good idea to have maximum and minimum values for numbers if you can.

Added description with their meaning. These values change based on the input. They represent a match within the input query

fearthecowboy · 2017-11-28T21:45:54Z

+        ],
+        "responses": {
+          "200": {
+            "description": "A JSON object containing the predictions.",


You shouldn't describe the payload in terms of encoding or transport, (ie, remove A JSON object containing the ) -- Give a description that describes what the output is. (What exactly is a list of predictions?)

Updated along with entity's description and their attributes

fearthecowboy · 2017-11-28T21:46:19Z

+            "in": "path",
+            "type": "string",
+            "required": true,
+            "description": "Format - guid. The application ID."


What is the application ID used for?

Added description for the Luis Application ID

pcostantini · 2017-11-29T01:52:00Z

Hi @fearthecowboy ,
We updated the spec based on your feedback.

Both GET and POST operation are the same... For the SDK Generation, do you have the configuration required to disable an operation?

Thanks!

azuresdkciprbot · 2017-11-29T02:45:21Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/cognitiveservices/data-plane/LUIS/Runtime/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

fearthecowboy · 2017-11-29T23:43:21Z

+## Runtime 2.0
+These settings apply only when `--tag=runtime_2_0` is specified on the command line.
+
+``` yaml $(tag) == 'runtime_2_0'


Change this section to this:

``` yaml $(tag) == 'runtime_2_0' input-file: v2.0/LUIS-Runtime.json # remove Resolve2 from code-generation (since the POST and GET operations are functionally identical. ) directive: remove-operation: Prediction_Resolve2 ```

It will suppress Resolve2 from getting code generation

fearthecowboy · 2017-11-29T23:45:19Z

+            "description": "The LUIS application ID (Guid)."
+          },
+          {
+            "name": "q",


On the q parameters, we should probably add:

"x-ms-client-name":"query",

in so the parameter name is query rather than q in the generated code (do this on Resolve2 as well, just for completion sake)

fearthecowboy · 2017-11-30T00:06:06Z

+
+### Runtime 2.0 - CSharp Settings
+These settings apply only when `--csharp` is specified on the command line.
+``` yaml $(csharp)


And, let's change this (uses new feature!)

csharp: override-client-name: LuisRuntimeAPI sync-methods: None license-header: MICROSOFT_MIT_NO_VERSION azure-arm: false namespace: Microsoft.Azure.CognitiveServices.Language.LUIS output-folder: $(csharp-sdks-folder)/CognitiveServices/dataPlane/Language/LUIS-Runtime/Generated clear-output-folder: true # csharp has support for modelAsExtensible now; replace modelAsString with that. directive: from: swagger-document where: $..['x-ms-enum'] transform: > if( $['modelAsString'] ) { $['modelAsExtensible'] = true; $['modelAsString'] = false; }

fearthecowboy · 2017-11-30T00:11:07Z

@pcostantini

I'm pretty good with this, just a couple minor tweaks that I've provided, and I think we're good-to-go.

add x-ms-client-name to query parameter to make a better codegen experience.
suppress Resolve2 from being codegen'd
tweak the x-ms-enum to switch to modelAsExtensible instead of modelAsString (we have a better means to generate code now for enums that may add items later)

azuresdkciprbot · 2017-11-30T00:56:09Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/cognitiveservices/data-plane/LUIS/Runtime/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

pcostantini · 2017-12-01T12:54:00Z

Hi @fearthecowboy ,
Got confirmation about the regions, the current ones are Ok. No more changes from out side =)
Regards,
-PabloC.

AutorestCI · 2017-12-01T15:41:41Z

No modification for AutorestCI/azure-sdk-for-ruby

AutorestCI · 2017-12-01T15:41:48Z

No modification for AutorestCI/azure-sdk-for-python

AutorestCI · 2017-12-01T15:41:52Z

No modification for AutorestCI/azure-sdk-for-node

pcostantini added 2 commits November 27, 2017 17:05

Add LUIS Runtime API Spec

ed96a5c

Change AzureRegion parameter to use ExtendedRegions template

da83d01

azuresdkci assigned fearthecowboy Nov 27, 2017

pcostantini mentioned this pull request Nov 27, 2017

[CognitiveServices] LUIS Runtime SDK Azure/azure-sdk-for-net#3889

Merged

9 tasks

pcostantini commented Nov 27, 2017

View reviewed changes

fearthecowboy reviewed Nov 28, 2017

View reviewed changes

fearthecowboy suggested changes Nov 28, 2017

View reviewed changes

pcostantini added 4 commits November 28, 2017 17:31

API Info Title - better description for API

84c120e

Operation Ids and Description update

aa673f0

Remove x-nullable: true

8a945c6

Add description to parameters and attributes

cefe9b4

Description field for LuisResult

c3ea0c8

fearthecowboy reviewed Nov 29, 2017

View reviewed changes

fearthecowboy reviewed Nov 30, 2017

View reviewed changes

pcostantini added 3 commits November 29, 2017 16:33

Remove Prediction_Resolve2 operation

8663698

Represent "q" parameter as "query" argument

5d0af83

modelAsExtensible

bb87a56

fearthecowboy approved these changes Nov 30, 2017

View reviewed changes

fearthecowboy merged commit 02fe069 into Azure:current Dec 1, 2017

pcostantini mentioned this pull request Dec 1, 2017

[CognitiveServices] LUIS Runtime SDK - Updates based on latest Spec Azure/azure-sdk-for-net#3910

Merged

9 tasks

stevengum mentioned this pull request Dec 9, 2017

Performed changes according to feedback on Azure/azure-rest-api-specs/pull/1833 omarelhariry137/azure-rest-api-specs#1

Closed

9 tasks

Conversation

pcostantini commented Nov 27, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcostantini commented Nov 29, 2017

Uh oh!

azuresdkciprbot commented Nov 29, 2017

Uh oh!

fearthecowboy Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy commented Nov 30, 2017

Uh oh!

azuresdkciprbot commented Nov 30, 2017

Uh oh!

pcostantini commented Dec 1, 2017

Uh oh!

AutorestCI commented Dec 1, 2017

Uh oh!

AutorestCI commented Dec 1, 2017

Uh oh!

AutorestCI commented Dec 1, 2017

Uh oh!

Reviewers

pcostantini commented Nov 27, 2017 •

edited by azuresdkci

Loading

fearthecowboy Nov 27, 2017 •

edited

Loading

fearthecowboy Nov 28, 2017 •

edited

Loading

fearthecowboy Nov 28, 2017 •

edited

Loading

fearthecowboy Nov 29, 2017 •

edited

Loading

fearthecowboy Nov 29, 2017 •

edited

Loading