Skip to content

📝 Document webhooks #554

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 26 commits into from
Jan 11, 2023
Merged

📝 Document webhooks #554

merged 26 commits into from
Jan 11, 2023
+197 −0

Conversation

@coyotte508
Copy link
Member

@coyotte508 coyotte508 commented Dec 13, 2022 •
edited
Loading

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Dec 13, 2022 •
edited
Loading

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

julien-c
julien-c reviewed Dec 13, 2022
View reviewed changes
julien-c
julien-c reviewed Dec 13, 2022
View reviewed changes
julien-c
julien-c reviewed Dec 13, 2022
View reviewed changes
julien-c
julien-c reviewed Dec 13, 2022
View reviewed changes
coyotte508 and others added 2 commits December 13, 2022 17:09
@coyotte508 @julien-c
Update docs/hub/webhooks.md
b9dca40 
Co-authored-by: Julien Chaumond <[email protected]>
@coyotte508 @julien-c
Update docs/hub/webhooks.md
0d7a9f2 
Co-authored-by: Julien Chaumond <[email protected]>
julien-c
julien-c reviewed Dec 13, 2022
View reviewed changes
julien-c
julien-c reviewed Dec 13, 2022
View reviewed changes
SBrandeis
SBrandeis reviewed Dec 13, 2022
View reviewed changes
SBrandeis
SBrandeis reviewed Dec 13, 2022
View reviewed changes
@coyotte508 @SBrandeis
Update docs/hub/webhooks.md
868df3e 
Co-authored-by: Simon Brandeis <[email protected]>
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
coyotte508 and others added 2 commits December 13, 2022 17:50
@coyotte508 @patrickvonplaten
Update docs/hub/webhooks.md
ccd7b17 
Co-authored-by: Patrick von Platen <[email protected]>
@coyotte508 @patrickvonplaten
Update docs/hub/webhooks.md
66930b8 
Co-authored-by: Patrick von Platen <[email protected]>
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Dec 13, 2022
View reviewed changes
Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Take my feedback with a grain of salt. My front-end / networks knowledge is quite limited and I spent 0 time on googling any of the words / definitions.

For me a full-on walk-through of:

  1. Set up a webhook
  2. An example of how this webhook is triggered

Possible with some screenshots in-between would be super useful

sanchit-gandhi
sanchit-gandhi reviewed Dec 13, 2022
View reviewed changes
Copy link
Contributor

@sanchit-gandhi sanchit-gandhi left a comment

Choose a reason for hiding this comment

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

Also not sure if I fall under your target audience! But if I do, I'd reiterate what Patrick has suggested and try and make the doc a bit more 'interactive' (either more examples or a full walkthrough)

The examples you've listed for Repo, Discussion and Comment are useful - they were the bits that were most clear for me. Until that point, it's a bit abstract and harder to understand from an ML practitioner point of view.

xianbaoqian
xianbaoqian reviewed Dec 14, 2022
View reviewed changes
@coyotte508 @julien-c
Update docs/hub/webhooks.md
22897e7 
Co-authored-by: Julien Chaumond <[email protected]>
@Narsil
Copy link

Narsil commented Dec 14, 2022

Overall I think it's a good document (and agree with all the comments still open).

What I think could take this the extra mile would be to do a tutorial of a full fledge hook. Like let's say I need to update my API when the model is modified ?

@coyotte508
Copy link
Member Author

coyotte508 commented Dec 14, 2022

Overall I think it's a good document (and agree with all the comments still open).

What I think could take this the extra mile would be to do a tutorial of a full fledge hook. Like let's say I need to update my API when the model is modified ?

And the hook could be hosted on a space!

I have something similar planned but it's not ML related (deploying a website on code changes), I think something ML-related would be better.

@julien-c
Copy link
Member

julien-c commented Dec 14, 2022

my thinking is that we can first do a "private beta" shipping of this (maybe using a hardcoded org like spaces-explorers when Spaces were in private beta) and get some team members or external fellows to experiment with this, and contribute some guides. WDYT?

@coyotte508
Copy link
Member Author

coyotte508 commented Dec 14, 2022

my thinking is that we can first do a "private beta" shipping of this (maybe using a hardcoded org like spaces-explorers when Spaces were in private beta) and get some team members or external fellows to experiment with this, and contribute some guides. WDYT?

not PRO users? x)

@coyotte508
Copy link
Member Author

coyotte508 commented Dec 14, 2022

Thank you @patrickvonplaten @xianbaoqian @sanchit-gandhi for your suggestions!

I rewrote the docs taking into account your comments and adding a few screenshots, it's previewable at https://moon-ci-docs.huggingface.co/docs/hub/pr_554/en/webhooks.

I also added a full payload as an example, and changed event and domain to event.action and event.scope. The scope is now a single string, eg repo.config (instead of ["repo", "config"])

philschmid
philschmid reviewed Dec 16, 2022
View reviewed changes
Copy link
Contributor

@philschmid philschmid left a comment

Choose a reason for hiding this comment

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

Looks amazing. 😍 This will allow us to build many cool features next year and examples for real MLOps automation.

Two question from my side are:

  1. Will the API be public to create webhooks? and will it then be integrated into huggingface_hub?
  2. Is there a way to identify the webhook request to a hub user programmatically?, e.g. whoisthis?secret=SECRET. => returns the userId
    -> This would allow us to add a "route" to the endpoints (UI), /webhooks to automatically rollout deployments.

@coyotte508
Copy link
Member Author

coyotte508 commented Dec 16, 2022 •
edited
Loading

  1. Will the API be public to create webhooks? and will it then be integrated into huggingface_hub?

Haha cc @Wauplin

See comment on original PR: https://github.com/huggingface/moon-landing/pull/4477#discussion_r1048361481

Opened an issue: https://github.com/huggingface/moon-landing/issues/4888. It'll be easy to make it public once it's live.

  1. Is there a way to identify the webhook request to a hub user programmatically?, e.g. whoisthis?secret=SECRET. => returns the userId
    -> This would allow us to add a "route" to the endpoints (UI), /webhooks to automatically rollout deployments.

We can add whoisthis?webhookId=XXX, since webhook.id will be passed in the payload.

Also created https://github.com/huggingface/moon-landing/issues/4889

@Wauplin
Copy link
Contributor

Wauplin commented Dec 16, 2022 •
edited
Loading

Will the API be public to create webhooks? and will it then be integrated into huggingface_hub?

Thanks for the ping. Actually yes it could be but not the priority for now.

@coyotte508 coyotte508 marked this pull request as ready for review December 16, 2022 09:08
osanseviero
osanseviero reviewed Dec 18, 2022
View reviewed changes
Copy link
Contributor

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Nice!

docs/hub/webhooks.md

`event.scope` can take one of the following values:

- `"repo"` - Global events on repos. Possible values for the associated `action`: `"create"`, `"delete"`, `"update"`, `"move"`.
Copy link
Contributor

@osanseviero osanseviero Dec 18, 2022

Choose a reason for hiding this comment

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

I wonder if a better format for this would be a table with three columns: event.scope value, description, associated actions

docs/hub/webhooks.md Outdated

## Rate limiting

Each webhook is limited to 1,000 triggers per 24 hours. You can check the daily triggers for your webhook in your webhook settings, in the "Activity" tab.
Copy link
Contributor

@osanseviero osanseviero Dec 18, 2022

Choose a reason for hiding this comment

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

Maybe add a link here for the activity tab

Copy link
Member Author

@coyotte508 coyotte508 Dec 19, 2022

Choose a reason for hiding this comment

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

not possible - the activity tab is hf.co/settings/webhooks/xxxx/activity , it depends on the individual webhook's id

coyotte508 and others added 3 commits December 19, 2022 11:58
@coyotte508 @osanseviero
Update docs/hub/webhooks.md
75ab75a 
Co-authored-by: Omar Sanseviero <[email protected]>
@coyotte508 @osanseviero
Update docs/hub/webhooks.md
7c400b5 
Co-authored-by: Omar Sanseviero <[email protected]>
@coyotte508 @osanseviero
Apply suggestions from code review
23f96c4 
Co-authored-by: Omar Sanseviero <[email protected]>
julien-c
julien-c reviewed Dec 20, 2022
View reviewed changes
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.

BTW did you see there's an existing api-webhook page

https://huggingface.co/docs/hub/api-webhook

Might make sense to redirect that old one to the new one

Slightly more concerning, that no-one ever contacted us to get access to the webhook hahaha 😹

gary149
gary149 reviewed Dec 26, 2022
View reviewed changes
Copy link
Contributor

@gary149 gary149 left a comment

Choose a reason for hiding this comment

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

Made a batch of copywriting suggestions.

SBrandeis
SBrandeis approved these changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@SBrandeis SBrandeis left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@coyotte508 coyotte508 merged commit 82f0d1e into main Jan 11, 2023
@coyotte508 coyotte508 deleted the webhooks branch January 11, 2023 10:18
@julien-c
Copy link
Member

julien-c commented Jan 20, 2023

please review also the related #627

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julien-c julien-c julien-c left review comments

@gary149 gary149 gary149 left review comments

@xianbaoqian xianbaoqian xianbaoqian left review comments

@SBrandeis SBrandeis SBrandeis approved these changes

+4 more reviewers

@osanseviero osanseviero osanseviero left review comments

@patrickvonplaten patrickvonplaten patrickvonplaten left review comments

@philschmid philschmid philschmid left review comments

@sanchit-gandhi sanchit-gandhi sanchit-gandhi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.