DO NOT MERGE. BatchAI. Specification for new 2018-05-01 API version #2896

AlexanderYukhanov · 2018-04-18T16:10:45Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-04-18T16:11:41Z

Automation for azure-libraries-for-java

The initial PR has been merged into your service PR:
AutorestCI/azure-libraries-for-java#89

AutorestCI · 2018-04-18T16:11:43Z

Automation for azure-sdk-for-python

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

AutorestCI · 2018-04-18T16:11:45Z

Automation for azure-sdk-for-node

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

AutorestCI · 2018-04-18T16:12:44Z

Automation for azure-sdk-for-go

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-go#1718

azuresdkciprbot · 2018-04-18T16:17:50Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/batchai/resource-manager/readme.md
Before the PR: Warning(s): 7 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

dsgouda · 2018-04-18T17:12:50Z

@AlexanderYukhanov Let me know when this is ready for review

Changes: - Added first class support for Horovod framework - Added first class support for custom mpi commands - Introduced workspaces - Introduces experiments as a grouping mechanism for related jobs - Moved jobs from the top level into workpace/experiment - Moved clusters and file servers from the top level into workspaces - Removing filter and select parameters as they are not suppported yet - Removing FileServer type as only NFS is supporter - Removed type of output directories - server doesn't support it - Removed 'createNew' attribute of output directory because there is no use-case scenario for having it equal to false.

azuresdkciprbot · 2018-04-18T17:24:14Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

File: specification/batchai/resource-manager/readme.md
Before the PR: Warning(s): 7 Error(s): 0
After the PR: Warning(s): 8 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

AlexanderYukhanov · 2018-04-18T17:31:47Z

Ready for review

dsgouda

Left a few comments

dsgouda · 2018-04-18T20:47:56Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+          ],
+          "x-ms-enum": {
+            "name": "UsageUnit",
+            "modelAsString": false


Setting this as false can cause breaking changes if the enum values change

fixed here and in other appropriate places

dsgouda · 2018-04-18T20:48:16Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+        "nextLink": {
+          "type": "string",
+          "description": "The URI to fetch the next page of compute resource usage information. Call ListNext() with this to fetch the next page of compute resource usage information."
+        }


Please mark this as readonly

dsgouda · 2018-04-18T20:48:32Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+    },
+    "Usage": {
+      "properties": {
+        "unit": {


Please check which of these values are readonly

Usages do not have a PUT. This is a GET only API. Do we still need to mark every property explicitly as readonly?

When we set readonly to true, the setter becomes private in generated model (which means no one can externally set this value, only deserialization process will do that).
tl;dr for better quality generated code, it would good, but won't block on this

sure, in this case we will update

on the other hand. what if a user wants to write unit tests for his code? he needs to be able to set values.
I remember how hard I hated google for similar limitations in android. let's do not do it?

If testing alone is the concern why not simply deserialize a production-like json representation.
Again, this is a decision left to you. Let me know what direction you'd like to take and I shall approve the PR

@pradeepgunda please vote

decided to go with readonly

dsgouda · 2018-04-18T20:49:17Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+        "nextLink": {
+          "type": "string",
+          "description": "The continuation token."
+        }


Please mark all nextLink as readonly

dsgouda · 2018-04-18T20:49:39Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+          ],
+          "x-ms-enum": {
+            "name": "CachingType",
+            "modelAsString": false


Same comment as before for modelAsString

done in other places, but not here. it's storage premium disk caching type and it seems to be a pretty stable enum

dsgouda · 2018-04-18T21:27:09Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+          "$ref": "#/definitions/ImageReference"
+        }
+      },
+      "description": "Setting


By default,. C# has the exact same structure for CloudError which is set as the Body property in CloudException. If this was the intention here, it would be easier to simply not specify the error model for the operations .

The reason we did not use CloudError is because we will be adding more properties to Job and Cluster errors. for e.g. category -- userError, serverError. Also we will be returning collection of errors.

pradeepgunda · 2018-04-19T22:50:08Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+          "$ref": "#/definitions/ImageReference"
+        }
+      },
+      "description": "Setting


can you add CreationTime

pradeepgunda · 2018-04-19T22:50:23Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+          "$ref": "#/definitions/ImageReference"
+        }
+      },
+      "description": "Setting


add creationtime

pradeepgunda · 2018-04-19T23:26:56Z

specification/batchai/resource-manager/Microsoft.BatchAI/stable/2018-05-01/BatchAI.json

+          "$ref": "#/definitions/ImageReference"
+        }
+      },
+      "description": "Setting


should we call this priorityLevel?

after discussion, decided to have schedulingPriority

dsgouda · 2018-04-20T21:00:39Z

Also,
Please address the validation errors/warnings here

…pplicable

azuresdkciprbot · 2018-04-20T23:28:37Z

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/batchai/resource-manager/readme.md

⚠️

0 new Warnings.(0 total)

❌0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2018-04-20T23:28:37Z

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/batchai/resource-manager/readme.md

⚠️

5 new Warnings.(5 total)

Code	Id	Source	Message
PutRequestResponseScheme	R2017	Link	A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'Workspaces_Create' Request Model: 'WorkspaceCreateParameters' Response Model: 'Workspace'
PutRequestResponseScheme	R2017	Link	A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'Experiments_Create' Request Model: 'ExperimentCreateParameters' Response Model: 'Experiment'
PutRequestResponseScheme	R2017	Link	A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'Jobs_Create' Request Model: 'JobCreateParameters' Response Model: 'Job'
PutRequestResponseScheme	R2017	Link	A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'FileServers_Create' Request Model: 'FileServerCreateParameters' Response Model: 'FileServer'
PutRequestResponseScheme	R2017	Link	A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'Clusters_Create' Request Model: 'ClusterCreateParameters' Response Model: 'Cluster'

❌0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

dsgouda

LGTM will merge when ready and CI pass

AlexanderYukhanov · 2018-04-26T19:26:32Z

Should we request the ARM approval? Can you please add the corresponding label?

azuresdkci assigned dsgouda Apr 18, 2018

azuresdkci requested a review from dsgouda April 18, 2018 16:12

AlexanderYukhanov changed the title ~~BatchAI. Specification for new 2018-05-01 API version~~ DO NOT MERGE. BatchAI. Specification for new 2018-05-01 API version Apr 18, 2018

dsgouda added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Apr 18, 2018

dsgouda suggested changes Apr 18, 2018

View reviewed changes

pradeepgunda reviewed Apr 19, 2018

View reviewed changes

Addressed CR feedback

ad43bbe

BatchAI. Get rid of booleans and added readonly attributes to where a…

1ed7093

…pplicable

dsgouda approved these changes Apr 20, 2018

View reviewed changes

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Apr 26, 2018

dsgouda removed ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review DoNotMerge <valid label in PR review process> use to hold merge after approval labels Apr 27, 2018

dsgouda merged commit 5f40c8e into Azure:master Apr 27, 2018

AutorestCI mentioned this pull request Apr 27, 2018

[AutoPR batchai/resource-manager] DO NOT MERGE. BatchAI. Specification for new 2018-05-01 API version Azure/azure-sdk-for-go#1717

Merged

DO NOT MERGE. BatchAI. Specification for new 2018-05-01 API version #2896

DO NOT MERGE. BatchAI. Specification for new 2018-05-01 API version #2896

Conversation

AlexanderYukhanov commented Apr 18, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

AutorestCI commented Apr 18, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented Apr 18, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Apr 18, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Apr 18, 2018 • edited Loading

Automation for azure-sdk-for-go

azuresdkciprbot commented Apr 18, 2018

dsgouda commented Apr 18, 2018

azuresdkciprbot commented Apr 18, 2018

AlexanderYukhanov commented Apr 18, 2018

dsgouda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderYukhanov Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgouda commented Apr 20, 2018

azuresdkciprbot commented Apr 20, 2018

AutoRest linter results for ARM Related Validation Errors/Warnings

azuresdkciprbot commented Apr 20, 2018

AutoRest linter results for SDK Related Validation Errors/Warnings

dsgouda left a comment

Choose a reason for hiding this comment

AlexanderYukhanov commented Apr 26, 2018

AlexanderYukhanov commented Apr 18, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

AlexanderYukhanov Apr 20, 2018 •

edited

Loading