Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

Support datalab connect() to connect a GCE instance created from Deep… #2112

Merged
merged 5 commits into from
Jan 15, 2019

Conversation

yixinshi
Copy link
Contributor

…learning images

qimingj
qimingj previously approved these changes Jan 12, 2019
Copy link
Contributor

@qimingj qimingj left a comment

Choose a reason for hiding this comment

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

a couple of nits.

@@ -68,7 +68,8 @@ class InvalidInstanceException(Exception):

_MESSAGE = (
'The specified instance, {}, does not appear '
'to have been created by the `datalab` tool, and '
'to have been created by the `datalab` tool, or '
'from any GCE Deeplearning images, and '
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the "and"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote it.

@@ -240,22 +241,35 @@ def flatten_metadata(metadata):
return result


def _check_datalab_tag(instance, tags):
def _check_datalab_tag(instance, status_tags_and_metadata):
Copy link
Contributor

Choose a reason for hiding this comment

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

Also rename the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@yixinshi
Copy link
Contributor Author

Thanks, qimingj. PTAL

qimingj
qimingj previously approved these changes Jan 12, 2019
qimingj
qimingj previously approved these changes Jan 14, 2019
@qimingj
Copy link
Contributor

qimingj commented Jan 14, 2019

There is a test failure. I just reran it and see if it is just flaky.

'so cannot be managed by it.')
'to have been created by the `datalab` tool, or '
'from any GCE Deeplearning images. Therefore it '
'so cannot be managed by `datalab` tool.')
Copy link
Contributor

Choose a reason for hiding this comment

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

The "so " here is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Can you grant me the write access to the repo? Thanks!

@yixinshi yixinshi merged commit c6050c7 into googledatalab:master Jan 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants