Hints for spot controller in sky status output.
#1467
Conversation
sky/utils/cli_utils/status_utils.py
Outdated
| click.echo( | ||
| f'{colorama.Style.DIM} Directly interacting with the ' | ||
| f'controller is typically not needed.{colorama.Style.RESET_ALL}' | ||
| ) | ||
| click.echo( | ||
| f' View spot jobs: {colorama.Style.BRIGHT}sky spot queue' | ||
| f'{reset}\n' | ||
| f' View job logs: {colorama.Style.BRIGHT}sky spot logs' | ||
| f'{reset}\n') |
There was a problem hiding this comment.
This looks a bit verbose to me. How about:
| click.echo( | |
| f'{colorama.Style.DIM} Directly interacting with the ' | |
| f'controller is typically not needed.{colorama.Style.RESET_ALL}' | |
| ) | |
| click.echo( | |
| f' View spot jobs: {colorama.Style.BRIGHT}sky spot queue' | |
| f'{reset}\n' | |
| f' View job logs: {colorama.Style.BRIGHT}sky spot logs' | |
| f'{reset}\n') | |
| click.echo( | |
| f'{colorama.Style.DIM} Directly interacting with the ' | |
| f'controller is typically not needed.{colorama.Style.RESET_ALL} (Check {colorama.Style.BRIGHT}sky spot --help{reset})' | |
| ) |
There was a problem hiding this comment.
How about this? Reason: Putting them on one line seems to make the line too long and the command not noticeable.
There was a problem hiding this comment.
How about For spot-related CLI, run: sky spot --help?
There was a problem hiding this comment.
Three versions. The bottom one looks best to me, as the command doesn't overlapped with LAUNCHED. Wdyt?
There was a problem hiding this comment.
I like the last one. Curious, why we don't add --help? That will explicitly telling the user it will show the helps (I think showing help without --help is a non-standard behavior reference).
Or we can reword it by Use spot jobs CLI that starts with: sky spot (or include a linke to the docs?)
There was a problem hiding this comment.
Good point; how does this look now?
There was a problem hiding this comment.
Hmm, having the sentence below the spot jobs CLI hint does not read very smoothly.
Can the sentence be shortened and combined with the (autostopped if idle for 10 min), such as (autostopped if idle for 10 min, direct interaction is typically not needed)?
I think the overlapping of the sky spot --help with LAUNCHED should be fine?
There was a problem hiding this comment.
Reordered the command hint and the 'not needed' sentence.
I thought about putting it inside the parens after 'if idle for 10min', but that could be understood as "direct interaction to stop the cluster".
| @@ -97,9 +97,15 @@ def show_status_table(cluster_records: List[Dict[str, Any]], | |||
| autostop_minutes = spot.SPOT_CONTROLLER_IDLE_MINUTES_TO_AUTOSTOP | |||
| click.echo(f'\n{colorama.Fore.CYAN}{colorama.Style.BRIGHT}' | |||
| f'{reserved_group_name}{colorama.Style.RESET_ALL}' | |||
| f'{colorama.Style.DIM} (will be autostopped if idle for ' | |||
| f'{colorama.Style.DIM} (autostopped if idle for ' | |||
There was a problem hiding this comment.
nit: autostopped alone does not have a clear meaning to me. Not sure if other people have the same feeling.
There was a problem hiding this comment.
LGTM! Thanks for the fix @concretevitamin ! This will make the spot controller concept more clear to the user. Left a small nit.
Here is how it shows in light schema:
|
Actually, spoke too soon, do you think having the hint after the table will look better? I found the two lines without left alignment look a bit weird to me. |
|
Discussed with @Michaelvll: we decided to leave out "direct interaction is typically not needed" as it may seem contradictory to the controller being shown. We arrived at the current wording. |
* Hints for spot controller in `sky status` output. * Update hint. * Update message. * Reword * Reorder * pylint * Reword * Remnant.
User reported some confusion on whether to use the controller to see spot job logs or not.
Adding some hints to hopefully address this confusion:
Does this look better?
TODO: