-
Notifications
You must be signed in to change notification settings - Fork 613
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
Add keras mixin #230
Add keras mixin #230
Conversation
One question! What's the connection between this and the existing |
@Rocketknight1 That's a hard-coded integration w/ transformers, AFAIK. In the future, it would probably be nice to use these Mixins directly instead and deprecate |
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.
Overall looks good, just a couple of questions
dd8f9b4
to
45c5864
Compare
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.
Solid, I like what you've done with it! And the testing suite is very nice, too. LGTM!
self._save_pretrained(save_directory) | ||
self._save_pretrained(save_directory, **kwargs) |
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.
The kwargs
are advertised as the kwargs to be passed to push_to_hub
yet they're passed to _save_pretrained
. I'd argue the kwargs to pass to each method ought to be different!
revision, | ||
cache_dir, | ||
force_download, | ||
proxies, | ||
resume_download, | ||
local_files_only, | ||
use_auth_token, |
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.
My intuition tells me all of these should be optional kwargs, even if they're only supposed to be called internally
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.
This looks great! Thanks a lot
class HubMixingCommonTest(unittest.TestCase): | ||
_api = HfApi(endpoint=ENDPOINT_STAGING) |
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.
Is this super class really needed? We only have one test class so maybe the API wrapper should be instantiated in setUpClass?
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.
You're probably right. I didn't want to mess with it too much, though. again, just following the previous file. I did find the superclass useful (if I remember right) when I played around with parametrizing the tests so we could run them over TF/PyTorch/etc.
Just a parenthesis discussion for a potential follow-up PR. Given that TF 2 suggested way of saving models is through SavedModels, should we also consider this use case? It's the recommended file format that is portable with TF Lite, TF Serving, etc. Pros
Cons
@nateraw, does the existing solution work with custom objects such as custom layers? Or will this only work if loaded with the same class? If not, I guess we can't use the created repos out of the box in the Inference API. |
cd4acc2
to
e7c4dbc
Compare
Co-authored-by: Omar Sanseviero <[email protected]>
@osanseviero I'm not sure if this supports custom layers/objects 😅 . I think we definitely want to use savedmodel going forward. This is a good first effort, but the next PR should be to use SavedModel. Didn't want to mess with this one too much just to keep things similar to whats available elsewhere in the HF ecosystem. |
Adds mixin for keras