-
Notifications
You must be signed in to change notification settings - Fork 443
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: Add ability to upload PT/TF models to Huggingface Hub #881
Conversation
Codecov Report
@@ Coverage Diff @@
## main #881 +/- ##
==========================================
- Coverage 94.92% 94.69% -0.23%
==========================================
Files 133 135 +2
Lines 5279 5355 +76
==========================================
+ Hits 5011 5071 +60
- Misses 268 284 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Depends on: #883 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this amazing feature, for the location I think it might be better to include it in the package in a models//factory folder or in a models/factory folder as @fg-mindee did for the from_hub() method (since it is a symmetrical operation and can be decorrelated from training).
What do you think ?
@charlesmindee Ok i have moved it to models/factory so it is for both TF and PT we do not need to split this :) But ... any idea how we can test this ? 😅 I am really not sure how without a test account 😃 |
There was a problem hiding this 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 Felix 🙏
Regarding the general idea of this feature, it needs to be complementary to "from_hub". For this, I would suggest that we try to find a way to imbue the config structure by default in the function. (Perhaps passing the content as args or kzwargs and creating the dictionary in the function)
Either way, this function needs to have a reduced minimal snippet to push a model in a shell.
So I'd argue that we need to ensure that :
- we create models with a comprehensive config attribute
- we use that attribute to create the config and push to hub
- this config is then used with from_hub to recreate the model
About testing, let's narrow down what we want to test:
- the HF api is well tested, so we need to focus on our own lines of code
- the process here is to create a dictionary, dump parameters, create a git repo with it and push it
- with temporary dirs of pytest, I'd argue we can go up until the creation of the repo
- the question now is how easily we can split those steps
What do you think? 🙂
@frgfm For the config part:
Failing tests currently depends on #883 merge and 2 tests in TF about get_layer i will check this later |
23f95ee
to
b23399e
Compare
@frgfm @charlesmindee |
@charlesmindee can you follow up on the discussion FG started for this PR please? |
@charlesmindee failing test has nothing to do with this PR please trigger again if we are fine with the other stuff :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests & the edits!
Thanks for the PR @felixdittrich92 🎉 |
Missing "type: new feature", "module: models", "ext: references" and "ext: test" as PR labels I think @charlesmindee :) |
This PR is a follow up on @fg-mindee object detection
from_hub
and adds the following functionality:--push-to-hub
as flag to your training scriptAny feedback is very welcome 🤗
@osanseviero Suggestions from your side as to whether we have forgotten anything or can do better ? 😄
@fg-mindee @charlesmindee
I am not sure if it is the correct place for the file ..maybe add in doctr.utils.hub to add some tests??
Example Uploads:
https://huggingface.co/Felix92/Test-recognition-PyTorch
https://huggingface.co/Felix92/Test-recognition-TensorFlow
Note: PR include cfg fix for TF Resnet50 and FasterRCNN link to #883 (sry for the mistake 😅 )