Repository: skypilot-org/skypilot
Pull request: Logging UX for `sky cancel`.
<!-- Describe the changes in this PR -->

Fixes #1536. Fixes #1537.


<!-- Describe tests ran in a "Tested:" section -->
<!-- Unit tests (tests/test_*.py) are part of Github CI; the below are additional tests. -->

Tested (run the relevant ones):

- [x] Any manual or new tests for this the PR
```
sky cancel 1
sky cancel myclus  
sky cancel sky-spot-controller-xx -a                
sky cancel sky-spot-controller-xx 1             
```


Problem statement:
UX: `sky cancel` usage hints should be friendlier
Users tended to type `sky cancel <job id>` when they only started playing with one cluster.

This would show
```
click.exceptions.UsageError: sky cancel requires either a job id (see `sky queue 1 -s`) or the --all flag.
```

We should instead show something like
```
... Use `sky cancel <cluster name> <job id>` or `sky cancel <cluster name> --all`. Job IDs can be looked up by `sky queue`.
``` 

UX: users may call `cancel` on spot controller 
When users call
```
sky cancel <spot controller> 1
```
We currently show
```
ValueError: Cluster 'sky-spot-controller-1234' is reserved for spot controller. Cancelling jobs is not allowed.
```
We should additionally show something like, 
```
...
Use `sky spot cancel <spot job id>` to cancel spot jobs.
```

Hints from issue discussion:

Commit message: Fix test

Patch to review:
diff --git a/sky/cli.py b/sky/cli.py
index a8560990372..8dbb0874f3a 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -1607,10 +1607,33 @@ def logs(
 def cancel(cluster: str, all: bool, jobs: List[int]):  # pylint: disable=redefined-builtin
     # NOTE(dev): Keep the docstring consistent between the Python API and CLI.
     """Cancel job(s)."""
+    bold = colorama.Style.BRIGHT
+    reset = colorama.Style.RESET_ALL
+    if not jobs and not all:
+        # Friendly message for usage like 'sky cancel 1'.
+        raise click.UsageError(
+            f'Use {bold}sky cancel <cluster> <job ID>{reset} '
+            f'or {bold}sky cancel <cluster> --all{reset} to cancel one '
+            'or all jobs on a cluster. Job IDs can be looked up by '
+            f'{bold}sky queue{reset}.')
     try:
         core.cancel(cluster, all, jobs)
     except ValueError as e:
-        raise click.UsageError(str(e))
+        if 'Cancelling jobs is not allowed' in str(e):
+            # Friendly message for usage like 'sky cancel <spot controller>
+            # -a/<job id>'.
+            if all:
+                arg_str = '--all'
+            else:
+                arg_str = ' '.join(map(str, jobs))
+            error_str = (
+                'Cancelling the spot controller\'s jobs is not allowed.'
+                f'\nTo cancel spot jobs, use: {bold}sky spot cancel <spot '
+                f'job IDs> [--all]{reset}'
+                f'\nDo you mean: {bold}sky spot cancel {arg_str}{reset}')
+        else:
+            error_str = str(e)
+        raise click.UsageError(error_str)
     except (exceptions.NotSupportedError, exceptions.ClusterNotUpError) as e:
         click.echo(str(e))
         sys.exit(1)
diff --git a/tests/test_spot.py b/tests/test_spot.py
index d24bc9b41fc..e921c446bac 100644
--- a/tests/test_spot.py
+++ b/tests/test_spot.py
@@ -169,4 +169,5 @@ def test_cancel_on_spot_controller(self, _mock_cluster_state):
         result = cli_runner.invoke(cli.cancel,
                                    [spot.SPOT_CONTROLLER_NAME, '-a'])
         assert result.exit_code == click.UsageError.exit_code
-        assert 'Cancelling jobs is not allowed' in str(result.output)
+        assert 'Cancelling the spot controller\'s jobs is not allowed.' in str(
+            result.output)


Code review comments:
diff_hunk: @@ -1607,10 +1607,33 @@ def logs(
 def cancel(cluster: str, all: bool, jobs: List[int]):  # pylint: disable=redefined-builtin
     # NOTE(dev): Keep the docstring consistent between the Python API and CLI.
     """Cancel job(s)."""
+    bold = colorama.Style.BRIGHT
+    reset = colorama.Style.RESET_ALL
+    if not jobs and not all:
+        # Friendly message for usage like 'sky cancel 1'.
+        raise click.UsageError(
+            f'Use {bold}sky cancel <cluster> <job ID>{reset} '
+            f'or {bold}sky cancel <cluster> --all{reset} to cancel one '
+            'or all jobs on a cluster. Job IDs can be looked up by '
+            f'{bold}sky queue{reset}.')
     try:
         core.cancel(cluster, all, jobs)
     except ValueError as e:
-        raise click.UsageError(str(e))
+        if 'Cancelling jobs is not allowed' in str(e):
+            # Friendly message for usage like 'sky cancel <spot controller>
+            # -a/<job id>'.
+            if all:
+                arg_str = '--all'
+            else:
+                arg_str = ' '.join(map(str, jobs))
+            error_str = (
+                'Cancelling the spot controller\'s jobs is not allowed.'
+                f'\nTo cancel spot jobs, use: {bold}sky spot cancel <spot '
+                f'job IDs> [--all]{reset}'
+                f'\nDo you mean: {bold}sky spot cancel {arg_str}{reset}')
+        else:
+            error_str = str(e)
+        raise click.UsageError(error_str)
path: sky/cli.py
text: @user1:
Nice! Is it cleaner to use a dedicated exception for the case when the user is trying to cancel the job on the spot controller, instead of using the `ValueError`?
Reason: checking the string in the exception can be less robust, as we need to keep the string value checking and the error message in sync, say if we change the error message in the future, but forget to change it here, it will fail.

@author:
Good call. Done.