[CLI] Improve interactive node configurability #123
Conversation
| help='If true, attach using screen.') | ||
| def gpunode(cluster: str, port_forward: Optional[List[int]], screen): | ||
| @_interactive_node_cli_command | ||
| def gpunode(cluster: str, port_forward: Optional[List[int]], |
There was a problem hiding this comment.
Lots of shared code between the XYZ nodes. Good to abstract them out?
There was a problem hiding this comment.
I took a second look and it seems reasonable inlined as is (the defaults for each command are different after all). Did you have specific blocks in mind that should get their own function?
There was a problem hiding this comment.
Yes like move all the checking of tmux and screen into its own function.
|
What's the thinking on specifying the common case -- requesting some GPUs -- with minimal typing? Current: num_accelerators_option = click.option(
'--num-accelerators',
'-n',
default=None,
type=int,
help='Number of accelerators (GPU/TPU) to use.')
accelerator_type_option = click.option('--accelerator-type',
'-g',
default=None,
type=str,
help='Accelerator type to use.')seems a bit verbose. Have you thought about parsing them out from a single flag, something like |
gmittal
force-pushed
the
user/gmittal/interactive-node-config
branch
from
2a8e6d7
to
adb9283
Compare
gmittal
changed the title
prototype/sky/cli.py
Outdated
| commands += ['screen', '-D', '-R'] | ||
| if screen_manager == 'tmux': | ||
| commands += ['tmux'] |
There was a problem hiding this comment.
Probably want options to re-attach back. tmux a only works when tmux is created, so needs something else.
There was a problem hiding this comment.
How about tmux attach || tmux new as in https://github.com/ray-project/ray/blob/0a5942d8b043bf01ffb7356bfa513234f775ee7f/python/ray/autoscaler/_private/commands.py#L881 ?
prototype/sky/cli.py
Outdated
| name = cluster | ||
| if name is None: | ||
| name = _default_interactive_node_name('cpunode') | ||
|
|
||
| cloud_provider = task_lib.CLOUD_REGISTRY.get(cloud, sky.AWS()) |
There was a problem hiding this comment.
Does it make sense to use cloud.upper()?
There was a problem hiding this comment.
I don't think it really saves much here since we still need to check if cloud is None and raise a click.UsageError if cloud not in ['aws', 'gcp', 'azure'].
This should really be
sky up ARGSand thensky gpunode/sky tpunode/sky cpunodeare just aliases with single node defaults.