[Autodown] Support for autodown #1217

Michaelvll · 2022-10-10T03:51:22Z

Closes #954.

Please refer to the following tests for the design of the CLI.
Main changes:

Add an option --terminate forsky launch and sky start
Add an option --terminate for sky autostop
sky.launch replaces the teardown option with terminate.

Tested:

sky launch -c test-autodown -i 1 --terminate; sky status -r
sky cpunode -c test-autodown --cloud gcp; sky autostop --terminate test-autodown; sky status -r
sky cpunode -c test-autodown; sky stop test-autodown; sky start test-autodown -i 1 --terminate; sky status -r
tests/run_smoke_tests.sh test_autodown
sky launch -c test-autodown -i 2 --terminate; sky status

NAME             LAUNCHED        RESOURCES             STATUS   AUTOSTOP      COMMAND                         
test-autodown    a few secs ago  1x AWS(m6i.2xlarge)   UP       2 min (down)  sky launch -c test-autodown...  
smoke-test-zhwu  4 days ago      1x GCP(n1-highmem-8)  STOPPED  -             sky start smoke-test-zhwu

concretevitamin

Nice @Michaelvll! Some comments about user-facing options/help strings.

We may need a smoke test for

sky launch --terminate
sky launch --terminate -i n

concretevitamin · 2022-10-10T19:16:58Z

sky/cli.py

@@ -992,6 +994,15 @@ def cli():
          'Setting this flag is equivalent to '
          'running ``sky launch -d ...`` and then ``sky autostop -i <minutes>``'
          '. If not set, the cluster will not be auto-stopped.'))
+@click.option(
+    '--terminate',


How do you feel about the alternative of --rm? (E.g., use this name in user interfaces; in the backend we can keep terminate)

Pros:

Popularized by docker: https://docs.docker.com/engine/reference/run/#clean-up---rm

For some reason --terminate to me makes sense only for serverful settings. I can imagine in the future sky launch may gain the ability to launch serverless jobs. In that case, "--terminate a job after it exits" is a bit confusing imo.

Feel free to push back as (1) this seems a weak argument (2) --rm may not be that much better.

Faster to type.

Another alternative is --down.

Pros:

Matches sky down.

Matches the colloquial "autodown".

Faster to type.

I think it would be better to keep this consistent with the option in sky.launch, so that the user do not need to learn another set of function parameters when using the programetic API. I feel that the rm can be a bit confusing when appear in the sky.launch(..., rm=True, ...). Do you think replacing both the --terminate and sky.launch(..., terminate=True, ...) with --down and sky.launch(..., down=True, ...) will be better?

My 2c: - --down >> --terminate >> --rm

--down gets highest preference because its consistent with sky down. I'm a little apprehensive of --terminate mainly because it adds a new word terminate to user vocabulary, which has the same effect as sky down. Since down is a fundamental operation, I think it's a good idea to maintain consistency here.

--down sounds great.

Minor: I see that some user-facing help strings get "terminate" replaced with "tear down". That seems okay but the former is much more common in cloud providers parlance (e.g., AWS console).

Good point! I found we are using both tear down and terminate in our docs. I would prefer tear down, since it is closer to the --down or sky down. I went through the docstr's, and tried to make them more consistent, but still keep some of the terminate. PTAL. : )

sky/backends/cloud_vm_ray_backend.py

sky/cli.py

sky/execution.py

sky/core.py

sky/cli.py

concretevitamin · 2022-10-11T15:10:52Z

sky/cli.py

@@ -992,6 +994,15 @@ def cli():
          'Setting this flag is equivalent to '
          'running ``sky launch -d ...`` and then ``sky autostop -i <minutes>``'
          '. If not set, the cluster will not be auto-stopped.'))
+@click.option(
+    '--terminate',


--down sounds great.

Minor: I see that some user-facing help strings get "terminate" replaced with "tear down". That seems okay but the former is much more common in cloud providers parlance (e.g., AWS console).

concretevitamin

Did a complete pass. A few comments.

tests/test_smoke.py

sky/core.py

concretevitamin · 2022-10-12T05:18:56Z

sky/execution.py

+        if stages is None or Stage.DOWN in stages:
+            if down and idle_minutes_to_autostop is None:
                backend.teardown_ephemeral_storage(task)
                backend.teardown(handle, terminate=True)


Yes, it will clean up after the job fails, but if the cluster fails at the setup/filemount.
During adding this I found that it previously will not terminate the cluster when the user ctrl-c'ed during the logging for the execution. I just moved the teardown into the finally: ... block to make sure the clean up will happen after the ctrl-c. Wdyt?

Do you mean that by putting this block in this finally, we will tear down when users ctrl-c a running job's streaming log? That seems unintuitive. More intuitive thing to do seems to be terminate it whenever the job finishes.

By "but if the cluster fails at the setup/filemount." - do you mean if these happen, the cluster will remain up? This is important to clarify in all func docstrs/help strs.

Oh, my bad, I meant when the --down or sky.launch(..., down=True, ...) is specified, the cluster should be terminated if the user press ctrl-c during setup or file mounts. After thinking more about this, I think it should be fine to place it back in its original place.

sky/skylet/events.py

concretevitamin

Some final nits -- mostly to ensure user-facing messages are clear.

sky/utils/cli_utils/status_utils.py

tests/test_smoke.py

sky/cli.py

sky/execution.py

sky/core.py

concretevitamin

Did a pass - mostly nits, some questions on semantics.

sky/cli.py

sky/core.py

sky/execution.py

concretevitamin · 2022-10-13T16:00:06Z

sky/execution.py

+    if isinstance(backend, backends.CloudVmRayBackend):
+        if down and idle_minutes_to_autostop is None:
+            # Use autostop(down) to terminate the cluster after the task is
+            # done. Otherwise, the cluster will be immediately terminated when


Wdym by "Otherwise, the cluster will be immediately terminated when # the user detach from the execution."? Can be omitted?

By that, I mean without the logic here, if the detach_run is specified, the function will immediately terminate the cluster without waiting the job to finish.
It should be fine to omit it. Done. Thanks!

concretevitamin · 2022-10-13T16:02:15Z

sky/execution.py

+        if idle_minutes_to_autostop is not None:
+            if idle_minutes_to_autostop == 0:
+                verb = 'torn down' if down else 'stopped'
+                logger.warning(f'{colorama.Fore.LIGHTBLACK_EX}Setting '


How about printing this warning only for -i 0 but not for --down? The latter is a "normal" use case so shouldn't see a warning.

The main reason for showing this logging for --down is that it can help reduce the confusion caused by seeing autostop 1m (down) after using sky launch --down. Wdyt?

How about logger.info and say something like "The cluster will be {verb}-ed after 1 minute of idleness (after all jobs finish)."?

This is to (1) avoid "warning" for a normal use case, and (2) not show the reason, as it's hard for users to understand.

I still think not showing this for --down is better as the UX is simpler. But up to you!

concretevitamin · 2022-10-13T16:03:04Z

sky/execution.py

@@ -271,7 +288,7 @@ def launch(
            auto-generate a name.
        retry_until_up: whether to retry launching the cluster until it is
            up.
-        idle_minutes_to_autostop: if provided, the cluster will be auto-stop
+        idle_minutes_to_autostop: if provided, the cluster will be autostop


keep consistent with cli?

Done. Also added the docstr for the other argument as well. I think it may require another PR to consolidate the docstr's for the programmatic API.

concretevitamin · 2022-10-13T16:03:45Z

sky/execution.py

+                verb = 'torn down' if down else 'stopped'
+                logger.warning(f'{colorama.Fore.LIGHTBLACK_EX}Setting '
+                               'idle_minutes_to_autostop to 1, to avoid '
+                               f'cluster being {verb} during task submission.'


Why would -i 0 have this issue of being torn down during task submission? Is that a bug in how the autostop event is implemented?

The reason is that after we set the autostop in the PRE_EXEC stage with -i 0, it could be possible that the cluster immediately found itself have no task running and start the autostop/down process, before the task is submitted in the EXEC stage.
I believe the behavior is expected. Wdyt?

That makes sense. Can we copy this comment into the code where we reset the var to 1?

concretevitamin

LGTM @Michaelvll!

concretevitamin · 2022-10-14T06:03:31Z

sky/execution.py

+                verb = 'torn down' if down else 'stopped'
+                logger.warning(f'{colorama.Fore.LIGHTBLACK_EX}Setting '
+                               'idle_minutes_to_autostop to 1, to avoid '
+                               f'cluster being {verb} during task submission.'


That makes sense. Can we copy this comment into the code where we reset the var to 1?

concretevitamin · 2022-10-14T06:07:59Z

sky/execution.py

+        if idle_minutes_to_autostop is not None:
+            if idle_minutes_to_autostop == 0:
+                verb = 'torn down' if down else 'stopped'
+                logger.warning(f'{colorama.Fore.LIGHTBLACK_EX}Setting '


How about logger.info and say something like "The cluster will be {verb}-ed after 1 minute of idleness (after all jobs finish)."?

This is to (1) avoid "warning" for a normal use case, and (2) not show the reason, as it's hard for users to understand.

I still think not showing this for --down is better as the UX is simpler. But up to you!

…nto auto-down

Michaelvll · 2022-10-14T06:33:14Z

Tested:

tests/run_smoke_tests.sh
sky launch -c test-autodown --down; sky status

NAME             LAUNCHED        RESOURCES             STATUS  AUTOSTOP   COMMAND                         
test-autodown    a few secs ago  1x AWS(m6i.2xlarge)   UP      1m (down)  sky launch -c test-autodown...  
smoke-test-zhwu  1 day ago       1x GCP(n1-highmem-8)  UP      -          sky start smoke-test-zhwu

…nto auto-down

* Support for autodown * Change API to terminate * fix flag * fix autostop * fix comment * address comment * address comment * format * Rename terminate to down * add smoke test * fix autodown for multi-node * format * fix syntax * use gcp for autodown test * fix smoke test * fix smoke test * address comments * Add comment * Switch back to terminate * fix comments * Change back to tear down * Change to tear down * fix comment * change the logic of --down to use auto-down by default * Use autodown for --down and address comments * fix comment * fix ux * Add test for cancel * fix UX * fix test_smoke * address comments * fix * fix logging and comment * fix environment variable overwrite * fix smoke test * print info

Michaelvll added 5 commits October 9, 2022 18:11

Support for autodown

c54ef6c

Change API to terminate

10f41e6

fix flag

420510f

fix autostop

9a741aa

fix comment

f057f3d

concretevitamin self-requested a review October 10, 2022 19:29

concretevitamin reviewed Oct 10, 2022

View reviewed changes

Michaelvll added 11 commits October 10, 2022 13:37

address comment

b646909

address comment

a6ccef8

format

0660eb1

Rename terminate to down

2a45671

add smoke test

d89fe93

fix autodown for multi-node

c2a4c4a

format

6354619

fix syntax

18bc534

use gcp for autodown test

8658d3e

fix smoke test

8280c9d

fix smoke test

5a08c84

Michaelvll requested a review from concretevitamin October 11, 2022 05:49

concretevitamin reviewed Oct 11, 2022

View reviewed changes

concretevitamin reviewed Oct 12, 2022

View reviewed changes

Michaelvll added 7 commits October 12, 2022 00:48

address comments

59ced1d

Add comment

f3b357e

Switch back to terminate

5198b1a

fix comments

bce99fc

Change back to tear down

c214028

Change to tear down

ccdd792

fix comment

5425c21

Michaelvll changed the title ~~Support for autodown~~ [Autostop] Support for autodown Oct 12, 2022

Michaelvll changed the title ~~[Autostop] Support for autodown~~ [Autodown] Support for autodown Oct 12, 2022

Michaelvll requested a review from concretevitamin October 12, 2022 20:24

concretevitamin reviewed Oct 12, 2022

View reviewed changes

Michaelvll added 7 commits October 12, 2022 14:47

change the logic of --down to use auto-down by default

7e309b4

Use autodown for --down and address comments

b625173

fix comment

306671d

fix ux

5aff9e4

Add test for cancel

2cda239

fix UX

787ac90

fix test_smoke

b7596b7

concretevitamin reviewed Oct 13, 2022

View reviewed changes

Michaelvll added 2 commits October 13, 2022 22:21

address comments

e34f88e

fix

faee1a0

Michaelvll requested a review from concretevitamin October 14, 2022 05:35

concretevitamin approved these changes Oct 14, 2022

View reviewed changes

Michaelvll added 2 commits October 13, 2022 23:19

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

e012373

…nto auto-down

fix logging and comment

f653c84

Michaelvll added 4 commits October 14, 2022 00:24

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

ca57e69

…nto auto-down

fix environment variable overwrite

57343b8

fix smoke test

1d32197

print info

83dbdff

Michaelvll merged commit 72e2837 into master Oct 14, 2022

Michaelvll deleted the auto-down branch October 14, 2022 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Autodown] Support for autodown #1217

[Autodown] Support for autodown #1217

Michaelvll commented Oct 10, 2022 •

edited

Loading

concretevitamin left a comment

concretevitamin Oct 10, 2022

Michaelvll Oct 10, 2022

romilbhardwaj Oct 11, 2022

concretevitamin Oct 11, 2022

Michaelvll Oct 12, 2022

concretevitamin Oct 11, 2022

concretevitamin left a comment

concretevitamin Oct 12, 2022

Michaelvll Oct 12, 2022

concretevitamin left a comment

concretevitamin left a comment

concretevitamin Oct 13, 2022

Michaelvll Oct 14, 2022

concretevitamin Oct 13, 2022

Michaelvll Oct 14, 2022

concretevitamin Oct 14, 2022

concretevitamin Oct 13, 2022

Michaelvll Oct 14, 2022

concretevitamin Oct 13, 2022

Michaelvll Oct 14, 2022

concretevitamin Oct 14, 2022

concretevitamin left a comment

concretevitamin Oct 14, 2022

concretevitamin Oct 14, 2022

Michaelvll commented Oct 14, 2022 •

edited

Loading

[Autodown] Support for autodown #1217

[Autodown] Support for autodown #1217

Conversation

Michaelvll commented Oct 10, 2022 • edited Loading

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concretevitamin left a comment

Choose a reason for hiding this comment

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll commented Oct 14, 2022 • edited Loading

Michaelvll commented Oct 10, 2022 •

edited

Loading

Michaelvll commented Oct 14, 2022 •

edited

Loading