Review request for Microsoft.MachineLearningServices to add version preview/2022-01-01-preview #17079
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, @superkevin813 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 Generation Artifacts
|
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
|
Hi, @superkevin813 your PR are labelled with WaitForARMFeedback. A notification email will be sent out shortly afterwards to notify ARM review board([email protected]). |
|
Hi @superkevin813, Your PR has some issues. Please fix the CI sequentially by following the order of
|
This was referenced Dec 14, 2021
|
LintDiff and ModelValidation error cause by existing API version copied over. Not related to new preview API (Only adding five new properties). Please suggest if need to fix those existing errors. |
ARM recommends string enums over boolean. Generally enums make better properties than booleans. They are more descriptive, flexible and future-proof while being easier for customers to discover, understand and use. Best option is to choose a better property (e.g. deleteType: { "Permanent", "Soft" }). Next best option if you really want a True/False value is to use a string enum "softDeleteEnabled": { "True", "False" }. Enums allow adding values later without requiring a new API version. In reply to: 994009474 In reply to: 994009474 Refers to: specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/preview/2022-01-01-preview/machineLearningServices.json:2096 in 960e39a. [](commit_id = 960e39a, deletion_comment = False) |
This sounds like an action rather than a property. Actions should be modeled as a POST action. In reply to: 994009559 In reply to: 994009559 In reply to: 994009559 Refers to: specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/preview/2022-01-01-preview/machineLearningServices.json:2101 in 960e39a. [](commit_id = 960e39a, deletion_comment = False) |
Recommend ResourceId instead of ArmId for these three property names. Refers to: specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/preview/2022-01-01-preview/machineLearningServices.json:4504 in 960e39a. [](commit_id = 960e39a, deletion_comment = False) |
Please spell out this acronym (or just remove it). In reply to: 994009813 Refers to: specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/preview/2022-01-01-preview/machineLearningServices.json:4505 in 960e39a. [](commit_id = 960e39a, deletion_comment = False) |
mentat9
added
the
ARMChangesRequested
|
@mentat9 thanks for the review. I have addressed your comments. |
mentat9
added
ARMSignedOff
|
@ArcturusZhang can I have a sign off from you? Is there still anything blocking please let me know. he LintDiff and ModelValidation is an existing issue from previous stable version. We will address them in the next stable version |
ArcturusZhang
requested changes
Jan 5, 2022
| { | ||
| "$ref": "#/definitions/Compute" | ||
| }, | ||
| { |
There was a problem hiding this comment.
Please do not use anonymous schema here, which usually generates bad SDK code.
Also please do not use multi-inheritance, which usually generates bad SDK code as well
Comment on lines
+3285
to
+3306
| "Kubernetes": { | ||
| "description": "A Machine Learning compute based on Kubernetes Compute.", | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/definitions/Compute" | ||
| }, | ||
| { | ||
| "$ref": "#/definitions/KubernetesSchema" | ||
| } | ||
| ], | ||
| "x-ms-discriminator-value": "Kubernetes" | ||
| }, | ||
| "KubernetesSchema": { | ||
| "type": "object", | ||
| "description": "Kubernetes Compute Schema", | ||
| "properties": { | ||
| "properties": { | ||
| "description": "Properties of Kubernetes", | ||
| "$ref": "#/definitions/KubernetesProperties" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Please combine these two schemas instead of using allOf.
Comment on lines
+3479
to
+3495
| "AmlCompute": { | ||
| "description": "An Azure Machine Learning compute.", | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/definitions/Compute" | ||
| }, | ||
| { | ||
| "type": "object", | ||
| "description": "Properties(top level) of AmlCompute", | ||
| "properties": { | ||
| "properties": { | ||
| "description": "Properties of AmlCompute", | ||
| "$ref": "#/definitions/AmlComputeProperties" | ||
| } | ||
| } | ||
| } | ||
| ], |
There was a problem hiding this comment.
Same, please do not use anonymous schema and multi-inheritance
| { | ||
| "$ref": "#/definitions/Compute" | ||
| }, | ||
| { |
There was a problem hiding this comment.
Same, multi-inheritance and anonymous schema
| { | ||
| "$ref": "#/definitions/Compute" | ||
| }, | ||
| { |
There was a problem hiding this comment.
Same, multi-inheritance and anonymous schema
| { | ||
| "$ref": "#/definitions/Compute" | ||
| }, | ||
| { |
There was a problem hiding this comment.
Same, multi-inheritance and anonymous schema
| { | ||
| "$ref": "#/definitions/ComputeSecrets" | ||
| }, | ||
| { |
There was a problem hiding this comment.
Same, multi-inheritance and anonymous schema
| "allOf": [ | ||
| { | ||
| "$ref": "#/definitions/ComputeSecrets" | ||
| }, |
| { | ||
| "$ref": "#/definitions/ComputeSecrets" | ||
| }, | ||
| { |
| { | ||
| "$ref": "../../../../../common-types/resource-management/v2/types.json#/definitions/Resource" | ||
| }, | ||
| { |
There was a problem hiding this comment.
Same, multi-inheritance and anonymous schema
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ArcturusZhang
approved these changes
Jan 5, 2022
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This was referenced Jan 6, 2022
LeiWang3
pushed a commit
to LeiWang3/azure-rest-api-specs
that referenced
this pull request
Mar 31, 2022
…review/2022-01-01-preview (Azure#17079) * Copy latest version 2021-07-01 to new version 2022-01-01-preview without change * apply changes for new preview API * remove unrelated changes to new preview API * address comments * fix name spell Co-authored-by: Xi Jin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.
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.