This is to add Cognitive Services data-plane API specs starting with Face API. by DavidLiCIG · Pull Request #1467 · Azure/azure-rest-api-specs

DavidLiCIG · 2017-07-26T06:20:57Z

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.

msftclas · 2017-07-26T06:21:00Z

@DavidLiCIG,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

fearthecowboy · 2017-07-27T17:54:06Z

You should have a readme.md file which acts as both documentation and AutoRest configuration in this request. (It also makes it easy to generate an SDK and see what kind of experience the SDK will provide)

Here's a simple readme.md (which should be placed at specification/cognitiveservices/data-plane/Face )

# Cognitive Services Face SDK

> see https://aka.ms/autorest

Example configuration for generating Face SDK.

The current release is `release_1_0`.

``` yaml

tag: release_1_0
```
# Releases

### Release 1.0
These settings apply only when `--tag=release_1_0` is specified on the command line.

``` yaml $(tag) == 'release_1_0'
input-file: v1.0/Face.json

```

## CSharp Settings
These settings apply only when `--csharp` is specified on the command line.
``` yaml $(csharp) 
csharp: 
  namespace: Azure.CognitiveServices.Face

```
## NodeJS Settings

These settings apply only when `--csharp` is specified on the command line.
``` yaml $(nodejs) 
nodejs: 
  namespace: Azure.CognitiveServices.Face

```

fearthecowboy

A few glitches in this document -- if you could fix them and update, that'd be great.

(I'll keep reviewing)

fearthecowboy · 2017-07-27T16:54:45Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+  "VerifyResponse": {
+   "type": "object",
+   "required": [
+    "isIdentifical"


You meant isIdentical not isIdentifical ?

fearthecowboy · 2017-07-27T16:55:59Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+  "CreatePersonResponse": {
+   "type": "object",
+   "required": [
+    "name"


You have required property name and no name property.

fearthecowboy · 2017-07-27T16:56:59Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+  "GetPersonFaceResponse": {
+   "type": "object",
+   "required": [
+    "persistedFacedId"


You meant persistedFaceId not persistedFacedId ?

olydis · 2017-07-27T23:15:48Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+         },
+         "faceLandmarks": {
+          "pupilLeft": {
+           "x": 412.7,


Note that you have a type mismatch here!
All these properties are modeled to have type Position which (see below) has integer x and y. Either the example or the model is wrong, please adjust one accordingly. (Maybe you have two types of "Position", one with integer coordinates and one without?)

balajikris · 2017-07-28T17:58:02Z

specification/cognitiveservices/data-plane/Face/readme.md

+
+```
+## NodeJS Settings
+


please remove this config :) we don't use namespaces in our nodejs sdks and the setting's description also says it will be applied for --csharp :)

Ah, sorry -- that was my fault. I pasted the sample that I was working with locally..

@DavidLiCIG -- I removed the nodejs section and pushed it to your branch.

Nice -- thanks 👍

also -- @balajikris -- This is a data-plane SDK -- do you currently publish other data-plane SDKs for nodejs yet?

@fearthecowboy -- yes, batch, keyvault, monitor, datalake are few services that come to mind.

salameer · 2017-07-31T20:30:42Z

Hi @DavidLiCIG

Are there any updates here?

DavidLiCIG · 2017-07-31T23:37:16Z

Hi:

I have updated the spec based on feedback. The CI is failing because the "File" type is not understood, but that's a feature gap for the CI validation I believe. @olydis

Thanks,
David

fearthecowboy · 2017-08-01T20:30:30Z

I'm working on finishing the review right now.

fearthecowboy

Hey @DavidLiCIG

You get the pleasure of being one of the first data-plane APIs that we're working with, so we're in some uncharted waters here, but this is really going to help us make sure that we've got the support for things we need to help make these SDKs great.

I'm trying to balance some of the conceptual things we've used for ARM resources as well as making sure that the generated API would be usable.

I've given feedback in a few places but the patterns are all throughout the document.

If you'd like we could meet and I we could go over some of this interactively, and we could come up with some patterns that would make a much better SDK experience.

fearthecowboy · 2017-08-01T20:58:00Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+     {
+      "name": "start",
+      "in": "query",
+      "description": "List person groups from the least personGroupId greater than the \"start\". It contains no more than 64 characters. Default is empty. ",


You've got a lot of text like Default is empty. in description fields. This is really not a good practice, as the description gets translated into doc comments in many languages and how each language supports default values can be different, plus it is creating a textual dependency between the description and the implementation ( ie, if the default changes in a later release, the text and the default flags have to be changed, and this is a real PITA later.

fearthecowboy · 2017-08-01T21:00:17Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+      "in": "query",
+      "description": "The number of person groups to list, ranging in [1, 1000]. Default is 1000. ",
+      "type": "integer",
+      "default": "1000",


This should not have an enum -- that's used for single values. you can put a range in place with

"minimum": 1, "maximum": 1000,

fearthecowboy · 2017-08-01T21:00:44Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+     {
+      "name": "top",
+      "in": "query",
+      "description": "The number of person groups to list, ranging in [1, 1000]. Default is 1000. ",


please don't specify default is ... (see above)

fearthecowboy · 2017-08-01T21:01:54Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+    ],
+    "responses": {
+     "200": {
+      "description": "A successful call returns an array of person groups and their information (personGroupId, name and userData).",


You probably don't want to specify details like (personGroupId, name and userData) -- the class definitions should do that nicely.

fearthecowboy · 2017-08-01T21:13:04Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+  "/persongroups/{personGroupId}/train": {
+   "post": {
+    "description": "Queue a person group training task, the training task may not be started immediately.",
+    "operationId": "PersonGroups_Train",


I'm curious about your operationIds.

The generator will group the methods into classes based on what's in front of the underscore, and method names from what is after.

The classes I see getting generated are

Faces PersonGroups Persons FaceLists

Are these the names of the classes that you'd like to see?

Some code would look like:

var api = new FaceAPI (); IList<DetectResponseElement> results = api.Faces.DetectAsync( ... )

Is Faces the right word there? Maybe Face?
And the result type is DetectResponseElement .. and you get an IList<> of those. -- could we find a better word like Detection (or something that describes a purpose rather than some Element

(If I'm way off base here, we can discuss it more ...)

fearthecowboy · 2017-08-01T21:23:36Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+    ]
+   },
+   "get": {
+    "description": "Retrieve a face list's information, including faceListId, name, userData and faces in the face list. ",


Again, don't bother mentioning property names.

fearthecowboy · 2017-08-01T21:29:55Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+   "properties": {
+    "faceId": {
+     "type": "string",
+     "description": "FaceId of the query face. User needs to call Face - Detect first to get a valid faceId. Note that this faceId is not persisted and will expire 24 hours after the detection call"


In the description User needs to call Face - Detect first to get a valid faceId. you say Face - Detect , but the operationId is Faces_Detect -- which generates a Faces class and the method Detect is implemented in several ways.

We should probably make sure this is clear and consistent. Saying Detect is probably fine, but the Face- part is not such a good idea

fearthecowboy · 2017-08-01T21:30:48Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+   "required": [
+    "faceId"
+   ],
+   "description": "",


Even though you've specified this should be flattened, the description should never be empty. Some languages may not work the same.

fearthecowboy · 2017-08-01T21:31:18Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+   "type": "object",
+   "description": "",
+   "properties": {
+    "anger": {


all missing description fields

fearthecowboy · 2017-08-01T21:33:14Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+    "$ref": "#/definitions/FindSimilarResponseElement"
+   }
+  },
+  "FindSimilarResponseElement": {


I'd avoid Response in class names, as they affect generation and don't give a good experience.

salameer · 2017-08-08T20:38:16Z

Hi @DavidLiCIG

Can we get some updates on your progress on this PR :)

fearthecowboy · 2017-08-08T21:26:29Z

@salameer -- we just synced up in person yesterday ... he's on it!

azuresdkciprbot · 2017-08-09T02:58:25Z

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/Face/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 87 Error(s): 16

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

DavidLiCIG · 2017-08-09T18:18:11Z

@salameer @fearthecowboy Hi, yep, just worked through and cleaned it up quite a bit.

fearthecowboy · 2017-08-11T17:50:59Z

specification/cognitiveservices/data-plane/Face/v1.0/Face.json

+  }
+ },
+ "x-ms-paths": {
+  "/detect?overload=url": {


Quick Question -- does the service expect the overload=url parameter to actually be sent? or does it infer the operation from the post contents?

fearthecowboy · 2017-08-11T18:18:29Z

specification/cognitiveservices/data-plane/Face/.gitignore

@@ -0,0 +1,2 @@
+out/**


hmm. don't include local .gitignore files in the PR. You can keep them local, but we really don't need them in the master repo.

fearthecowboy

Just a couple minor things -- drop the .gitignore file, and a question.

After that, I can approve immediately.

azuresdkciprbot · 2017-08-11T18:38: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/Face/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 87 Error(s): 16

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

fearthecowboy · 2017-08-11T21:34:44Z

I'm merging this -- the CI is falsely claiming that there isn't a type 'file'

(we're not publishing SDKs quite yet anyway :D)
.

AutorestCI · 2017-08-11T21:42:46Z

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

AutorestCI · 2017-08-11T21:42:47Z

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

AutorestCI · 2017-08-11T21:42:49Z

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

…d balancer (#1528) * Updating documentation for Microsoft.VisualStudio with project resource APIs. Corrections to existing examples. (#1498) * Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs (#1502) * Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs * Updated tag names * Microsoft.Sql - Added data sync to package-2015-05-preview (#1510) * Added advisors and data sync to package-2015-05-preview * Removed advisors so current branch is only data sync * Make MsDeploy and MsDeployLog Azure resources (#1519) * Add public certificates, Functions Admin Token and MSDeploy APIs * Revert resource definition changes and description change for site properties * Make type and name readonly properties. Add ARM envelope to MSDeployStatus object * Fix missing quotation issue. * Add long running operation to MS deploy * Make MsDeploy and MsDeployLog Azure resources * Fix some AutoRest validation issues. Use dictionary for msdeploy.setparameters * Ensure models are same across all schemas in Microsoft.Web * Fix azure resource type across all models * Remove conflicting ListOperations. It is not used for public Azure. * no YAML in glob patterns for report scripts * Copying 2017-06-01 to 2017-08-01 * Changes for new API version * Updated script to run autorest with grouping swaggers based on config file (#1529) * Removed the maxLength for entities (#1527) * Update folder structure example. (#1489) Agreed on the changes * [Travis-ci] Oad in ci status (#1520) * Breaking change build should fail if result contains error Skipping newly added files Incorporating review feedback * testing * log the result as oad.compare returns promise for caller * Use published oad * Revert "testing" This reverts commit a9be94d. * Removed maxLength from entity name (#1531) * This is to add Cognitive Services data-plane API specs starting with Face API. (#1467) * Adding data plane spec starting with Face * Move Spec to right folder * Fixing validation issues. * Fixing issues and adding a readme.md * Fixing examples by removing decimals. * Removing another decimal value * Update readme.md * Updating spec based on comments. * Removing .gitignore. * Relay: Removed the maxLength for entity names (#1535)

…d balancer (#1528) (#1543) * Updating documentation for Microsoft.VisualStudio with project resource APIs. Corrections to existing examples. (#1498) * Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs (#1502) * Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs * Updated tag names * Microsoft.Sql - Added data sync to package-2015-05-preview (#1510) * Added advisors and data sync to package-2015-05-preview * Removed advisors so current branch is only data sync * Make MsDeploy and MsDeployLog Azure resources (#1519) * Add public certificates, Functions Admin Token and MSDeploy APIs * Revert resource definition changes and description change for site properties * Make type and name readonly properties. Add ARM envelope to MSDeployStatus object * Fix missing quotation issue. * Add long running operation to MS deploy * Make MsDeploy and MsDeployLog Azure resources * Fix some AutoRest validation issues. Use dictionary for msdeploy.setparameters * Ensure models are same across all schemas in Microsoft.Web * Fix azure resource type across all models * Remove conflicting ListOperations. It is not used for public Azure. * no YAML in glob patterns for report scripts * Copying 2017-06-01 to 2017-08-01 * Changes for new API version * Updated script to run autorest with grouping swaggers based on config file (#1529) * Removed the maxLength for entities (#1527) * Update folder structure example. (#1489) Agreed on the changes * [Travis-ci] Oad in ci status (#1520) * Breaking change build should fail if result contains error Skipping newly added files Incorporating review feedback * testing * log the result as oad.compare returns promise for caller * Use published oad * Revert "testing" This reverts commit a9be94d. * Removed maxLength from entity name (#1531) * This is to add Cognitive Services data-plane API specs starting with Face API. (#1467) * Adding data plane spec starting with Face * Move Spec to right folder * Fixing validation issues. * Fixing issues and adding a readme.md * Fixing examples by removing decimals. * Removing another decimal value * Update readme.md * Updating spec based on comments. * Removing .gitignore. * Relay: Removed the maxLength for entity names (#1535)

DavidLiCIG added 2 commits July 25, 2017 23:17

Adding data plane spec starting with Face

5e734a0

Move Spec to right folder

e8f1ee2

msftclas added the cla-not-required label Jul 26, 2017

azuresdkci assigned fearthecowboy Jul 26, 2017

Fixing validation issues.

c56cc0e

fearthecowboy suggested changes Jul 27, 2017

View reviewed changes

olydis reviewed Jul 27, 2017

View reviewed changes

DavidLiCIG added 2 commits July 27, 2017 17:47

Fixing issues and adding a readme.md

e46c1ff

Fixing examples by removing decimals.

53aff4a

balajikris reviewed Jul 28, 2017

View reviewed changes

DavidLiCIG and others added 2 commits July 28, 2017 11:19

Removing another decimal value

c529bbb

Update readme.md

93cb8ef

fearthecowboy suggested changes Aug 1, 2017

View reviewed changes

Updating spec based on comments.

95cac3a

fearthecowboy reviewed Aug 11, 2017

View reviewed changes

fearthecowboy suggested changes Aug 11, 2017

View reviewed changes

Removing .gitignore.

3de638b

fearthecowboy approved these changes Aug 11, 2017

View reviewed changes

fearthecowboy merged commit 1c67d11 into Azure:current Aug 11, 2017

gmainar mentioned this pull request Aug 14, 2017

New API Version 2017-08-01: Adding Sku property into publicIP and loa… #1543

Merged

10 tasks

Conversation

DavidLiCIG commented Jul 26, 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!

msftclas commented Jul 26, 2017

Uh oh!

fearthecowboy commented Jul 27, 2017

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!

Choose a reason for hiding this comment

Uh oh!

salameer commented Jul 31, 2017

Uh oh!

DavidLiCIG commented Jul 31, 2017

Uh oh!

fearthecowboy commented Aug 1, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salameer commented Aug 8, 2017

Uh oh!

fearthecowboy commented Aug 8, 2017

Uh oh!

azuresdkciprbot commented Aug 9, 2017

Uh oh!

DavidLiCIG commented Aug 9, 2017

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!

azuresdkciprbot commented Aug 11, 2017

Uh oh!

fearthecowboy commented Aug 11, 2017

Uh oh!

AutorestCI commented Aug 11, 2017

DavidLiCIG commented Jul 26, 2017 •

edited by azuresdkci

Loading