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

FlyIn (VSCode) Plugin Notification #1953

Open
wants to merge 56 commits into
base: master
Choose a base branch
from

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Nov 12, 2023

Tracking issue

Fixes flyteorg/flyte#4284

Why are the changes needed?

In some cases, users want to notify themselves before the vscode server is terminated.

What changes were proposed in this pull request?

  • add a class BaseNotifier, for creating a notifier like SendgridNotifier or SlackNotifier
  • add a class NotifierExecutor, for handling cases when messages need to be sent
  • add warning_seconds_before_termination in the vscode decorator, for deciding when to send notification to users
  • add notifier in the vscode decorator, for sending email by specific services. (e.g. sendgrid and slack)
  • add sendgrid notifier and slack notifier
  • add tests for above
    image

How was this patch tested?

  • Send sendgrid notification and slack notification
  • unit test

Setup process

  1. start the flyte cluster
  2. build an image with this PR's latest gitsha
  3. add secret to flytesnacks-development using kubectl (default secret mount place in flyte)
  4. run a task in flyte cluster with vscode decorator and notifier
flytectl demo start --disable-agent --force
docker buildx build -t localhost:30000/flyin-notification:1655 -f DockerfileFlyin_v2 . --load
kubectl create secret generic sendgrid-api \
    --from-literal=token="sendgrid-api-key" \
    -n flytesnacks-development

kubectl create secret generic slack-api \
    --from-literal=token="slack-api-key" \
    -n flytesnacks-development```
pyflyte run --remote vscode_task.py wf
from flytekit import Secret, task, workflow
from flytekitplugins.flyin import vscode, VscodeConfig, SendgridNotifier, SendgridConfig, SlackNotifier, SlackConfig

sengrid_notifier = SendgridNotifier(sendgrid_conf=SendgridConfig(
                    from_email="xxx@gmail.com",
                    to_email="xxxxx@xxx.tw",
                    secret_group="sendgrid-api",
                    secret_key="token",
                ))
slack_notifier = SlackNotifier(slack_conf=SlackConfig(
                        channel="demo",
                        secret_group="slack-api",
                        secret_key="token",
                    ))

@task(
    container_image="localhost:30000/flyin-notification:1655",
    secret_requests=[Secret(key="token", group="sendgrid-api"),
                    Secret(key="token", group="slack-api"),   ],
)
@vscode(
    config=VscodeConfig(),
    max_idle_seconds=80,
    warning_seconds_before_termination=60,
    notifier=slack_notifier,
)
def t(a: int, b: int) -> int:
    return a + b

@workflow
def wf(a: int = 5, b: int = 3) -> int:
    out = t(a=a, b=b)
    return out
FROM python:3.10-slim-buster

MAINTAINER Flyte Team <users@flyte.org>
LABEL org.opencontainers.image.source https://github.com/flyteorg/flytekit
WORKDIR /root
ENV PYTHONPATH /root

ARG VERSION
ARG TARGETARCH

# 1. Update the necessary packages for flytekit
# 2. Install code-server
# 3. Download code-server extensions for Python and Jupyter via wget
# 4. Install flytekit and flytekit-flyin with no cache
# 5. Delete apt cache. Reference: https://gist.github.com/marvell/7c812736565928e602c4
# 6. Some packages will create config file under /home by default, so we need to make sure it's writable
# 7. Change the permission of /tmp, so that others can run command on it
RUN apt-get update \
    && apt-get install build-essential git wget -y \
    && mkdir -p /tmp/ \
    && mkdir -p /tmp/code-server \
    && wget --no-check-certificate -O /tmp/code-server/code-server-4.19.0-linux-${TARGETARCH}.tar.gz https://github.com/coder/code-server/releases/download/v4.19.0/code-server-4.19.0-linux-${TARGETARCH}.tar.gz \
    && tar -xzf /tmp/code-server/code-server-4.19.0-linux-${TARGETARCH}.tar.gz -C /tmp/code-server/ \
    && wget --no-check-certificate https://open-vsx.org/api/ms-python/python/2023.20.0/file/ms-python.python-2023.20.0.vsix -P /tmp/code-server \
    && wget --no-check-certificate https://open-vsx.org/api/ms-toolsai/jupyter/2023.9.100/file/ms-toolsai.jupyter-2023.9.100.vsix -P /tmp/code-server
RUN pip install -U git+https://github.com/Future-Outlier/flytekit.git@f24bcf1b854931bd20d1e73db1f085d5fe0a5362 \
    && pip install -U git+https://github.com/Future-Outlier/flytekit.git@f24bcf1b854931bd20d1e73db1f085d5fe0a5362#subdirectory=plugins/flytekit-flyin \
    && apt-get clean autoclean \
    && apt-get autoremove --yes \
    && rm -rf /var/lib/{apt,dpkg,cache,log}/ \
    && useradd -u 1000 flytekit \
    && chown -R flytekit:flytekit /tmp/code-server \
    && chown flytekit: /root \
    && chown flytekit: /home \
    && :

# Set the environment variable for code-server
ENV PATH="/tmp/code-server/code-server-4.19.0-linux-${TARGETARCH}/bin:${PATH}"

USER flytekit

# Install extensions using code-server
# Execution is performed here as code-server configuration depends on the USER setting
# If we install it as ROOT, the config will be stored in /root/.config/code-server/config.yaml
# Now, the config of code-server will be stored in /home/flytekit/.config/code-server/config.yaml
RUN code-server --install-extension /tmp/code-server/ms-python.python-2023.20.0.vsix \
    && code-server --install-extension /tmp/code-server/ms-toolsai.jupyter-2023.9.100.vsix

Screenshots

Sendgrid

image
image

Slack

image

image

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Future Outlier added 2 commits November 12, 2023 12:49
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (892b474) 85.77% compared to head (bbace16) 85.53%.
Report is 1 commits behind head on master.

Files Patch % Lines
...lyin/flytekitplugins/flyin/vscode_lib/decorator.py 21.42% 11 Missing ⚠️
...ytekitplugins/flyin/notification/slack_notifier.py 65.21% 8 Missing ⚠️
...lytekitplugins/flyin/notification/base_notifier.py 82.60% 4 Missing ⚠️
...kitplugins/flyin/notification/sendgrid_notifier.py 91.17% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1953      +/-   ##
==========================================
- Coverage   85.77%   85.53%   -0.25%     
==========================================
  Files         313      309       -4     
  Lines       23500    23017     -483     
  Branches     3512     3512              
==========================================
- Hits        20158    19687     -471     
+ Misses       2734     2722      -12     
  Partials      608      608              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Future Outlier added 21 commits November 15, 2023 21:46
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
nit
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Future Outlier added 8 commits November 29, 2023 12:43
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Signed-off-by: Future Outlier <eric901201@gmai.com>
Future Outlier added 11 commits January 16, 2024 11:55
…code-plugin-notification

Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
Signed-off-by: Future-Outlier <eric901201@gmai.com>
@Future-Outlier Future-Outlier marked this pull request as ready for review January 17, 2024 06:52
@Future-Outlier Future-Outlier marked this pull request as draft January 17, 2024 06:53
Signed-off-by: Future-Outlier <eric901201@gmai.com>
@Future-Outlier Future-Outlier marked this pull request as ready for review January 17, 2024 07:27
Signed-off-by: Future-Outlier <eric901201@gmai.com>
@kumare3
Copy link
Contributor

kumare3 commented Jan 17, 2024

My preference is to have a node or a task notification logic and uses the central notification system.
Instead of this one off can we send notification through admin events.

Granted this will need a bigger change, but I s extremely useful for many other scenarios, we can use it for HITL, or signal nodes too.

Sample

'''
@task(notification=...)
Def tk(...):
...

@Future-Outlier
Copy link
Member Author

My preference is to have a node or a task notification logic and uses the central notification system.
Instead of this one off can we send notification through admin events.

Granted this will need a bigger change, but I s extremely useful for many other scenarios, we can use it for HITL, or signal nodes too.

Sample

'''
@task(notification=...)
Def tk(...):
...

Thanks a lot, will think about this problem tomorrow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants