Skip to content
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

Add AI Face next GA version #31023

Merged
merged 24 commits into from
Jan 18, 2025
Merged

Add AI Face next GA version #31023

merged 24 commits into from
Jan 18, 2025

Conversation

Han-msft
Copy link
Member

@Han-msft Han-msft commented Oct 15, 2024

Data Plane API Specification Update Pull Request

Tip

Overwhelmed by all this guidance? See the Getting help section at the bottom of this PR description.

  • Add new api version for face
    • Exclude Person Directory, which is not ready for GA.

PR review workflow diagram

Please understand this diagram before proceeding. It explains how to get your PR approved & merged.

spec_pr_review_workflow_diagram

API Info: The Basics

Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.

  • Link to API Spec engagement record issue:

Is this review for (select one):

  • a private preview
  • a public preview
  • GA release

Change Scope

This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.

  • Design Document:
  • Previous API Spec Doc:
  • Updated paths:

Viewing API changes

For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.

Suppressing failures

If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
Swagger-Suppression-Process
to get approval.

❔Got questions? Need additional info?? We are here to help!

Contact us!

The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.

Click here for links to tools, specs, guidelines & other good stuff

Tooling

Guidelines & Specifications

Helpful Links

Getting help

  • First, please carefully read through this PR description, from top to bottom.
  • If you don't have permissions to remove or add labels to the PR, request write access per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositories
  • To understand what you must do next to merge this PR, see the Next Steps to Merge comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.
  • For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure
    and https://aka.ms/ci-fix.
  • If the PR CI checks appear to be stuck in queued state, please add a comment with contents /azp run.
    This should result in a new comment denoting a PR validation pipeline has started and the checks should be updated after few minutes.
  • If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.

fix #31108

@Han-msft Han-msft requested a review from a team as a code owner October 15, 2024 01:52
@Han-msft Han-msft requested review from tg-msft and weidongxu-microsoft and removed request for a team October 15, 2024 01:52
Copy link

openapi-pipeline-app bot commented Oct 15, 2024

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

@mikekistler
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mikekistler
Copy link
Member

Breaking changes previously reviewed and approved in #29781

@mikekistler mikekistler added the BreakingChange-Approved-Previously Changes were reviewed and approved in a previous PR label Oct 22, 2024
Copy link
Member

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@@ -166,7 +166,7 @@ union ImageType {
}

@doc("The liveness classification for target face.")
model LivenessOutputsTarget {
model LivenessDecisionTarget {
Copy link
Member

Choose a reason for hiding this comment

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

This is a retro breaking change. Shouldn't you use @rename versioning decorator?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it's okay since it's mostly only affecting documentation. We are not changing the actual name inside payload.
BTW, this is some feedback from C# SDK review, but I think it worth to change it at REST level for better naming.

Copy link
Member

Choose a reason for hiding this comment

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

@rename should work for intention to change it going forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's wait for the final design, we may need to rebuild a lot of models after the change.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Nov 18, 2024

API change check

APIView has identified API level changes in this PR and created following API reviews.

Common

Copy link
Member

@der3318 der3318 left a comment

Choose a reason for hiding this comment

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

applying the patch: #31205 for v1.2

Copy link
Member

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

This is very close -- just need to get the form data parameters all listed separately.

Comment on lines 477 to 483
{
"name": "Parameters",
"in": "formData",
"description": "The parameters for creating session.",
"required": true,
"type": "string"
},
Copy link
Member

Choose a reason for hiding this comment

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

I think what you want here is the properties of CreateLivenessWithVerifySessionJsonContent all listed as separate formData parameters. I'm not sure what you have to do in TypeSpec to get that to happen but hopefully @allenjzhang can help you get that figured out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean expanding each field in CreateLivenessWithVerifySessionJsonContent as a part in MFD?
Something looks like

        "parameters": [
          {
            "name": "livenessOperationMode",
            "in": "formData",
            "description": "Type of liveness mode the client should follow.",
            "required": true,
            "type": "string"
          },
          {
            "name": "deviceCorrelationIdSetInClient",
            "in": "formData",
            "description": "Type of liveness mode the client should follow.",
            "required": true,
            "type": "bool"
          },
...
          {
            "name": "VerifyImage",
            "in": "formData",
            "description": "The image stream for verify. Content-Disposition header field for this part must have filename.",
            "required": true,
            "type": "file"
          }
        ],

This would be different from the current design. We put all fields inside json and serialize them into a string, and put them inside one part in MFD. I think there is no way to describe current design schema completely in swagger 2.0.

Copy link
Member

Choose a reason for hiding this comment

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

That is what I meant, and when you say

We put all fields inside json and serialize them into a string

I think you mean "We require the user to put all the fields into an object and serialize that to JSON". AND the REST API gives no indication of this or what fields belong there. So a user trying to use the REST API would be completely stymied.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense to me.
BTW, should we also change the name "VerifyImage" to camelCase?

Copy link
Member

Choose a reason for hiding this comment

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

@mikekistler Following up on Han's question, should we use camel-case or pascal-case for the parameter names? i.e. verifyImage or VerifyImage? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Camel case is probably best, as that is what we recommend for JSON. But we don't have formal guidance on field names in multipart/form-data so I think there is wiggle room here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

Thanks for all the fixes!

@mikekistler mikekistler added the APIStewardshipBoard-SignedOff The Azure API Stewardship team has reviewed and approved the changes. label Dec 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity There has been no recent activity on this issue. label Dec 23, 2024
@Han-msft Han-msft removed the no-recent-activity There has been no recent activity on this issue. label Dec 24, 2024
@Han-msft
Copy link
Member Author

Waiting for service deployment, PR will be merged when all the backend work are ready.

@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity There has been no recent activity on this issue. label Jan 13, 2025
@Han-msft Han-msft added PublishToCustomers Acknowledgement the changes will be published to Azure customers. and removed no-recent-activity There has been no recent activity on this issue. labels Jan 13, 2025
@Han-msft Han-msft enabled auto-merge (squash) January 18, 2025 00:14
@Han-msft Han-msft removed the request for review from tg-msft January 18, 2025 00:15
@Han-msft
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Han-msft Han-msft merged commit 24d856b into Azure:main Jan 18, 2025
26 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIStewardshipBoard-SignedOff The Azure API Stewardship team has reviewed and approved the changes. BreakingChange-Approved-Previously Changes were reviewed and approved in a previous PR BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required data-plane new-api-version PublishToCustomers Acknowledgement the changes will be published to Azure customers. TypeSpec Authored with TypeSpec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cognitive Services Face API - Azure AI Vision Face API] API Review
8 participants