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

Push to hub mixins that do not leverage git #847

Merged
merged 53 commits into from
Aug 17, 2022

Conversation

LysandreJik
Copy link
Member

This draft PR proposes a refactor of both mixins, ModelHubMixin and push_to_hub_keras, so that they do not leverage git under the hood. It is the continuation of #321.

This PR was done to present how it would behave. It was not done with backward-compatibility in mind, but instead by thinking of the best API, therefore with lean signatures. I recommend reading the files themselves rather than the diff as they are drastically simpler:

If we choose to go this way, a standard backward compatibility approach would be implemented.

PyTorch Keras
Relevant code: ModelHubMixin Relevant code: Keras
def push_to_hub(
    self,
    repo_id: str,
    *,
    commit_message: Optional[str] = "Add model",
    private: Optional[bool] = None,
    api_endpoint: Optional[str] = None,
    token: Optional[str] = None,
    branch: Optional[str] = None,
    config: Optional[dict] = None,
) -> str:
def push_to_hub_keras(
    model,
    repo_id: str,
    *,
    log_dir: Optional[str] = None,
    commit_message: Optional[str] = "Add model",
    private: Optional[bool] = None,
    api_endpoint: Optional[str] = None,
    token: Optional[str] = True,
    branch: Optional[str] = None,
    config: Optional[dict] = None,
    include_optimizer: Optional[bool] = False,
    task_name: Optional[str] = None,
    plot_model: Optional[bool] = True,
    **model_save_kwargs,
):
import os.path
from typing import List

import torch

from huggingface_hub import ModelHubMixin
from torch import nn


class MyModel(nn.Module, ModelHubMixin):
    def __init__(self):
        super().__init__()

        self.ln_1 = nn.Linear(10, 100)
        self.ln_2 = nn.Linear(100, 10)

    def __call__(self, *args, **kwargs):
        return self.ln_1(self.ln_2(*args, **kwargs))

    def _save_pretrained(self, save_directory) -> List[str]:
        path = os.path.join(save_directory, 'pytorch_model.bin')
        torch.save(self, path)

        return [path]


MyModel().push_to_hub(
    'Jikiwa/my-new-model', 
    commit_message="Adding a config.", 
    config={'parameter': 2}
)
import tensorflow as tf

from huggingface_hub import push_to_hub_keras


class MyModel(tf.keras.Model):
    def __init__(self, **kwargs):
        super().__init__()
        self.l1 = tf.keras.layers.Dense(2, activation="relu")
        dummy_batch_size = input_dim = 2
        self.dummy_inputs = tf.ones([dummy_batch_size, input_dim])

    def call(self, x):
        return self.l1(x)


model = MyModel()
model(tf.ones([2, 2]))
push_to_hub_keras(
    model, 
    'Jikiwa/my-new-keras-model', 
    commit_message="Committing model number 2!"
)

cc @osanseviero @NielsRogge @nateraw @merveenoyan

LysandreJik and others added 4 commits April 20, 2022 16:25
Add in commit_message + tests
Co-authored-by: Lysandre <lysandre.debut@reseau.eseo.fr>
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 20, 2022

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

@adrinjalali
Copy link
Contributor

Merging #849 should reduce the diff here.

@merveenoyan
Copy link
Contributor

merveenoyan commented Apr 25, 2022

I loved the refactor with HF API! I can open a PR for tests if you want @LysandreJik

@LysandreJik
Copy link
Member Author

Thank you for the proposal @merveenoyan, I'll likely proceed to a backward compatibility implementation + test update next week. I'll let you know if I can use assistance on any of it!

Copy link
Contributor

@nateraw nateraw left a comment

Choose a reason for hiding this comment

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

I'm excited to move towards this type of logic 😄 - here's a few comments while its still WIP.

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/hub_mixin.py Show resolved Hide resolved
src/huggingface_hub/hub_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved
@LysandreJik
Copy link
Member Author

This is on hold as another, more robust mechanism to upload files is in the making on the backend side.

@fcakyon
Copy link

fcakyon commented May 16, 2022

This is on hold as another, more robust mechanism to upload files is in the making on the backend side.

🤔 waiting for the new upload mechanism to integrate push to hub functionality to my packages!

@LysandreJik
Copy link
Member Author

Glad to hear it @fcakyon, we will let you know.

Copy link
Member Author

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

This looks good! Only left a few comments regarding documentation & error messages.

Very impressed by the tests! Super cool to use mocking, I believe this is the first occurrence of such tests in the codebase.

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/hub_mixin.py Show resolved Hide resolved
src/huggingface_hub/hub_mixin.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #847 (44d5941) into main (880408a) will increase coverage by 0.65%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
+ Coverage   80.69%   81.34%   +0.65%     
==========================================
  Files          29       29              
  Lines        3316     3378      +62     
==========================================
+ Hits         2676     2748      +72     
+ Misses        640      630      -10     
Impacted Files Coverage Δ
src/huggingface_hub/utils/endpoint_helpers.py 97.84% <ø> (ø)
src/huggingface_hub/hf_api.py 87.03% <91.66%> (+0.41%) ⬆️
src/huggingface_hub/hub_mixin.py 89.81% <100.00%> (+3.60%) ⬆️
src/huggingface_hub/keras_mixin.py 90.47% <100.00%> (-0.62%) ⬇️
src/huggingface_hub/utils/_deprecation.py 100.00% <100.00%> (+22.22%) ⬆️
src/huggingface_hub/file_download.py 85.35% <0.00%> (+0.27%) ⬆️
src/huggingface_hub/repository.py 80.55% <0.00%> (+0.34%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@LysandreJik
Copy link
Member Author

Big effort! Let's merge this, thanks for the help and all the work improving & wrapping it up @Wauplin!

@Wauplin Wauplin merged commit 17cf79a into huggingface:main Aug 17, 2022
@Wauplin Wauplin deleted the non-git-mixin branch August 17, 2022 10:28
@Wauplin
Copy link
Contributor

Wauplin commented Aug 17, 2022

Many thanks for the help and support to finally make it @LysandreJik ! 😀

@fcakyon
Copy link

fcakyon commented Aug 17, 2022

Great PR 💯

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.

10 participants