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

Implement webhooks API #2209

Merged
merged 49 commits into from
May 24, 2024
Merged

Conversation

lappemic
Copy link
Contributor

@lappemic lappemic commented Apr 9, 2024

Hey @Wauplin, here's the initial draft for #1808, trying to align with #1905 as you suggested. However, a couple of points are still unclear to me, and I'd love to get your input to ensure the implementation aligns correctly:

  • Could you clarify what the secret parameter represents in create_webhook and update_webhook methods? Is it intended to be the user token?
  • Regarding the definition of DOMAIN_T Literal, would it be appropriate to define it directly within the function signatures?

I plan to finalize docstrings, module imports, and tests once we're on the same page regarding these questions.

Thanks a lot for your guidance and feedback!

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Wauplin
Copy link
Contributor

Wauplin commented Apr 9, 2024

Thanks @lappemic!

I didn't have a look yet to your changes but to answer your question:

  • the secret correspond to a value set by the user and that will be sent in every webhook. This way the user is sure the webhook is sent by the Hub. So this is not the same as token no. You can have more info here: https://huggingface.co/docs/hub/webhooks
  • if it's used only once, then you don't need to define DOMAIN_T explicitly. If used more than once, yes it's best to define it (maybe called WEBHOOK_DOMAIN_T).

Also 2 comments about the changes:

  • it's best to define a dataclass rather than a typed dict as return value
  • there are a lot of changes due to styling. Please run pip install -e '.[dev]' from your local folder to install all the required deps and then run make style to comply with huggingface_hub standard. You can run make quality to check everything's fine.

@Wauplin Wauplin changed the title first draft for discussion [wip] Implement webhooks API Apr 9, 2024
@lappemic
Copy link
Contributor Author

lappemic commented Apr 9, 2024

Thanks for the quick response, @Wauplin!

  1. Understood about the secret; it's clear now.
  2. Since WEBHOOK_DOMAIN_T is utilized more than once, I'll go ahead and define it in constants.py. Does that sound right?
  3. I'm a bit uncertain about your suggestion to prefer a dataclass over a TypedDict for return values. Could you specify which part of my changes this refers to? I would think it is abou the WatchedItem definition to do rather like this
@dataclass
class WatchedItem:
    """Data structure containing informatin about webhooks watched for updates.

    Attributes:
        type (`Literal["dataset", "model", "org", "space", "user"]`):
            Type of the item to be watched. Can be one of `["dataset", "model", "org", "space", "user"]`.
        name (`str`):
            Name of the item to be watched. Can be the username, organization name, model name, dataset name or space name.
    """

    type: Literal["dataset", "model", "org", "space", "user"]
    name: str
  1. Regarding the style changes, I had previously run make style and make quality thinking they'd align my code with Hugging Face's style guidelines. However, it seems some styling issues persist. I've run both commands again without any changes. Any clue what might be causing this?

Appreciate your guidance and looking forward to your thoughts!

@Wauplin
Copy link
Contributor

Wauplin commented Apr 9, 2024

Since WEBHOOK_DOMAIN_T is utilized more than once, I'll go ahead and define it in constants.py. Does that sound right?

Yes!

I would think it is about the WatchedItem definition to do rather like this

Exactly! That will be easier to use for users downstream

Regarding the style changes, I had previously run make style and make quality thinking they'd align my code with Hugging Face's style guidelines. However, it seems some styling issues persist. I've run both commands again without any changes. Any clue what might be causing this?

Hmm that's weird. I would assume something's off with the install. Are you using a virtualenv? If no, I'd advice to create a new one (installing Python3.9 or 3.10 in it) and install the dependencies in it. You can check install guidelines here: https://huggingface.co/docs/huggingface_hub/installation#install-with-pip (you'll need an editable install from source in a virtual env)

@lappemic
Copy link
Contributor Author

lappemic commented Apr 9, 2024

Thank you for the feedback!

I've updated the PR with the latest commits, according to your suggestions.

Regarding the styling issues, I initially followed the steps outlined in the repo's CONTRIBUTING.md without success. Then, I set up a new virtual environment and followed the installation instructions from https://huggingface.co/docs/huggingface_hub/installation#install-with-pip, which worked now. The only difference is the `[dev]' notation. I assume this should not make any difference?

Looking forward to your review and approval to continue.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks @lappemic! I'll have a closer look to it when I have time later this week. It seems that the styling still have some issues (see here) but I can take care of that if you want. Thanks again!

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @lappemic! I just reviewed it and made high level comment to it. The structure is good and I think you can continue on that. Sometimes I commented on a specific method but the comment can be applied to all comment.

As you can see, I also pushed commits to fix styling stuff. Make sure not to push changes that are unrelated to your PR, otherwise it makes the review very difficult (I have to "check" if a change is related to your PR or to some styling). If you correctly install the [dev] dependencies + run make style, it shouldn't happen. Anyway, let me know if you have questions related to this. And don't forget to git pull before starting to make changes.

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
@lappemic
Copy link
Contributor Author

Thanks you very much for the proper review and inputs @Wauplin. I hope to find time to incorporate everything this week. It might take up to next week though.

@Wauplin
Copy link
Contributor

Wauplin commented Apr 17, 2024

Thanks @lappemic! And no worries about the timing, days are always too busy 😄

@lappemic
Copy link
Contributor Author

lappemic commented Apr 25, 2024

Hey @Wauplin, found some time to implement your feedback.

I was not quite sure if enable_webhook and disable_webhook also should return the Webhook? I assumed yes, so the users gets instant feedback.

As soon as the main changes are alright, i will implement the tests and extend the documentation accordingly.

PS. Sorry for the style mess in the previous commits!! 🙈

@lappemic
Copy link
Contributor Author

lappemic commented May 2, 2024

Hi @Wauplin,

I have included the changes based on your feedback and fixed some issues in the last commits:

  1. Adjusted keyword order and making them keyword only
  2. Standardized the argument order in all WebhookInfo return values and updated corresponding docstrings, including examples
  3. Resolved serialization issue in create_webhook and update_webhook
  4. Fixed JSON parsing issue
  5. Added tests

Additionally I verified the return values of enable_webhook and disable_webhook: Both methods return a value, so I have made no changes made there.

Let me know if this looks good or if i can adjust something. Looking forward to your feedback.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Great job @lappemic! Everything looks good implementation-wise now :) I left minor comments but that's all.

Last but not least, could you add some documentation to let users know about this new feature? Here is what I would do:

In practice it means:

Please let me know if that's fine for you and if you have any questions. That would make a complete PR for a new feature, from implementation to testing to documenting it. Thanks!

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
tests/test_hf_api.py Show resolved Hide resolved
@lappemic
Copy link
Contributor Author

lappemic commented May 6, 2024

Thanks a lot @Wauplin!

I would love to finish this off with the added documentation. I ping you asap it's implemented.

@lappemic
Copy link
Contributor Author

lappemic commented May 7, 2024

@Wauplin in the last commits i have

  • renamed the guides file from webhooks_server.md to webhooks.md
  • created the "Managing Webhooks" section in the guides/webhooks.md, moved the server section and rephrased some parts to account for "Server" and "Managing" sections
  • added WebhookInfo and WebhookWatchedItem to package_reference
  • updated the _redirects.yml (Went ahead and just adopted the style) and _toctree.yml file

I first was confused about the webhooks_server part in the package_reference. But now i think there is no need for changing this part. WDYT?

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks a ton for the changes @lappemic! I finally reviewed the last detailed and pushed a few modifications in e60bbc8 (mostly to fix code styling in CI + switch the 2 sections in the guide). Looks good now so I'll merge it now. Failing tests in the CIs are unrelated to your PR. Thanks again for the hard work! This is a very nice addition! 🔥

(and sorry for the long delay, I was off in May 🤗)

@Wauplin Wauplin merged commit ce00270 into huggingface:main May 24, 2024
11 of 14 checks passed
@Wauplin Wauplin changed the title [wip] Implement webhooks API Implement webhooks API May 27, 2024
@Wauplin Wauplin mentioned this pull request May 27, 2024
@lappemic
Copy link
Contributor Author

No worries, well deserved time off i would say! 😄

Thanks a lot for the guidance and help on this @Wauplin! I really enjoyed the process!

@lappemic lappemic deleted the implement_webhooks_api branch July 8, 2024 06:59
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.

3 participants