Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Notification support #446

Draft
wants to merge 75 commits into
base: main
Choose a base branch
from
Draft

Conversation

andrii-i
Copy link
Collaborator

@andrii-i andrii-i commented Oct 20, 2023

Problem

Jupyter Scheduler provides people who use Jupyter with an ability to run notebooks as jobs including running them on a schedule. Since these tasks can be long-running, users have to go back to JupyterLab instance running Jupyter Scheduler and check the status of the running jobs manually. This PR adds support for notifications to solve these problems.

Proposed Solution

  1. Server
  • Add class Notification(BaseModel) that represents a notification. Attributes:
    send_to (List[str]): A list of symbols (for example, email addresses) to which notifications should be sent.
    events (List[NotificationEvent]): A list of events that should trigger the sending of notifications.
    include_output (bool): A flag indicating whether a output should be included in the notification. Default is False.
  • Add class NotificationEvent(str, Enum) that represents events triggering notifications. Implementers can extend this enum to include additional notification events as needed. Attributes:
    SUCCESS (str): Sent when a job completes successfully.
    FAILURE (str): Sent on job failure.
    STOPPED (str): Sent when a job is manually stopped.
  • Modify RuntimeEnvironment. Add notifications_enabled: bool = False that would indicate if notifications are available in this environment and if notification picker UI should be rendered at the frontend. notification_events: List[Type[NotificationEvent]] that would indicate the list of NotificationEvents available in this environment. By default, it includes standard events: Success, Failure, Stopped. Third parties can override this default by providing their own notification events.
  • Add notification: Optional[Notification] = None to Job and JobDefinition models, database schema
  1. Jupyterlab
  • Add notification picker UI to CreateJob screen that allows to specify send to symbols and events that would trigger notifications
  • Add Notification section to job and job definition detail screens

Rely on third parties to implement notifications delivery mechanisms (notifications delivery mechanism is out of scope of this PR).

Screen.Recording.2023-10-23.at.11.14.10.AM.mov

@andrii-i andrii-i added the enhancement New feature or request label Oct 20, 2023
@andrii-i andrii-i marked this pull request as ready for review October 23, 2023 18:05
@andrii-i andrii-i changed the title Add Notifications support Add Notification support Oct 23, 2023
@andrii-i andrii-i self-assigned this Oct 23, 2023
@andrii-i
Copy link
Collaborator Author

@Zsailer would love to get your feedback on this PR adding notifications support to Jupyter Scheduler. Please add anyone else who might be relevant.

jupyter_scheduler/models.py Outdated Show resolved Hide resolved
jupyter_scheduler/models.py Outdated Show resolved Hide resolved
jupyter_scheduler/orm.py Outdated Show resolved Hide resolved
src/components/notification-detail.tsx Outdated Show resolved Hide resolved
Comment on lines 45 to 60
<FormLabel component="legend">{trans.__('Send To')}</FormLabel>
{notification.send_to.map((email, idx) => (
<NotificationItem
key={idx}
label={trans.__(`Send To ${idx + 1}`)}
value={email}
/>
))}
<FormLabel component="legend">{trans.__('Events')}</FormLabel>
{notification.events.map((event, idx) => (
<NotificationItem
key={idx}
label={trans.__(`Event ${idx + 1}`)}
value={event}
/>
))}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI here needs work. For the emails, you should probably use a table, list of badges, or something else that doesn't take up a block element for every email. This gets hard to read after maybe 5 emails.

For the events, recall that the number of events is bounded to 3 presently. So you can just comma-join all the events and use a single field to indicate that.

Copy link
Collaborator Author

@andrii-i andrii-i Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considered different options. Decided to go with comma-separated string of values to display both send to symbols and notification events as it looks most consistent with how other values are displayed in details view:
Screenshot 2023-11-02 at 9 27 53 AM

From visual appeal point of view, I prefer chips, But if we use chips to display send to symbols (or send to and events), send to field becomes visually and logically more important than other fields. This is not always the case so always visually emphasizing send to field decreases readability:
Screenshot 2023-11-02 at 9 25 29 AM
Screenshot 2023-11-02 at 9 21 45 AM

Overall, I agree that details view design could use further improvement but it needs to be redesigned as a whole to maintain visual consistency.

src/components/notification-picker.tsx Outdated Show resolved Hide resolved
src/components/notification-picker.tsx Outdated Show resolved Hide resolved
src/components/notification-picker.tsx Outdated Show resolved Hide resolved
src/components/notification-picker.tsx Outdated Show resolved Hide resolved
src/mainviews/create-job.tsx Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member

Zsailer commented Oct 26, 2023

@akshaychitneni

@akshaychitneni
Copy link

@andrii-i thanks for adding the notification support. I think it would also be helpful to support multiple notification types like email, slack etc and encapsulate in the schema. wdyt?

@andrii-i
Copy link
Collaborator Author

@akshaychitneni, thank you for your feedback. When creating this PR, I was envisioning that third parties implementing the notification delivery mechanisms would independently decide on the notification types. However, I see the potential benefit of directly supporting various notification types backed by a schema.

I’d like to better understand your vision: imagine a scenario where users can pick anywhere from zero to several notification types for each job. Would the JSON schema fields then be meant more as informational aids to help guide their choices? Or, are you envisioning a setup where users would be actively filling out specific fields for each notification type, as laid out in the JSON schema?

@Zsailer
Copy link
Member

Zsailer commented Oct 27, 2023

When creating this PR, I was envisioning that third parties implementing the notification delivery mechanisms would independently decide on the notification types

I haven't had a chance to dive into the details of this code, but one of the general challenges we've faced with the scheduler plugin is that it's difficult to extend without re-writing a significant portion of the server extension and client plugin.

How would one go about defining their own notification type in this implementation?

Or, are you envisioning a setup where users would be actively filling out specific fields for each notification type, as laid out in the JSON schema?

Ideally, I'd love to have the job definition specified by a JSON schema defined before the server starts. The job definitions coming in from the client can be automatically validated by the backend (via pydantic, or whatever tool you like) using this schema and the forms/views in the frontend are automatically generated using the schema.

That way, this plugin can be extended/customized by the "user"—most likely this means the person deploying JupyterLab to a bunch of users who want to submit Notebook jobs—simply by extending the JSON schema. It wouldn't require code to customize, just new schema entries.

@Zsailer
Copy link
Member

Zsailer commented Oct 27, 2023

I'm thinking of something like https://jsonforms.io/

If the job definition can be specified by a (JSON) schema, this extension becomes extendable.

@andrii-i
Copy link
Collaborator Author

I haven't had a chance to dive into the details of this code, but one of the general challenges we've faced with the scheduler plugin is that it's difficult to extend without re-writing a significant portion of the server extension and client plugin.

How would one go about defining their own notification type in this implementation?

@Zsailer Thank you for looking into this. As it stands now, extension does not provide mechanisms for delivery of notifications or for defining additional properties such as notification types. RuntimeEnvironment data model has hardcoded notifications-related fields meant for configuration, notifications_enabled and notification_events. We could change RuntimeEnvironment data model and add new field, for example notification_type or a field with schema.

To set RuntimeEnvironment fields (for example, set notifications_enabled or provide an array of notification_events), 3rd parties are supposed to:

  1. Subclass CondaEnvironmentManager and override list_environments method so that it would create/return the modified RuntimeEnvironment (https://github.com/jupyter-server/jupyter-scheduler/blob/main/jupyter_scheduler/environments.py#L29)
  2. Specify custom environment manager as the value for environment_manager_class trait (https://github.com/jupyter-server/jupyter-scheduler/blob/main/jupyter_scheduler/extension.py#L48C7-L48C7)

In earlier iterations of the design I also had json schema field in the RuntimeEnvironment that allowed to define generic fields for notifications UI but we decided against proceeding with it at the time.

@andrii-i andrii-i force-pushed the notifications branch 2 times, most recently from 796545e to 4458c9c Compare October 30, 2023 22:51
@andrii-i andrii-i requested a review from dlqqq November 2, 2023 16:41
@cflann
Copy link

cflann commented Dec 12, 2023

I would love to see this feature added. Thank you @andrii-i

@krassowski
Copy link
Contributor

I agree it would be nice to have this feature. Of note argo-jupyter-scheduler implements Slack notifications. If it could hook into the new API to offer consistent UI/UX that would be amazing.

@andrii-i andrii-i marked this pull request as draft June 6, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement notifications for jobs
6 participants