Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update machine learning compute portion of the machine learning services swagger #3889

Merged
merged 5 commits into from
Sep 19, 2018
Merged

update machine learning compute portion of the machine learning services swagger #3889

merged 5 commits into from
Sep 19, 2018

Conversation

dagald
Copy link

@dagald dagald commented Sep 13, 2018

Update Machine Learning Compute portion of the swagger.
Examples updated

Run OAV and RestAPI validation.

@AutorestCI AutorestCI mentioned this pull request Sep 13, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Sep 13, 2018 •
edited
Loading

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Sep 13, 2018 •
edited
Loading

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Sep 13, 2018 •
edited
Loading

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@AutorestCI
Copy link

AutorestCI commented Sep 13, 2018 •
edited
Loading

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI AutorestCI mentioned this pull request Sep 13, 2018
Closed
@AutorestCI
Copy link

AutorestCI commented Sep 13, 2018 •
edited
Loading

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#2762

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

dsgouda
dsgouda suggested changes Sep 13, 2018
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

...er/Microsoft.MachineLearningServices/preview/2018-03-01-preview/machineLearningServices.json Show resolved Hide resolved
...er/Microsoft.MachineLearningServices/preview/2018-03-01-preview/machineLearningServices.json Outdated
],
"responses": {
"202": {
"description": "Compute System Update initiated.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this and update for the MachineLearningCompute resource?
Update is usually a PATCH operation. Did this service undergo an API review?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, 202 corresponds to Accepted which ideally should be a long running operation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an update on an internal system that is deployed on top of the compute. Not the compute itself.

The swagger that was uploaded before was incorrect. And the service has gone to a lot of changes

@dagald-msft
added long-running op
4591722 
update accordingly
@dsgouda
Copy link
Contributor

dsgouda commented Sep 13, 2018

In case changes were made to the service itself we might need an ARM review here

@dsgouda dsgouda added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Sep 13, 2018
KrisBash
KrisBash reviewed Sep 13, 2018
View reviewed changes
...er/Microsoft.MachineLearningServices/preview/2018-03-01-preview/machineLearningServices.json
@@ -608,15 +608,68 @@
}
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for adding a POST here? In any case, POST requires an action. This is invalid at /workspaces/{workspaceName}/computes/{computeName} without an action.

Copy link

@asjindal3312 asjindal3312 Sep 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the REST methods here are not consistent with REST API design protocols. How are these methods decided in the first place?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this swagger is just representing the current state of the service api

KrisBash
KrisBash suggested changes Sep 13, 2018
View reviewed changes
Copy link
Contributor

@KrisBash KrisBash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments regarding that POST on /computes/{computeName}

@KrisBash KrisBash added ARMReviewInProgress and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Sep 13, 2018
KrisBash
KrisBash approved these changes Sep 19, 2018
View reviewed changes
Copy link
Contributor

@KrisBash KrisBash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving from ARM on the understanding that this swagger update is aligning to actual service implementation. Comments can be addressed in a future API version.

@KrisBash KrisBash added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMReviewInProgress labels Sep 19, 2018
dsgouda
dsgouda approved these changes Sep 19, 2018
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dsgouda dsgouda merged commit 0e79c73 into Azure:master Sep 19, 2018
@AutorestCI
Copy link

AutorestCI commented Sep 19, 2018 •
edited
Loading

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

@AutorestCI AutorestCI mentioned this pull request Sep 19, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salilbapat salilbapat salilbapat left review comments

@KrisBash KrisBash KrisBash approved these changes

@dsgouda dsgouda dsgouda approved these changes

@asjindal3312 asjindal3312 asjindal3312 approved these changes

Assignees

@dsgouda dsgouda

Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@dagald @AutorestCI @azuresdkci @dsgouda @KrisBash @salilbapat @asjindal3312 @dagald-msft