Add CLU and CT Language authoring 2022-03-01-preview APIs #17033
Conversation
|
Hi, @moabba Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. [email protected] |
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
1 similar comment
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
|
Hi @moabba, Your PR has some issues. Please fix the CI sequentially by following the order of
|
| } | ||
| }, | ||
| "400": { | ||
| "description": "This error can be returned if the request's parameters are incorrect meaning the required parameters are missing, malformed, or too large.\\r\\n\\r\\nThis error can be returned if the request's body is incorrect meaning the JSON is missing, malformed, or too large." |
There was a problem hiding this comment.
All error code needs an error schema. https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#handling-errors
And it need to have x-ms-error-response. https://azure.github.io/autorest/extensions/#x-ms-error-response
Additionally it needs a "default" for other errors not covered here (e.g. 5xx).
| "description": "Creates a new or update a project.", | ||
| "operationId": "Projects_CreateOrUpdateProject", | ||
| "consumes": [ | ||
| "application/json" |
There was a problem hiding this comment.
JSON Merge Patch (RFC7396) is recommended. https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#json-resource-schema--field-mutability
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "parameters": [ |
There was a problem hiding this comment.
None of the API have api-version query parameter?
I see api-version property in payload of PUT/POST (usual approach would be query parameter). However how do you handle GET request?
| "operation-location": { | ||
| "description": "The location of the status API for monitoring the created job.", | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
What will happen is the request if accepted, but response is lost to caller due to network issue?
Also it helps to provide examples, so reviewer and tooling has some idea of your request/response. https://azure.github.io/autorest/extensions/#x-ms-examples
| "200": { | ||
| "description": "List of all deployments.", | ||
| "schema": { | ||
| "$ref": "#/definitions/DeploymentObjectList" |
There was a problem hiding this comment.
This API appear to be pagination. Please use appropriate extension https://azure.github.io/autorest/extensions/#x-ms-pageable
|
Hi, @moabba. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
ghost
added
the
There was a problem hiding this comment.
You also need to update the data-plane/readme.md file and this should contain all the files for inference. Ideally, you should create a new api-version using https://aka.ms/openapihub from the latest stable or preview version (in this case, preview).
api-versions should generally be additive, and right not the entire inference APIs are missing and code cannot be correctly generated since necessary entries are missing from the readme.md (that OpenAPI Hub would create).
moabba
changed the title
There was a problem hiding this comment.
Still no update to readme.md in the data-plane root, nor is common.json here or other swaggers that are being authored for conversation analysis. You're also missing endpoint samples needed for doc generation and validation. The latter could be done with a new tracking bug, but has to be done before we can release.
| @@ -213,9 +213,6 @@ | |||
| "post": { | |||
| "description": "Triggers a job to export project data in JSON format.", | |||
| "operationId": "Projects_TriggerExportProjectJob", | |||
| "consumes": [ | |||
| @@ -225,20 +222,16 @@ | |||
| "type": "string" | |||
| }, | |||
| { | |||
| "in": "header", | |||
| "in": "query", | |||
There was a problem hiding this comment.
The projectName above should come from common.json, which you're missing in this branch. You need to start from the existing CLU preview version in the release/cognitiveservices/clu-2021-11-01-preview branch.
| { | ||
| "in": "query", | ||
| "name": "top", | ||
| "description": "The number of utterances to be returned.", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "skip", | ||
| "description": "The offset in the response.", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "maxpagesize", | ||
| "description": "The maximum number of utterances per page.", | ||
| "type": "string" |
There was a problem hiding this comment.
These will be common parameters and should also be defined in common.json.
| "400": { | ||
| "description": "This error can be returned if the request's parameters are incorrect meaning the required parameters are missing, malformed, or too large.\\r\\n\\r\\nThis error can be returned if the request's body is incorrect meaning the JSON is missing, malformed, or too large." | ||
| }, | ||
| "401": { | ||
| "description": "You do not have access. \\r\\n\\r\\nReasons can include:\\r\\n\\r\\n* used endpoint subscription key, instead of authoring key\\r\\n* invalid, malformed, or empty authoring key\\r\\n* authoring key doesn't match region\\r\\n* you are not the owner or collaborator\\r\\n* invalid order of API calls." | ||
| }, | ||
| "403": { | ||
| "description": "Total monthly key quota limit exceeded." | ||
| }, | ||
| "404": { | ||
| "description": "The project or the job does not exist." | ||
| }, | ||
| "429": { | ||
| "description": "Rate limit is exceeded." | ||
| } |
There was a problem hiding this comment.
Do not document all errors. Just default that references common.json#/definitions/AzureError (or something like that - see the existing preview swagger for details).
| "in": "query", | ||
| "name": "projectType", | ||
| "description": "The CLU project type to get the supported languages for.", | ||
| "name": "projectKind", | ||
| "description": "The project kind to get the supported languages for.", | ||
| "required": true, | ||
| "type": "string" | ||
| "type": "string", | ||
| "enum": [ | ||
| "conversation", | ||
| "workflow" | ||
| ] |
There was a problem hiding this comment.
This parameter is also defined common.json, IIRC.
| "400": { | ||
| "description": "This error can be returned if the request's parameters are incorrect meaning the required parameters are missing, malformed, or too large.\\r\\n\\r\\nThis error can be returned if the request's body is incorrect meaning the JSON is missing, malformed, or too large." | ||
| }, | ||
| "401": { | ||
| "description": "You do not have access. \\r\\n\\r\\nReasons can include:\\r\\n\\r\\n* used endpoint subscription key, instead of authoring key\\r\\n* invalid, malformed, or empty authoring key\\r\\n* authoring key doesn't match region\\r\\n* you are not the owner or collaborator\\r\\n* invalid order of API calls." | ||
| }, | ||
| "403": { | ||
| "description": "Total monthly key quota limit exceeded." | ||
| }, | ||
| "404": { | ||
| "description": "The project does not exist." | ||
| }, | ||
| "429": { | ||
| "description": "Rate limit is exceeded." | ||
| } |
There was a problem hiding this comment.
Don't define errors explicitly. See my previous comment for details (I'll not repeat, so please correct all instances).
| ], | ||
| "type": "object", | ||
| "properties": { | ||
| "jobId": { | ||
| "description": "Gets or sets the jobId.", | ||
| "description": "Gets or sets the job id.", |
There was a problem hiding this comment.
| "description": "Gets or sets the job id.", | |
| "description": "Gets or sets the job ID.", |
| "kind": { | ||
| "description": "Represents the evaluation kind. By default, the evaluation kind is set to percentage.", | ||
| "enum": [ | ||
| "percentage", | ||
| "set" | ||
| ], | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Is this or any other enum ever returned? If so, it should use the modelAsString extension.
| "properties": { | ||
| "f1": { | ||
| "format": "double", | ||
| "description": "Represents the model precision", |
There was a problem hiding this comment.
- Make sure this represents a double from 0 to 1, as is typical across the Language pillar.
- Update the docs to say that as well.
| "$ref": "#/definitions/TrainingJobStatusResult" | ||
| }, | ||
| "jobId": { | ||
| "description": "Gets or sets the jobId.", | ||
| "description": "Gets or sets the job id.", |
There was a problem hiding this comment.
| "description": "Gets or sets the job id.", | |
| "description": "Gets or sets the job ID.", |
Check elsewhere as well, in case I missed one. ID is a pseudo-acronym and should be capitalized.
|
@heaths The state of the current PR is for API review. We'll update the swagger file to be suitable for this repo once the APIs stabilize from a review perspective. |
|
It would still be good to pull in common.json and use its definitions when defined to make sure your service actually accomodates any differences. The Language pillar is trying to consolidate around common patterns as well, and this will help detect any differences. |
|
From internal discussion, since this apparently isn't meant to be complete but only enough for a review, you should be targeting a feature branch. Azure REST guidelines are that any version in main - preview or stable - is immutable (at least for breaking changes, which even adding a property is). I recommend creating one from the current Cognitive Language release feature branch, copying over common.json, and starting from there. You don't have to start completely over, though. Assuming you're in your current topic branch for this PR: git fetch upstream release/cognitiveservices/clu-2021-11-01-preview
git rebase FETCH_HEADcommon.json should show up in 2021-11-01-preview which you can copy over into your release folder as well. |
| "running", | ||
| "succeeded", | ||
| "failed", | ||
| "cancelled", |
There was a problem hiding this comment.
Canceled, see https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#long-running-operations--jobs for example status names.
| } | ||
| }, | ||
| "dataset": { | ||
| "description": "The dataset for this utterance. Allowed values are 'Train' and 'Test'.", |
| } | ||
| } | ||
| }, | ||
| "LanguageGetObject": { |
| } | ||
| } | ||
| }, | ||
| "CreateOrReplaceDeploymentJobObject": { |
| "/projects/{projectName}/models/{trainedModelName}/evaluation/predictions": { | ||
| "get": { | ||
| "description": "Get trained model evaluation predictions.", | ||
| "operationId": "Models_GetTrainedModelEvaluationPredictions", |
There was a problem hiding this comment.
consider not adding the collection name to the operationid to ensure there are no duplicate names. This will generate quite some ugly method names in generated clients when the generator uses this id as the generated method name.
Ideally the second part of this operationid is already unique.
| "/projects/{projectName}/train/jobs": { | ||
| "get": { | ||
| "description": "Get training jobs.", | ||
| "operationId": "Training_GetTrainingJobsList", |
| }, | ||
| "403": { | ||
| "description": "Total monthly key quota limit exceeded." | ||
| }, |
There was a problem hiding this comment.
are those codes really returned when getting the job status?
| } | ||
| } | ||
| }, | ||
| "/projects/global/prebuiltEntities": { |
| "description": "Represents the model F1 score", | ||
| "type": "number" | ||
| }, | ||
| "truePositivesCount": { |
|
Hi, @moabba. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
There was a problem hiding this comment.
Where are the rest of the files like ConversationAnalysis.json - the inference APIs - in this directory? While the Language RP has been a bit of a problem having many different related-but-separate APIs, each feature like CLU needs to be complete i.e., you need both inference and authoring APIs here.
|
Hi @moabba , instead of the |
|
Hi, @moabba. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
|
Is this PR still valid? |
| } | ||
| } | ||
| }, | ||
| "DeploymentJobStatusObject": { |
There was a problem hiding this comment.
Deployment job should return a snapshot of a deployment so that users can reason what has been deployed and what model is used to do the inference. Essentially, modelId should be returned.
|
@moabba 2022-03-01-preview has been merged to main. Making these changes now would be considered breaking and require exemption. Alternatively, you could punt to the cognitiveservices-Language-2022-04-01-preview branch, or close this PR if no longer needed. |
|
Further changes will be done in this PR #18694 |
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Add a changelog entry for this PR by answering the following questions:
February 2022.
Mid-January 2022.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Otherwise your PR may be subject to ARM review requirements. Complete the following:
Check this box if any of the following apply to the PR so that label "WaitForARMFeedback" will be added automatically to begin ARM API Review. Failure to comply may result in delays to the manifest.
-[ ] To review changes efficiently, ensure you copy the existing version into the new directory structure for first commit and then push new changes, including version updates, in separate commits.
Ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If any of the following scenarios apply to the PR, request approval from the Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.