Skip to content

[Cogs Face] Align with latest released version of Face API with feature of multiple recognition models. #5381

Merged
veronicagg merged 24 commits intoAzure:masterfrom
acured:AddRecognitionModelType
Mar 25, 2019
Merged

[Cogs Face] Align with latest released version of Face API with feature of multiple recognition models. #5381
veronicagg merged 24 commits intoAzure:masterfrom
acured:AddRecognitionModelType

Conversation

@lebronJ
Copy link
Contributor

@lebronJ lebronJ commented Mar 14, 2019 •
edited
Loading

(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 Multiple Recognition Model features. (rollout will be started as soon as we get final approval from API review side)

Note: This PR has

  1. Added a parameter to existing API interfaces.
  2. Added a new field in API response json schema.

I understand this will be a breaking change from SDK perspective while at least this will not affect existing SDK users before they upgrade to latest SDK version. We have asked an exception from Azure API Review Board for this.

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

Your Name and others added 21 commits November 13, 2018 16:12
add RecognitionModelType
e146d53
remove generated files
0892694
Rename RecognitionModelType to RecognitionMode, Indents replaced tab …
babb2d2 
…to spaces, Add new definitions named MetaDataContract inheriting from NameAndUserDataContract
my comment
bb08a55
Add recognitionModel as parameter in detect operation. Fixed some str…
6940d93 
…ing format issue.
Removed generated files.
6f2ae67
Change RecognitionModel required to false.
193808f
Fixed generate SDK issue when set required to false, need change 'in'…
b127b27 
… from 'path' to 'query'
fix conflict
ec461a6
Merge branch 'master' into AddRecognitionModelType
d5c7226
fix conflict error
538aa8a
change recognition mode to enum
f11b872
update examples.
760cf89
Rename RecognitionModel to recognitionModel in request parameter.
52516fd
update 'modelAsString' to true in RecognitionModel for future expansion.
d50105d
update description.
b0e7e0e
Remove duplicate key in 4193
c7a8581
Add link and remove redundant br tag.
675acfa
Add br tag for line break.
d9c5b46
Change RecognitionModel to recognitionModel in parameter. Change Reco…
497a576 
…gnitionModel description.
rename recognition_v01(2) to recognition_01(2)
f46597d
@AutorestCI AutorestCI mentioned this pull request Mar 14, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Mar 14, 2019 •
edited
Loading

Automation for azure-sdk-for-python

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

@AutorestCI AutorestCI mentioned this pull request Mar 14, 2019
Closed
@AutorestCI
Copy link

AutorestCI commented Mar 14, 2019 •
edited
Loading

Automation for azure-sdk-for-js

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-js#1772

@lebronJ
Copy link
Contributor Author

lebronJ commented Mar 14, 2019

@johan can help explain more on this and I would appreciate if you could kindly offer us a sign-off from Azure API review's perspective, thanks.

@AutorestCI AutorestCI mentioned this pull request Mar 14, 2019
Closed
@AutorestCI
Copy link

AutorestCI commented Mar 14, 2019 •
edited
Loading

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#4998

@lebronJ
Copy link
Contributor Author

lebronJ commented Mar 15, 2019

@lebronJ thanks a lot for the detailed comments at the top, since they answered several of my questions from the start :) the change looks good to me. I will add the potential-breaking-changes label so SDK owners know. Are the API changes deployed by now? If so, I can approve and merge this change.

@veronicagg Thanks a lot for your quick review while our asking for exception to Azure API review is still on-going. Please hold on this a little bit and will let you know when we reach agreement on merging this.

nschonni
nschonni reviewed Mar 15, 2019
View reviewed changes
adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Mar 18, 2019
@adxsdknet
.NET SDK Resource Provider:'Face'
3cbb87d 
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit
@yangyuan
Copy link
Member

yangyuan commented Mar 19, 2019

This involves API design changes. Have you finished the API design review?

@lebronJ
Copy link
Contributor Author

lebronJ commented Mar 20, 2019

Hi @yangyuan , could you please first add a Do-Not-Merge label for this PR?

Actually we are now asking the API review board for exception and we are also applying some API interface changes for service side now upon the review comments. There will be changes for the Swagger after we finalize the implementation for service side and get review sign-off.

Will ping back this PR then.

This was referenced Mar 20, 2019
Closed
Closed
adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Mar 20, 2019
@adxsdknet
.NET SDK Resource Provider:'Face'
38f35bf 
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit
@veronicagg veronicagg added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Mar 20, 2019
@veronicagg
Copy link
Contributor

veronicagg commented Mar 20, 2019

@lebronJ would you prefer to close this PR and re-open when you're ready? you can tag me back when ready. Thanks!

@veronicagg veronicagg changed the title [Cogs Face] Align with latest released version of Face API with feature of multiple recognition models. [Do not merge] [Cogs Face] Align with latest released version of Face API with feature of multiple recognition models. Mar 20, 2019
lebronJ
lebronJ commented Mar 21, 2019
View reviewed changes
This was referenced Mar 21, 2019
Closed
Closed
adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Mar 21, 2019
@adxsdknet
.NET SDK Resource Provider:'Face'
347dca2 
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit
@lebronJ
Copy link
Contributor Author

lebronJ commented Mar 24, 2019

@lebronJ would you prefer to close this PR and re-open when you're ready? you can tag me back when ready. Thanks!

Hi @veronicagg and @yangyuan, we have gotten the API review sign-off and I have forwarded it to Cognitive Services Experience Team and you two. Can we move forward to review and merge this as soon as possible (thanks), so that we can catch the dates of our roll out, along with the API reference updates, SDKs and samples?

@lebronJ
Copy link
Contributor Author

lebronJ commented Mar 24, 2019

@lebronJ would you prefer to close this PR and re-open when you're ready? you can tag me back when ready. Thanks!

Hi @veronicagg and @yangyuan, we have gotten the API review sign-off and I have forwarded it to Cognitive Services Experience Team and you two. Can we move forward to review and merge this as soon as possible (thanks), so that we can catch the dates of our roll out, along with the API reference updates, SDKs and samples?

Besides, could anyone please help remove the DoNotMerge label and reopen the PR of SDK changes? Let me know if any action needed from my side.

@veronicagg veronicagg changed the title [Do not merge] [Cogs Face] Align with latest released version of Face API with feature of multiple recognition models. [Cogs Face] Align with latest released version of Face API with feature of multiple recognition models. Mar 25, 2019
veronicagg
veronicagg approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the above, the specs submitted reflect your APIs which have been deployed, so I'm approving and merging. To request SDKs releases, please use https://portal.azure-devex-tools.com/app/tools/request-api-release . Thanks!

@veronicagg veronicagg merged commit 4205daa into Azure:master Mar 25, 2019
@AutorestCI AutorestCI mentioned this pull request Mar 25, 2019
Merged
@lebronJ lebronJ mentioned this pull request Mar 26, 2019
Merged
dsgouda pushed a commit to Azure/azure-sdk-for-net that referenced this pull request Mar 26, 2019
@adxsdknet @dsgouda
AutoPr-Face-lebronJ-REST Spec PrNumber 5381 (#5457)
e70a784 
* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit

* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit

* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit

* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit
@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Mar 29, 2019

@AutorestCI regenerate azure-sdk-for-go

mentat9 pushed a commit to mentat9/azure-sdk-for-net that referenced this pull request Jun 10, 2019
@adxsdknet @mentat9
AutoPr-Face-lebronJ-REST Spec PrNumber 5381 (Azure#5457)
3cd2698 
* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit

* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit

* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit

* .NET SDK Resource Provider:'Face'
REST Spec PR 'Azure/azure-rest-api-specs#5381'
REST Spec PR Author 'lebronJ'
REST Spec PR Last commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yangyuan yangyuan yangyuan approved these changes

@DavidLiCIG DavidLiCIG Awaiting requested review from DavidLiCIG

+3 more reviewers

@nschonni nschonni nschonni left review comments

@acured acured acured left review comments

@veronicagg veronicagg veronicagg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@veronicagg veronicagg

Labels

DoNotMerge <valid label in PR review process> use to hold merge after approval potential-sdk-breaking-change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@lebronJ @AutorestCI @azuresdkci @adxsdknet @veronicagg @yangyuan @jhendrixMSFT @nschonni @acured

Comments