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 sky tpunode #120

Merged
merged 2 commits into from
Dec 23, 2021
Merged

Add sky tpunode #120

merged 2 commits into from
Dec 23, 2021

Conversation

gmittal
Copy link
Collaborator

@gmittal gmittal commented Dec 22, 2021

TPU nodes in addition to GPU/CPU nodes! Verified that sky tpunode exhibits same functionality as cpunode/gpunode and that the TPU is actually attached to the created cluster using capture_tpu_profile --tpu=$TPU_NAME --monitoring_level=2.

prototype/sky/cli.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@michaelzhiluo michaelzhiluo left a comment

Choose a reason for hiding this comment

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

Just change back to AWS. Otherwise, LGTM

Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! I suggested some refactoring, but it can be deprioritized in favor of agility for now

Comment on lines +573 to +590
@click.option('--cluster',
'-c',
default=None,
type=str,
help=_CLUSTER_FLAG_HELP)
@click.option('--port-forward',
'-p',
multiple=True,
default=[],
type=int,
required=False,
help=('Port to be forwarded. To forward multiple ports, '
'use this option multiple times.'))
@click.option('--screen',
default=False,
is_flag=True,
help='If true, attach using screen.')
def tpunode(cluster: str, port_forward: Optional[List[int]], screen):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possible future refactor, not necessary:
There seems to be considerable overlap between gpunode, cpunode and tpunode (similar click.options, docs, args and body). Maybe there's some opportunity for refactoring here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great suggestion. Made a note in #122.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@gmittal gmittal merged commit 353431e into master Dec 23, 2021
@gmittal gmittal deleted the user/gmittal/tpunode branch December 23, 2021 08:09
@concretevitamin
Copy link
Collaborator

I tested this and filed #129.

gmittal added a commit that referenced this pull request Mar 15, 2022
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