search

Azure / azure-rest-api-specs

The source for REST API specifications for Microsoft Azure.
MIT License
2.69k stars 5.12k forks source link

[Unified Vision Service ] API Review #27498

Open azure-sdk opened 10 months ago

azure-sdk commented 10 months ago

New API Review meeting has been requested.

Service Name: Unified Vision Service Review Created By: Kailash Joshi Review Date: 02/15/2024 04:00 PM PT Onboarding Record: PR: Hero Scenarios Link: Not Provided Core Concepts Doc Link: Not Provided

Description: Search/Ingestion API for computer vision

Detailed meeting information and documents provided can be accessed here For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

heaths commented 10 months ago

Is there a PR available yet for us to review?

azure-sdk commented 9 months ago

Meeting updated by Kailash Joshi

Service Name: Unified Vision Service Review Created By: Kailash Joshi Review Date: 02/15/2024 04:00 PM PT Onboarding Record: PR: https://github.com/Azure/azure-rest-api-specs/pull/27768 Hero Scenarios Link: Not Provided Core Concepts Doc Link: Not Provided

Description: Search/Ingestion API for computer vision

Detailed meeting information and documents provided can be accessed here For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

mikekistler commented 9 months ago

Notes from API Review meeting 2/15/24

We should probably make another pass on this API once the above issues are addressed (as many as possible)

MinjingZhu commented 9 months ago

Thank for the notes Mike! Just want to make a small correction to it: Skip and top are still needed for paging, but $ will be removed.

mikekistler commented 9 months ago

From @MinjingZhu in Teams:

Hi Mike/Jeffrey, below is a follow-up for the open issues we identified during the meeting. The list is based on Mike's notes.

The LRO for creating ingestion

  • I confirmed with my team that ingestions are designed to be resources created in the database, and ingestions cannot be updated once created, so I believe in this case PUT is the correct action to use.

Property naming convention

  • We will fix the following to align with the guideline:
    • state property should be called status
    • Date-times should use "At" suffix and should use millisecond precision (not greater)
    • Don't use $ in any parameter names

Header request-id should maybe be x-ms-request-id

  • Headers are managed by the APIM layer, owned by the platform team. The header value seems to be the same for all cognitive services. We don't have control over it.

TypeSpec

  • For this and next public preview, we won't need the SDK. Before GA, we will investigate how to auto-generate the TypeSpec from the code. Mike/Jeffrey, do you happen to have any pointers for this?

Drop PUT on /retrieval/indexes/{indexName} -- just support PATCH

  • The intent behind separating the Create and Update APIs was to make it more explicit that some properties in the indexes are updatable, some are not. The two APIs have different request bodies, so it's less likely to be mistaken.

Response schema of PATCH should be the same as the request body

  • The request body of the PATCH is only a subset of index properties that is modifiable, while the response body includes all properties of an index. Would this justify the difference?

Patch content type should be "application/merge-patch+json"

  • Currently the swagger is auto-generated based on our C# code, and we didn't explicitly set the content type in the code. I'll investigate how to fix it.

Don't return eTag if you don't support conditional requests

  • Darren confirmed that we do support conditional requests. Sorry I wasn't very familiar with this part.

How does a customer deal with failure in an ingestion?

  • We have enabled users to check the ingestion status of each document, in additional to the status of the entire ingestion. Users can choose different detail levels, e.g. failed, succeeded, all, none. If they turn on the detailed logging, they will be able to know which ones need to be re-ingested.
  • The ingestion might fail due to various regions. For example, user might input an invalid video URL or the video type is not supported. In this case, users need to correct the request body before submitting a new ingestion.

Please let us know how to proceed the review. Do we still need another meeting or is it ok to use this channel to discuss?

mikekistler commented 9 months ago

@MinjingZhu please make the changes above and then tag us to re-review. We'll be able to assess whether a new meeting is needed once we see the updates.

mikekistler commented 9 months ago

Also ... a comment on the directory structure. Since this is a new service that is distinct from UnifiedVision, it should have its own directory under cognitiveservices/data-plane -- not a subdirectory within UnifiedVision. I recommend cognitiveservices/data-plane/UnifiedVisionSearch.

MinjingZhu commented 9 months ago

Hi @mikekistler, I just updated the PR with changes to the property names and the directory structure. Could you please help review it? Thanks!

mikekistler commented 9 months ago

@MinjingZhu Directory structure looks right now -- thanks. And I don't see any more property name issues, so that's good.

But there are still a number of basic problems with the API. I know you want to defer moving to TypeSpec but most if not all of these problems would be fixed if you adopted TypeSpec and the Azure TypeSpec libraries. Deferring the move to TypeSpec creates work in the near term to address these problems that could be completely avoided by moving to TypeSpec.

Resources to help you get started with TypeSpec: