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.

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

Add compute cluster properties to job launching #86

Conversation

niole
Copy link
Contributor

@niole niole commented Mar 22, 2021

This PR adds compute cluster support to the start job endpoint. This looks like it is the only endpoint that can launch cluster related executions, so I only added it there.

There doesn't seem to be a standardized way for doing input validation, yet there is input validation on the start job endpoint, so I added basic input validation for the compute cluster definition. I did not do validation on whether the HWTs or env ids exist or not, because domino will reject with informative messaging if that is the case.

https://dominodatalab.atlassian.net/browse/DOM-27821

Copy link
Contributor

@mpkouznetsov mpkouznetsov left a comment

Choose a reason for hiding this comment

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

Looks good; two minor comments.

domino/domino.py Outdated Show resolved Hide resolved
domino/domino.py Outdated Show resolved Hide resolved
@niole niole force-pushed the niole.DOM-27821.add-support-for-cluster-properties-domino-python branch from b6b1970 to 1c1cf86 Compare April 14, 2021 20:35
Copy link

@chittshota chittshota left a comment

Choose a reason for hiding this comment

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

@niole Changes look fine other than the master storage option. Can you remove it completely here. masterStorage is becoming obsolete. We can remove it here and later on from the backend as well. For now this API doesn't need to send any value for masterStorage or None/Non-validated.

@ddl-alexpanin ddl-alexpanin merged commit b105d34 into dominodatalab:master Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants