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

[WIP] Non-git mixin #8

Merged

Conversation

Wauplin
Copy link
Collaborator

@Wauplin Wauplin commented Aug 4, 2022

Disclaimer: I made quite some mess with the branch/fork so this PR might be temporary one that we close without merging. I created a branch wauplin-non-git-mixin from the main of the main repo and merged the branch non-git-mixin from @LysandreJik's fork. My goal was just to start with a clean state once the merge conflicts were resolved. This PR is quite huge because of the new features from the main branch. To see what huggingface#847 would be after merging see, see the diff comparison (I would rather not start a new PR from this branch to main directly just to avoid losing the previous discussion in huggingface#847).

What is done ?

  • resolve conflicts with main
  • use upload_folder in both hub and keras mixins
  • some docstring

What remains ?
- save pretrained -> signature changes to return List[str] instead of None. Does it impact other subclasses and how ? Document it (see comment).-> done
- adapt tests (and create new ones) -> done
- deprecate previous arguments (repo_path_or_name, repo_url, use_auth_token, git_user, git_email) -> done
- deprecate skip_lfs_files -> introduced in huggingface#858 . For what I understand from this comment, skip_lfs_files is now useless since we do not pull the repo anyway => to be deprecated -> done

How to deprecate arguments ?
I am still wondering what is the policy to deprecate things without breaking changes.

  1. In this change, we force the usage of keyword arguments and add the deprecation decorator at the same time. My understanding would be that in this PR/release (0.9 ?) we add the decorator but we still allow it and in next release (0.10 or 0.11 ?) we add the *to force the usage of keyword arguments. Do you confirm ?
  2. For argument deprecation, is there a utility decorator I could use same as _deprecate_positional_args ? I didn't find any but maybe you already have this in another repo ?
  3. In some cases, I don't see how we can avoid breaking changes.
    a. Will work: use_auth_token. I feel it's fine because we can take the token from either token or use_auth_token and both old and new behaviour work.
    b. Will not always work: repo_path_or_name. What if the user is passing a path to a local directory on its machine ? We cannot handle it with the new behaviour. Does it mean we still need the old implementation with Repository as well and make it live until next release ? I am a bit afraid of "dead code" if we do like this. Any opinion ?

I am also fine with merging this PR to Lysandre's repo and then continue the discussion in huggingface#847

@Wauplin Wauplin marked this pull request as ready for review August 4, 2022 07:32
@LysandreJik LysandreJik changed the base branch from non-git-mixin to main August 4, 2022 08:52
@LysandreJik LysandreJik changed the base branch from main to non-git-mixin August 4, 2022 08:52
src/huggingface_hub/hf_api.py Outdated 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
tests/test_keras_integration.py Outdated Show resolved Hide resolved
tests/test_keras_integration.py Show resolved Hide resolved
@Wauplin Wauplin changed the title [WIP] [tmp] Non-git mixin [WIP] Non-git mixin Aug 4, 2022
Copy link
Owner

@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 great! Thanks for working on this, @Wauplin.

I think we can merge this and then ask others for review on the original PR.

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hub_mixin.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/hub_mixin.py Show resolved Hide resolved
src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved
tests/test_keras_integration.py Show resolved Hide resolved
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 5, 2022

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

@Wauplin
Copy link
Collaborator Author

Wauplin commented Aug 5, 2022

This looks great! Thanks for working on this, @Wauplin.

I think we can merge this and then ask others for review on the original PR.

Thanks @LysandreJik for the review. I'll fix the few things you highlighted and then merge it to continue the discussion on the "real" PR :)

Copy link
Owner

@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 great, thanks for iterating @Wauplin!

@LysandreJik LysandreJik merged commit 134b5bd into LysandreJik:non-git-mixin Aug 8, 2022
@LysandreJik LysandreJik deleted the wauplin-non-git-mixin branch August 8, 2022 13:17
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.

3 participants