[Spot] Event callback for adding notification #2106

MaoZiming · 2023-06-20T14:02:40Z

Issue #2000: A user would like to have a customizable event callback for the spot job, so that they can implement some notification when the spot job status changes. Example:

event_callback:
  send_slack_message $JOB_ID $JOB_STATUS

Allows the controller to execute a customizable bash script when the spot status changes.

Example:
Register a Slack message web hook from here.

event_callback: |
  echo 'this is an event callback script'
  curl -X POST -H 'Content-type: application/json' --data "{\"text\":\"JOB_ID: $SKYPILOT_JOB_ID, JOB_STATUS: $JOB_STATUS\"}" https://hooks.slack.com/services/...

The user will receive a Slack notification when the spot status changes.

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

Michaelvll

Thank you for adding this feature @MaoZiming! It would be very useful for the users. Left several comments.

Additional comments:

We may want to call the event callback when the spot job starts recovering as well, according to the users feedback.
We may want to think of whether we should run the callback asynchronously, instead of in a blocking way. Otherwise, our control logic may be significantly delayed.

sky/spot/controller.py

sky/task.py

…o spot-event-callback

MaoZiming · 2023-06-23T03:27:18Z

@Michaelvll Incorporated all the comments. The basic skeleton for slack notification should be there. Prob need better UI

…o spot-event-callback

Michaelvll

Thanks for the quick fix @MaoZiming! The slack messages were sent like a magic. It works well for STARTING/RUNNING/RECOVERING/RECOVERED/SUCCEEDED, but it seems like not working well with the CANCELLING/CANCELLED.

sky/spot/controller.py

sky/spot/spot_state.py

sky/task.py

Michaelvll · 2023-06-27T19:17:55Z

sky/spot/spot_state.py

@@ -419,9 +470,13 @@ def set_cancelling(job_id: int):
            (SpotStatus.CANCELLING.value, time.time(), job_id))
        if rows.rowcount > 0:
            logger.info('Cancelling the job...')
+    if callback_func is not None:
+        callback_func(-1, 'CANCELLING', '')


Weirdly, when I am trying this PR, the STARTING, RUNNING, RECOVERING, RECOVERED works normally, while this CANCELLING/CANCELLED state was not sent to my slack. Do you have an idea why this happens?
Fyi, I did the following:

sky spot launch test.yaml

manually kill the cluster on the console

the states were correctly sent

sky spot cancel -a: the cancelling/cancelled states were not correctly sent.

For set_cancelling outside of SpotController, I didn't pass in event_callback_func.
https://github.com/skypilot-org/skypilot/blob/spot-event-callback/sky/spot/controller.py#L456

Do we want to pass the event_callback_func to those as well? Otherwise, sky spot cancel -a will not send the message.

I am trying to think of a clean way to do this since we need to call event_callback_func outside of SpotController and multiprocessing.Process(target=_run_controller...).
Do you know if is there an easy way to get task inside the child process?

Ahh, as we did for the _cleanup function, we can retrieve the dag using the day_yaml as we did in _cleanup function

skypilot/sky/spot/controller.py

Line 430 in 438f815

_cleanup(job_id, dag_yaml=dag_yaml)

However, we are not able to get some of the environment variable. We can probably print some log in the event callback saying which variables are not accessible, and we can wait for the user feedback to see if they want to have some of the variables in the CANCEL.

@Michaelvll Thanks for all the detailed feedback! The suggested changes should all be incorporated

sky/spot/spot_state.py

Michaelvll

Thanks for this exciting PR @MaoZiming! The code looks good to me. Several small nits.

Michaelvll · 2023-07-04T01:19:11Z

sky/spot/controller.py

@@ -19,6 +19,7 @@
 from sky.spot import recovery_strategy
 from sky.spot import spot_state
 from sky.spot import spot_utils
+from sky.spot.spot_utils import event_callback_func


style nit: import module instead of functions. https://google.github.io/styleguide/pyguide.html#224-decision

Michaelvll · 2023-07-04T01:22:32Z

sky/spot/controller.py

+        first_task_id = 0
+        first_task = dag.tasks[first_task_id]


nit:

Suggested change

first_task_id = 0

first_task = dag.tasks[first_task_id]

first_task = dag.tasks[0]

Actually, do you think we can use spot_state.get_latest_task_id_status to get the current task_id, so that the message can include the information of the currently cancelled task?

Yep. I think that's better. Thanks!

Michaelvll · 2023-07-04T01:43:40Z

sky/spot/spot_utils.py

+    cluster_name = generate_spot_cluster_name(task.name,
+                                              job_id) if task.name else None
+    logger.info(f'=== START: event callback for {state!r} ===')
+    log_path = os.path.join(constants.SKY_LOGS_DIRECTORY,


Should we place it in a folder instead of the root directory of the logs? Probably, under ~/sky_logs/spot_event?

MaoZiming · 2023-07-04T07:12:19Z

Thank you so much @Michaelvll! I have incorporated all the suggestions.

Michaelvll

Thanks for the quick fix @MaoZiming! I just realized that the current way to pass the callback function can be a bit confusing. Do you think we can do something like the comments below?

sky/spot/spot_state.py

Michaelvll

Thanks for awesome work @MaoZiming! LGTM. Please feel free to merge it.

concretevitamin · 2023-07-07T01:26:27Z

Very nice @MaoZiming!

MaoZiming added 3 commits June 18, 2023 22:59

spot event callback, added controller_log option

9989fac

added callback_script, with subprocess.run

5b94f5e

merge master

5eba995

MaoZiming linked an issue Jun 20, 2023 that may be closed by this pull request

[Spot] Event callback for adding notification #2000

Closed

running format.sh

868fc0e

Michaelvll reviewed Jun 21, 2023

View reviewed changes

MaoZiming added 5 commits June 22, 2023 08:43

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

cca5a2f

…o spot-event-callback

use run_bash_command_with_log, fix feedback

2077716

added callback func for starting, and recovering

ed605c1

modified callback string

527532d

added cluster_name and task_name for callback

0f44298

MaoZiming marked this pull request as ready for review June 23, 2023 03:26

MaoZiming added 4 commits June 26, 2023 14:08

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

73bccfe

…o spot-event-callback

added event callbacks for cancelling and pending

c86deaf

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

38e5ec7

…o spot-event-callback

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

6dfaecf

…o spot-event-callback

concretevitamin requested a review from Michaelvll June 27, 2023 15:58

Michaelvll reviewed Jun 27, 2023

View reviewed changes

MaoZiming added 8 commits June 28, 2023 09:56

incorperate code reviews: import,CallbackType, defaultNone, etc

cf675b1

pass job_id to event_callback and added EVENT_TYPE

6afd3aa

move event_callback_func outside and added set_cancelling

92e1220

move spotcallback to spot_utils

ec1442a

fix format

e6e05ae

fix TASK_ID_ENV_VAR

6e82724

remove set_pending callback

814ea02

remove comment for callback

07299fa

Michaelvll approved these changes Jul 4, 2023

View reviewed changes

incorperated code reviews, import, task_id, folder

2a9d24a

Michaelvll reviewed Jul 5, 2023

View reviewed changes

sky/spot/spot_state.py Outdated Show resolved Hide resolved

MaoZiming added 2 commits July 6, 2023 09:12

reformat callback_func

e7080bb

move status inside callback_func

a8b4135

Michaelvll approved these changes Jul 6, 2023

View reviewed changes

MaoZiming merged commit 9f39f8d into master Jul 7, 2023

MaoZiming deleted the spot-event-callback branch July 7, 2023 01:20

Michaelvll mentioned this pull request Jul 7, 2023

Docs: add "Using Environment Variables". #2188

Merged

5 tasks

cblmemo mentioned this pull request Dec 28, 2023

[SkyServe] Call back functions for sensitive action #2918

Open

concretevitamin mentioned this pull request Jul 24, 2024

[k8s] how to get the job sutatus change event #3775

Open

cblmemo mentioned this pull request Sep 4, 2024

[Docs] Add docs for event callback notification #3909

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spot] Event callback for adding notification #2106

[Spot] Event callback for adding notification #2106

MaoZiming commented Jun 20, 2023 •

edited

Loading

Michaelvll left a comment

MaoZiming commented Jun 23, 2023 •

edited

Loading

Michaelvll left a comment

Michaelvll Jun 27, 2023

MaoZiming Jun 28, 2023

Michaelvll Jun 28, 2023

MaoZiming Jun 28, 2023 •

edited

Loading

Michaelvll Jun 29, 2023

MaoZiming Jul 1, 2023

Michaelvll left a comment

Michaelvll Jul 4, 2023

Michaelvll Jul 4, 2023

Michaelvll Jul 4, 2023

MaoZiming Jul 4, 2023

Michaelvll Jul 4, 2023

MaoZiming commented Jul 4, 2023

Michaelvll left a comment

Michaelvll left a comment

concretevitamin commented Jul 7, 2023

	first_task_id = 0
	first_task = dag.tasks[first_task_id]
	first_task = dag.tasks[0]

[Spot] Event callback for adding notification #2106

[Spot] Event callback for adding notification #2106

Conversation

MaoZiming commented Jun 20, 2023 • edited Loading

Michaelvll left a comment

Choose a reason for hiding this comment

MaoZiming commented Jun 23, 2023 • edited Loading

Michaelvll 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

MaoZiming Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll 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

MaoZiming commented Jul 4, 2023

Michaelvll left a comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment

concretevitamin commented Jul 7, 2023

MaoZiming commented Jun 20, 2023 •

edited

Loading

MaoZiming commented Jun 23, 2023 •

edited

Loading

MaoZiming Jun 28, 2023 •

edited

Loading