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

Deprecation of pushing arguments for cleaner DX in Keras mixin #846

Closed
merveenoyan opened this issue Apr 20, 2022 · 2 comments
Closed

Deprecation of pushing arguments for cleaner DX in Keras mixin #846

merveenoyan opened this issue Apr 20, 2022 · 2 comments

Comments

@merveenoyan
Copy link
Contributor

Currently push_to_hub_keras looks like this:

def push_to_hub_keras(
    model,
    repo_path_or_name: Optional[str] = None,
    repo_url: Optional[str] = None,
    commit_message: Optional[str] = "Add model",
    organization: Optional[str] = None,
    private: Optional[bool] = None,
    api_endpoint: Optional[str] = None,
    use_auth_token: Optional[Union[bool, str]] = True,
    git_user: Optional[str] = None,
    git_email: Optional[str] = None,
    config: Optional[dict] = None,
    include_optimizer: Optional[bool] = False,
    **model_save_kwargs,
):

But it's not intuitive and clean. Would be better if we'd do

def push_to_hub_keras(
    model,
    repo_id: str,
    commit_message: Optional[str] = "Add model",
    private: Optional[bool] = None,
    use_auth_token: Optional[Union[bool, str]] = True,
    config: Optional[dict] = None,
    include_optimizer: Optional[bool] = False,
    **model_save_kwargs,
):

As put by @osanseviero here.
We need to deprecate a couple of arguments and put FutureWarnings if provided and then remove completely.
Pinging @ariG23498 for discussion 🤗

@LysandreJik
Copy link
Member

Nice proposal, I agree that what you propose here is much cleaner! It is similar to what is proposed in #847, with some additional arguments stripped away. I'd be very eager to hearing your feedback and proposals on that PR!

@Wauplin
Copy link
Contributor

Wauplin commented Sep 30, 2022

Since #847 is merged, previous signature is deprecated and will change in v0.12 to something very similar to what is proposed here (see comment here).

Therefore I'm closing this issue :)

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

No branches or pull requests

3 participants