[Cogs Face] Align with latest released version of Face API with million-scale features. #3552

lebronJ · 2018-07-31T04:57:24Z

(I am one of the main devs working on Face API and Face containers.)

This is to align Face API swaggers with latest published version with million-scale features.

We have locally generated AutoRest SDK and written more tests to ensure all functionalities work.
This is also to catch container SDK preview with relatively high priority.

===================================================================
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.

azuresdkci · 2018-07-31T04:57:25Z

Can one of the admins verify this patch?

AutorestCI · 2018-07-31T04:58:08Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#3011

AutorestCI · 2018-07-31T04:58:27Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#3241

AutorestCI · 2018-07-31T04:58:59Z

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#1519

AutorestCI · 2018-07-31T04:59:16Z

Automation for azure-sdk-for-java

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-java#2270

AutorestCI · 2018-07-31T05:00:44Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2374

yangyuan

LGTM

yangyuan · 2018-07-31T06:09:15Z

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

+        }
+      }
+    },
+    "/largepersongroups/{largePersonGroupId}/persons/{personId}": {


Just double confirm, the paths are not case sensitive right?

Yes, at least the generated .NET SDK works well. I'd like to append a minor commit to unify them to lower-case.
(BTW, the parameters inside {} are actually user-provided variables, rather than part of path.)

I bring this up because I saw: "/largepersongroups/{largePersonGroupId}/persons/{personId}/persistedFaces/{persistedFaceId}"
For parameters, lowerCamelCase is good enough.

Yes, thanks.

lebronJ · 2018-07-31T15:43:36Z

@annatisch, I think this is good to merge, thanks.

annatisch · 2018-07-31T16:28:56Z

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

+      },
+      "get": {
+        "description": "List all faces in a large face list, and retrieve face information (including userData and persistedFaceIds of registered faces of the face).",
+        "operationId": "LargeFaceListFace_List",


I think this operation ID should be changed to "LargeFaceList_ListFaces"
This would make it more consistent with the other operations like "LargeFaceList_GetFace" and "LargeFaceList_DeleteFace" etc.

Alternatively you could rename all of them: "LargeFaceListFace_Delete", "LargeFaceListFace_Get" etc

Good catch! Thanks for pointing out this! Fixed with the latest commit.

@annatisch Also updated our generated .NET SDK and all works.

…ple naming.

huxuan · 2018-08-01T07:54:14Z

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

+    "/largepersongroups/{largePersonGroupId}/persons/{personId}/persistedfaces": {
+      "post": {
+        "description": "Add a representative face to a person for identification. The input face is specified as an image with a targetFace rectangle.",
+        "operationId": "LargePersonGroupPerson_AddPersonFaceFromUrl",


Seems this is not consistent with LargePersonGroupPerson_Delete/Get/UpdateFace. Maybe AddFaceFromUrl? But since the conventional PersonGroup also have this problem, I think it is OK to change it in a different pull request, but maybe it is better to align them before the release of breaking change.

Fixed it in this PR since it is tiny and new PR does take longer.

huxuan · 2018-08-01T07:55:29Z

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

+      },
+      "get": {
+        "description": "List all persons in a large person group, and retrieve person information (including personId, name, userData and persistedFaceIds of registered faces of the person).",
+        "operationId": "LargePersonGroupPerson_List",


If we prefer LargeFaceList_ListFaces then how about this? Besides, we also have LargePersonGroupPerson_Create/Get/Update/Delete.

Using LargeFaceList_ListFaces is to be differentiated with LargeFaceList_List which refers to list large face lists. I guess you are thinking about the pattern of LargeFaceListFace_xx for face level operations. Yes, it's another pattern while my thought is the SDK could be more friendly to user if it uses same level as API reference, where LargeFaceList operations are all inside LargeFaceList section, without another level of LargeFaceListFace section.

Actually I am think about using LargePersonGroup_ListPersons to align with LargeFaceList_ListFaces, but I think it is OK if we keep following the structure of API reference.

annatisch · 2018-08-01T22:10:55Z

@lebronJ - LGTM
Last question - I see the operation LargePersonGroupPerson_Create returns a Person in the response, whereas the other two create operations (LargePersonGroup_Create, LargeFaceList_Create) don't. I'm guessing this is intentional?

huxuan · 2018-08-01T22:28:29Z

@annatisch, Yes, confirmed. It is compatible with our service and the reason is that the LargeFaceListId and LargePersonGroupId is specified by the customers while PersonId is auto generated by our service. So we need to return the generated PersonId to the customers but no need to return anything for LargePersonGroup and LargeFaceList creation.

annatisch · 2018-08-01T22:32:13Z

Thanks @huxuan - let me know if you are finished reviewing internally and I can merge.

jhendrixMSFT · 2018-08-01T22:40:16Z

@AutorestCI regenerate azure-sdk-for-go

huxuan · 2018-08-01T23:27:17Z

Hi @annatisch and @jhendrixMSFT, the internal review already finished before sending this pull request. Please feel free to move forward! :-)

lebronJ added 12 commits July 31, 2018 12:47

Add large person group management.

a6c0558

Add large face list management.

ef9df82

Nit

1ec4103

Add find similar with large face list.

cb6e38c

Add identify with large person group.

2206ad2

Add verify with large person group.

6abd79f

Fix typo of training status.

0cf51f4

Add missing adding face to large face list & large person group.

9abe7f4

Add missing large face list face operations.

663d5ad

Nit fix.

f344814

Rebase and update master's endpoint changes to examples.

5ed1c0c

Nit fix documentation.

9b4c21a

lebronJ requested a review from yangyuan as a code owner July 31, 2018 04:57

AutorestCI mentioned this pull request Jul 31, 2018

[AutoPR cognitiveservices/data-plane/Face] [Cogs Face] Align with latest released version of Face API with million-scale features. Azure/azure-sdk-for-python#3049

Merged

azuresdkci assigned annatisch Jul 31, 2018

azuresdkci requested a review from annatisch July 31, 2018 04:59

AutorestCI mentioned this pull request Jul 31, 2018

[AutoPR cognitiveservices/data-plane/Face] [Cogs Face] Align with latest released version of Face API with million-scale features. Azure/azure-sdk-for-java#2289

Merged

AutorestCI mentioned this pull request Jul 31, 2018

[AutoPR cognitiveservices/data-plane/Face] [Cogs Face] Align with latest released version of Face API with million-scale features. Azure/azure-sdk-for-go#2349

Closed

yangyuan approved these changes Jul 31, 2018

View reviewed changes

Nit path case refinement.

a2c9e88

annatisch reviewed Jul 31, 2018

View reviewed changes

Fix LargeFaceListFace_List to LargeFaceList_ListFaces and refine exam…

40abfc7

…ple naming.

huxuan reviewed Aug 1, 2018

View reviewed changes

lebronJ added 2 commits August 1, 2018 16:14

[Minor] Person_AddPersonFace to Person_AddFace.

6019c55

[Minor] Amend last commit of Person_AddPersonFace to Person_AddFace.

a43b96d

annatisch approved these changes Aug 1, 2018

View reviewed changes

AutorestCI mentioned this pull request Aug 1, 2018

[AutoPR cognitiveservices/data-plane/Face] [Cogs Face] Align with latest released version of Face API with million-scale features. Azure/azure-sdk-for-go#2370

Merged

huxuan approved these changes Aug 1, 2018

View reviewed changes

annatisch merged commit 603513f into Azure:master Aug 1, 2018

This was referenced Aug 2, 2018

[Cogs Face] Update Face SDK‘s auto-generated code to 3.0.0-preview with latest swagger. Azure/azure-sdk-for-net#4618

Merged

[Cogs Face] Update Face SDK test aligning with latest Swagger changes. Azure/azure-sdk-for-python#3103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cogs Face] Align with latest released version of Face API with million-scale features. #3552

[Cogs Face] Align with latest released version of Face API with million-scale features. #3552

lebronJ commented Jul 31, 2018 •

edited

Loading

azuresdkci commented Jul 31, 2018

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

yangyuan left a comment

yangyuan Jul 31, 2018

lebronJ Jul 31, 2018 •

edited

Loading

yangyuan Jul 31, 2018

lebronJ Aug 1, 2018

lebronJ commented Jul 31, 2018

annatisch Jul 31, 2018

lebronJ Aug 1, 2018

lebronJ Aug 1, 2018

huxuan Aug 1, 2018 •

edited

Loading

lebronJ Aug 1, 2018

huxuan Aug 1, 2018

lebronJ Aug 1, 2018

huxuan Aug 1, 2018

annatisch commented Aug 1, 2018

huxuan commented Aug 1, 2018

annatisch commented Aug 1, 2018

jhendrixMSFT commented Aug 1, 2018

huxuan commented Aug 1, 2018

[Cogs Face] Align with latest released version of Face API with million-scale features. #3552

[Cogs Face] Align with latest released version of Face API with million-scale features. #3552

Conversation

lebronJ commented Jul 31, 2018 • edited Loading

PR information

Quality of Swagger

azuresdkci commented Jul 31, 2018

AutorestCI commented Jul 31, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Jul 31, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Jul 31, 2018 • edited Loading

Automation for azure-sdk-for-ruby

AutorestCI commented Jul 31, 2018 • edited Loading

Automation for azure-sdk-for-java

AutorestCI commented Jul 31, 2018 • edited Loading

Automation for azure-sdk-for-go

yangyuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebronJ Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebronJ commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huxuan Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annatisch commented Aug 1, 2018

huxuan commented Aug 1, 2018

annatisch commented Aug 1, 2018

jhendrixMSFT commented Aug 1, 2018

huxuan commented Aug 1, 2018

lebronJ commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

AutorestCI commented Jul 31, 2018 •

edited

Loading

lebronJ Jul 31, 2018 •

edited

Loading

huxuan Aug 1, 2018 •

edited

Loading