Write Mixins/Integration guide #1362

Wauplin · 2023-02-24T17:45:59Z

Resolve #1333 (from @NielsRogge 's suggestion in private slack).

This PR adds a new guide page "Integrate any ML framework with the Hub". Contains:

what is an integration?
what is the ModelHubMixin. How to use it?
concrete example with PyTorchHubMixin: how to use it and how has it been implemented

(+ edited some docstrings)

Writing this guide made me realize we can even improve further the ModelHubMixin class, especially for cases where we want to delete files from the Hub (when pushing multiple times). This is done separately in the Keras integration to replace training logs. I think we update change this once #1352 is implemented.

(Note: failing tests are unrelated to this PR)

HuggingFaceDocBuilderDev · 2023-02-24T17:50:13Z

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

osanseviero

Overall looks good! I'm not convinced of making this the officially recommended way of integrations though

docs/source/guides/integrations.mdx

osanseviero · 2023-02-27T17:33:53Z

docs/source/guides/integrations.mdx

+In most cases, a library already implements its model using a Python class. The class contains the properties of the model
+and methods to load, run, train, evaluate,... it. It would be nice to extend the class with the 2 new methods!
+
+The recommended approach to do so is to use inheritance, and more precisely mixins. A [Mixin](https://stackoverflow.com/a/547714)


Do we want to make this the official recommended approach? This is more complex/scary than the usually push to hub/download_from_hub with the HTTP endpoints and requires having classes in their libraries

stevhliu

Really nice, I love the link to the guide from the docstring!

docs/source/guides/integrations.mdx

merveenoyan

I loved the guide, just left some nits

docs/source/guides/integrations.mdx

Wauplin · 2023-02-28T16:41:46Z

Thank you very much @osanseviero @merveenoyan @stevhliu for the detailed review, it's really appreciated.

Apart from all the small suggestions that I will follow (I feel that half of the guide is rewritten, and I'm glad it is the case! 😄), the main question is "do we want to brand that as our recommended way to integration a library"? I think what I'll do is to re-arrange a bit the guide:

do not mention we have a recommended way
mention the possibility to implement simple methods (+having an example for that)
mention the advantages/drawbacks of both approached. So far I think the Mixin method is the more robust is the sense that repo creation/PR creation/revisions/... are not handled by the external library (i.e. less maintenance on their side) but at a cost of a more rigid framework
WDYT?

stevhliu · 2023-02-28T17:07:19Z

I think that's a great idea! Having a table concisely describing the pros/cons of both approaches would also be super nice to have

osanseviero · 2023-03-01T00:10:01Z

Good idea! 🔥

Co-authored-by: Omar Sanseviero <[email protected]> Co-authored-by: Steven Liu <[email protected]> Co-authored-by: Merve Noyan <[email protected]>

Co-authored-by: Merve Noyan <[email protected]> Co-authored-by: Steven Liu <[email protected]> Co-authored-by: Omar Sanseviero <[email protected]>

Co-authored-by: Steven Liu <[email protected]>

codecov · 2023-03-01T15:33:10Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (b93803f) 84.53% compared to head (d673b36) 84.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1362      +/-   ##
==========================================
- Coverage   84.53%   84.53%   -0.01%     
==========================================
  Files          48       48              
  Lines        4812     4810       -2     
==========================================
- Hits         4068     4066       -2     
  Misses        744      744

Impacted Files	Coverage Δ
src/huggingface_hub/keras_mixin.py	`94.16% <ø> (ø)`
src/huggingface_hub/hub_mixin.py	`94.04% <100.00%> (-0.14%)`	⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Wauplin · 2023-03-01T16:52:30Z

@osanseviero @stevhliu @merveenoyan Following my last comment, I made some changes to the guide:

integrated all of your feedback (:heart:)
added a section "A first approach: helpers"
kept/renamed the mixin section "A more complex approach: class inheritance". I've updated the first paragraph to make the transition with the section above.
added a "Quick comparison" section at the bottom to quickly sum up the 2 approaches. I was not very inspired about what to add to this table so I kept it quite minimal. It's not meant to be understood by someone that did not read the full guide.

Let me know if you have new feedback :)

stevhliu

Great job and the table is awesome!

docs/source/guides/integrations.mdx

stevhliu · 2023-03-01T17:26:32Z

docs/source/guides/integrations.mdx

+This is of course only an example. If you are interested in more complex manipulations (delete remote files, upload
+weights on the fly, persist weights locally,...) please refer to the [upload files](./upload) guide.
+
+### Limitations


I'd move the Limitations section above the from_pretrained method. I think it'd be nice to have a full picture of the pros/cons of using this approach before actually trying to implement it.

I understand your point but I think it could be confusing to users -at least with the text I wrote-. If I move the "limitations" section up, the reader would not really have in mind the method signatures/implementation that I already list everything that is missing from it.

What I can do is to add a sentence in the introduction to say "hey, we will see 2 different approaches, each of them have advantages/drawbacks so choose what's best for you". So that when the user reads the first approach he/she already have in mind that there are some limitations (without knowing which ones yet). What do you think?

Great idea! 👍

docs/source/guides/integrations.mdx

Co-authored-by: Steven Liu <[email protected]>

Wauplin · 2023-03-02T08:50:23Z

@stevhliu Thanks again for the detailed review! I've addressed all of your comments (see above about the limitations section). I think we are close to have a final version to merge :)

Wauplin · 2023-03-02T18:32:02Z

Merging this!

osanseviero · 2023-03-02T20:14:37Z

The table is amazing! 🔥

first draft

6f96169

Wauplin added the documentation Improvements or additions to documentation label Feb 24, 2023

Wauplin added 5 commits February 24, 2023 18:53

Merge branch 'main' into mixins-guide

f3338a9

title

edb696b

complete guide + more docstrings

4697f49

code qualirt

b0f81a1

title

175dc68

Wauplin requested review from LysandreJik, osanseviero and stevhliu February 27, 2023 11:26

Wauplin marked this pull request as ready for review February 27, 2023 11:27

Wauplin added 2 commits February 27, 2023 12:30

split sections

84109f9

docs

2ece604

osanseviero reviewed Feb 27, 2023

View reviewed changes

stevhliu approved these changes Feb 27, 2023

View reviewed changes

merveenoyan reviewed Feb 28, 2023

View reviewed changes

docs/source/guides/integrations.mdx Outdated Show resolved Hide resolved

docs/source/guides/integrations.mdx Outdated Show resolved Hide resolved

docs/source/guides/integrations.mdx Outdated Show resolved Hide resolved

docs/source/guides/integrations.mdx Outdated Show resolved Hide resolved

Merge branch 'main' into mixins-guide

fde1293

Wauplin marked this pull request as draft March 1, 2023 14:39

Wauplin and others added 4 commits March 1, 2023 15:43

Apply suggestions from code review

7051901

Co-authored-by: Omar Sanseviero <[email protected]> Co-authored-by: Steven Liu <[email protected]> Co-authored-by: Merve Noyan <[email protected]>

Apply suggestions from code review

57ada5b

Co-authored-by: Merve Noyan <[email protected]> Co-authored-by: Steven Liu <[email protected]> Co-authored-by: Omar Sanseviero <[email protected]>

Apply suggestions from code review

b810e9a

Co-authored-by: Steven Liu <[email protected]>

more requested changes

2f8d26d

Wauplin added 2 commits March 1, 2023 17:24

two approaches

26b5c12

add comparison

ed2a895

Wauplin marked this pull request as ready for review March 1, 2023 16:45

breaks

7623b9f

stevhliu reviewed Mar 1, 2023

View reviewed changes

Wauplin and others added 2 commits March 2, 2023 09:30

Apply suggestions from code review

d673b36

Co-authored-by: Steven Liu <[email protected]>

requested changes

c195585

Wauplin merged commit 3a051bd into main Mar 2, 2023

Wauplin deleted the mixins-guide branch March 2, 2023 18:32

tomaarsen mentioned this pull request Mar 14, 2023

Unable to save_pretrained model: TypeError: unsupported operand huggingface/setfit#334

Closed

Wauplin mentioned this pull request Jun 29, 2023

Docs review #818

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write Mixins/Integration guide #1362

Write Mixins/Integration guide #1362

Wauplin commented Feb 24, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Feb 24, 2023 •

edited

Loading

osanseviero left a comment

osanseviero Feb 27, 2023

stevhliu left a comment

merveenoyan left a comment

Wauplin commented Feb 28, 2023

stevhliu commented Feb 28, 2023

osanseviero commented Mar 1, 2023

codecov bot commented Mar 1, 2023 •

edited

Loading

Wauplin commented Mar 1, 2023

stevhliu left a comment

stevhliu Mar 1, 2023

Wauplin Mar 2, 2023

stevhliu Mar 2, 2023

Wauplin commented Mar 2, 2023

Wauplin commented Mar 2, 2023

osanseviero commented Mar 2, 2023

Write Mixins/Integration guide #1362

Write Mixins/Integration guide #1362

Conversation

Wauplin commented Feb 24, 2023 • edited Loading

HuggingFaceDocBuilderDev commented Feb 24, 2023 • edited Loading

osanseviero left a comment

Choose a reason for hiding this comment

osanseviero Feb 27, 2023

Choose a reason for hiding this comment

stevhliu left a comment

Choose a reason for hiding this comment

merveenoyan left a comment

Choose a reason for hiding this comment

Wauplin commented Feb 28, 2023

stevhliu commented Feb 28, 2023

osanseviero commented Mar 1, 2023

codecov bot commented Mar 1, 2023 • edited Loading

Codecov Report

Wauplin commented Mar 1, 2023

stevhliu left a comment

Choose a reason for hiding this comment

stevhliu Mar 1, 2023

Choose a reason for hiding this comment

Wauplin Mar 2, 2023

Choose a reason for hiding this comment

stevhliu Mar 2, 2023

Choose a reason for hiding this comment

Wauplin commented Mar 2, 2023

Wauplin commented Mar 2, 2023

osanseviero commented Mar 2, 2023

Wauplin commented Feb 24, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Feb 24, 2023 •

edited

Loading

codecov bot commented Mar 1, 2023 •

edited

Loading