MachineLearningServices Preview Job API [Do Not Merge] #11073
Closed
Conversation
This file contains hidden or 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
j-so
added
the
DoNotMerge
[Staging] Swagger Validation Report
❌ |
| Rule | Message |
|---|---|
|
The new version adds a response code 'default'. New: Microsoft.MachineLearningServices/preview/2020-09-01-preview/machineLearningServices.json#L595:11 |
️❌LintDiff: 1 Errors, 0 Warnings [Detail] [Expand]
❌| Rule | Message |
|---|---|
|
"details":" "Channel": "fatal", "Text": "swagger-document/compose - FAILED" "Channel": "fatal", "Text": "Error: The 'title' across provided OpenAPI definitions has to match. Found: 'Azure Machine Learning ManagementFrontEnd Service', 'Azure Machine Learning Workspaces'. Please adjust or provide an override (--title=...)."(node:5625) UnhandledPromiseRejectionWarning: Error: The 'title' across provided OpenAPI definitions has to match. Found: 'Azure Machine Learning ManagementFrontEnd Service', 'Azure Machine Learning Workspaces'. Please adjust or provide an override (--title=...). at Object.ComposeSwaggers (/home/vsts/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist/lib/pipeline/swagger-loader.js:308:15)", "location":"https://github.com/Azure/azure-rest-api-specs/blob/76b281a8e853b5c55843dc394f6090d2cd5012ad/specification/machinelearningservices/resource-manager/readme.md" |
️️✔️Avocado [Detail]
️✔️Validation passes for Avocado.
️️✔️ModelValidation [Detail]
️✔️Validation passes for ModelValidation.
️️✔️SemanticValidation [Detail]
️✔️Validation passes for SemanticValidation.
|
Azure Pipelines successfully started running 1 pipeline(s). |
j-so
added
the
WaitForARMFeedback
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure CLI Extension Generation
|
azure-sdk-for-java
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-go
|
azure-sdk-for-js
|
azure-sdk-for-net
|
azure-sdk-for-python
- Breaking Change detected in SDK
|
azure-resource-manager-schemas
|
Trenton Generation
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
|
Azure Pipelines successfully started running 1 pipeline(s). |
This was referenced Oct 7, 2020
|
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-python-track2
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
No pipelines are associated with this pull request. |
|
No pipelines are associated with this pull request. |
chiragg4u
reviewed
Oct 9, 2020
| "codeArtifactId": "some code artifact", | ||
| "command": [ | ||
| "python", | ||
| "train.py", |
Contributor
There was a problem hiding this comment.
train.py", [](start = 13, length = 10)
How are these scripts put in the workspace?
Contributor
Author
There was a problem hiding this comment.
CodeArtifactId refers to a CodeVersion: #10876
CodeVersion manages files (via blob url). You can create a CodeVersion as a blob file or blob directory. In the Job, you can reference these files.
chiragg4u
reviewed
Oct 9, 2020
| "dataBindings": { | ||
| "data binding 1": { | ||
| "sourceDataReference": "some data reference 1", | ||
| "localReference": "./", |
Contributor
There was a problem hiding this comment.
localReference [](start = 13, length = 14)
how is the data being referred?
Contributor
Author
There was a problem hiding this comment.
Similar to above (#11073 (comment)), sourceDataReference refers to a DataVersion. localReference is the path in the DataVersion (#11043)
chiragg4u
reviewed
Oct 9, 2020
| "additionalProp1": "string", | ||
| "additionalProp2": "string", | ||
| "additionalProp3": "string" | ||
| }, |
Contributor
There was a problem hiding this comment.
what are these tags? Objects already have top level tags.
Contributor
Author
There was a problem hiding this comment.
This is a proxy resource (which do not have top level tags/properties). Will reach out via email to discuss (we've had a similar discussion before #10421 (comment)).
chiragg4u
reviewed
Oct 9, 2020
...ft.MachineLearningServices/preview/2020-09-01-preview/examples/createOrUpdateCommandJob.json
Outdated
Show resolved
Hide resolved
chiragg4u
reviewed
Oct 9, 2020
| }, | ||
| "paths": { | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.MachineLearningServices/workspaces/{workspaceName}/jobs/{id}": { | ||
| "put": { |
Contributor
There was a problem hiding this comment.
Does put starts the job also? What if job fails and needs to be restarted how will it work?
Have you think about the pattern of JOB creation independent of job operations like start/stop?
There was a problem hiding this comment.
yes, PUT starts job also, if job fails user can submit new job with same inputs.
for machine learning tracking lineage is really important so we dont allow update on most of inputs in jobs.
we do allow cancel job and delete job.
we have thought about independent job creation but feedback we got from user is they prefer starting job as part of create instead of one extra API call.
that said, that is why we are not publishing this APIs to public yet and hence doNotMerge tag so we can get early feedback and change accordingly.
we had detailed discussion with ARM team in Aug regarding this.
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
|
@j-so You will need to fix the validations errors above (https://aka.ms/ci-fix ), particularly the Swagger breaking changes indicated would require a breaking change review from the api review board |
Contributor
|
Marking it ARM Approved, please resolve the breaking change and follow the process given @ https://dev.azure.com/azure-sdk/internal/_wiki/wikis/internal.wiki/202/Overall-Process-of-AME-Onboarding |
chiragg4u
added
ARMSignedOff
|
No pipelines are associated with this pull request. |
Member
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Member
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
markcowl
requested changes
Oct 14, 2020
| "schema": { | ||
| "$ref": "#/definitions/ListWorkspaceQuotas" | ||
| } | ||
| }, |
Member
There was a problem hiding this comment.
This is the breaking change described in the breaking change error. This is entirely the right thing to do with this Swagger (not sure how the original swagger was checked in without this). The impact of this in generated clients is that the type of the returned exception may change.
The breaking change tool complains that technically, this requires a new api-version, however, since it is a correction to a preview swagger, I will sign off on this breaking change
| { | ||
| "swagger": "2.0", | ||
| "info": { | ||
| "title": "Azure Machine Learning ManagementFrontEnd Service", |
Member
There was a problem hiding this comment.
Need to fix this as specified in the linting error
| ], | ||
| "parameters": [ | ||
| { | ||
| "$ref": "machineLearningServices.json#/parameters/SubscriptionIdParameter" |
Member
There was a problem hiding this comment.
You should use the common definitions for parameters and resources here: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/common-types/resource-management/v1/types.json
| "default": { | ||
| "description": "Error", | ||
| "schema": { | ||
| "$ref": "machineLearningServices.json#/definitions/MachineLearningServiceError" |
Member
There was a problem hiding this comment.
exception types should come from the common definitions as well (as long as this does not cause breaking changes)
| ], | ||
| "responses": { | ||
| "default": { | ||
| "description": "Error", |
Member
There was a problem hiding this comment.
Should add more meaningful descriptions for each result (this eventually generates REST API documentation)
| "type": "string", | ||
| "x-ms-enum": { | ||
| "name": "DataBindingMode", | ||
| "modelAsString": false |
Member
There was a problem hiding this comment.
See above comment. In this case, these may be the only actions for all time
| "type": "string", | ||
| "x-ms-enum": { | ||
| "name": "DistributionType", | ||
| "modelAsString": false |
Member
There was a problem hiding this comment.
see comments above - if these will ever change, this should be modeled as string
| "readOnly": true, | ||
| "x-ms-enum": { | ||
| "name": "JobStatus", | ||
| "modelAsString": false |
Member
There was a problem hiding this comment.
This should almost certainly be model as string: true
| ], | ||
| "type": "string", | ||
| "x-ms-enum": { | ||
| "name": "JobType", |
Member
There was a problem hiding this comment.
see admonition above about modelasstring
| "Paused" | ||
| ], | ||
| "type": "string", | ||
| "x-ms-enum": { |
markcowl
added
Approved-BreakingChange
Member
|
@j-so Do you still want this change? If no response, will close at end of next week. |
Member
|
closing for lack of response. |
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
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 there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.
Please follow the link to find more details on PR review process.