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

[Feat] Webhook server #1410

Merged
merged 38 commits into from
Apr 14, 2023
Merged

[Feat] Webhook server #1410

merged 38 commits into from
Apr 14, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Mar 27, 2023

This PR adds a new WebhookServer object to create and run a server that can handle HF Webhooks. Is it based on top of Gradio and FastAPI. The main goal is to ease setting up a Space to handle webhooks.

Initially started the discussion on slack (internal link) cc @abidlabs @freddyaboulton @davanstrien @merveenoyan who showed some interest back in February. Please let me know if you have any feedback (design-wise, API-wise, doc-wise,...)

@LysandreJik @julien-c @osanseviero Most of the code/content is already written but before a thorough review I think it's best to wait for a high-level feedback :)

Main features:

  1. UI: the app has a UI based on Gradio. Default UI prints the endpoints and some instructions but the user can define its own UI manually.
  2. Secrets: the app handles webhook secrets. All webhook endpoints are protected with a user-defined webhook_secret (or WEBHOOK_SECRET environment variable)
  3. Debug => gradio tunnel allows for local debugging i.e. HF webhooks are processed directly on the machine => easier than debugging/editing in a Space
  4. @webhook_endpoint: 1-liner to register a function as endpoint + start server quick implementation
  5. WebhookServer: complete class for more flexibility
  6. WebhookPayload: pydantic model for auto-validation of the payload
  7. /docs: documented + testable webhooks thanks to FastAPI
  8. experimental: flagged as "experimental" => warns the user the API might change => we do not commit in maintaining it if usage doesn't grow

Examples:

  1. Use @webhook_endpoint => server is automatically started
from huggingface_hub import webhook_endpoint, WebhookPayload

@webhook_endpoint
async def trigger_training(payload: WebhookPayload):
    if payload.repo.type == "dataset" and payload.event.action == "update":
        # Trigger a training job if a dataset is updated
        ...

or

  1. Use WebhookServer for more flexibility (custom secret/ui)
import gradio as gr
from huggingface_hub import WebhookServer, WebhookPayload

with gr.Blocks as ui:
    # custom landing page for the Space
    ...

app = WebhookServer(ui=ui, webhook_secret="my_secret_key")

@app.add_webhook("/say_hello")
async def hello(payload: WebhookPayload):
    return {"message": "hello"}

app.run()

TODO:

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 27, 2023

The documentation is not available anymore as the PR was closed or merged.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Patch coverage: 82.95% and project coverage change: -0.05 ⚠️

Comparison is base (043e37e) 84.39% compared to head (1aa8986) 84.34%.

❗ Current head 1aa8986 differs from pull request most recent head 3840ff0. Consider uploading reports for the commit 3840ff0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1410      +/-   ##
==========================================
- Coverage   84.39%   84.34%   -0.05%     
==========================================
  Files          49       52       +3     
  Lines        5254     5430     +176     
==========================================
+ Hits         4434     4580     +146     
- Misses        820      850      +30     
Impacted Files Coverage Δ
src/huggingface_hub/__init__.py 75.75% <ø> (ø)
src/huggingface_hub/utils/_runtime.py 56.15% <50.00%> (-0.30%) ⬇️
src/huggingface_hub/_webhooks_server.py 73.46% <73.46%> (ø)
src/huggingface_hub/_webhooks_payload.py 98.27% <98.27%> (ø)
src/huggingface_hub/constants.py 98.14% <100.00%> (+0.03%) ⬆️
src/huggingface_hub/utils/__init__.py 100.00% <100.00%> (ø)
src/huggingface_hub/utils/_experimental.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Wauplin Wauplin changed the title [WIP] Feat webhook app [Feat] Webhook server Mar 29, 2023
@Wauplin Wauplin marked this pull request as ready for review March 29, 2023 14:38
Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

took only a very superficial glance, but looks cool

also cc @coyotte508 for webhooks context

My only suggestion is maybe rename @as_webhook_endpoint to just the simpler @webhook_endpoint

@coyotte508
Copy link
Member

Very cool!

@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 31, 2023

@coyotte508 thanks for letting me know. I added the pinned attribute.
@julien-c yeah why not. I switched to @webhook_endpoint :)

# Start Gradio App
# - as non-blocking so that webhooks can be added afterwards
# - as shared if launch locally (to debug webhooks)
self.fastapi_app, _, _ = ui.launch(prevent_thread_lock=True, share=_is_local)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can create the fastapi app used by gradio without launching the server.

app = gr.routes.App.create_app(self.ui)

I think that will make it easier to check the webhook secret via a middleware.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I don't think this will work as calling launch after adding the middleware will recreate the FastAPI app and remove the middleware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not using a middleware at all in the end. The add_middleware stuff was legacy code I didn't removed.

Good to know about the gr.routes.App.create_app possibility! But as you said I need the .launch() call afterwards so I'll not use it here.

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

Awesome PR @Wauplin. Tested and works great and the gradio usage is sound!

src/huggingface_hub/_webhooks_server.py Outdated Show resolved Hide resolved
# Start Gradio App
# - as non-blocking so that webhooks can be added afterwards
# - as shared if launch locally (to debug webhooks)
self.fastapi_app, _, _ = ui.launch(prevent_thread_lock=True, share=_is_local)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I don't think this will work as calling launch after adding the middleware will recreate the FastAPI app and remove the middleware

@Wauplin
Copy link
Contributor Author

Wauplin commented Apr 13, 2023

Thanks for reviewing @freddyaboulton! It was useful after all 😄 Glad you tested it as well 👍

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Really nice work! 👏

Super detailed docstrings, in fact, I think the docstrings may be a bit overloaded with information 😅 and some of it would be fantastic in a how-to guide. I feel like here, users are learning about Webhooks for the first time instead of using the reference as a description of what a parameter does.

docs/source/package_reference/environment_variables.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/webhooks_server.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/webhooks_server.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/webhooks_server.mdx Outdated Show resolved Hide resolved
src/huggingface_hub/_webhooks_server.py Outdated Show resolved Hide resolved
src/huggingface_hub/_webhooks_server.py Outdated Show resolved Hide resolved
src/huggingface_hub/_webhooks_server.py Outdated Show resolved Hide resolved
src/huggingface_hub/_webhooks_server.py Outdated Show resolved Hide resolved
src/huggingface_hub/_webhooks_server.py Outdated Show resolved Hide resolved
@Wauplin
Copy link
Contributor Author

Wauplin commented Apr 14, 2023

Thanks for the feedback @stevhliu !

Super detailed docstrings, in fact, I think the docstrings may be a bit overloaded with information sweat_smile and some of it would be fantastic in a how-to guide. I feel like here, users are learning about Webhooks for the first time instead of using the reference as a description of what a parameter does.

Yes, I hesitated to do a guide but felt that it would be too short. In the end I followed your advice and created this step-by-step guide. It takes major parts of the dostrings + adds a few explanations on how to deploy that to a Space.

@Wauplin
Copy link
Contributor Author

Wauplin commented Apr 14, 2023

I'm finally merging it! :)

Thanks to everyone involved 🙏

@Wauplin Wauplin merged commit 25e20ef into main Apr 14, 2023
@Wauplin Wauplin deleted the feat-webhook-app branch April 14, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants