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

Add changes for push_to_hub_fastai to use the new http-based approach. #1040

Merged

Conversation

nandwalritik
Copy link
Contributor

Fixes #996

This PR updates the fastai integration from git-based to the new http-based approach.

Who can review:
@Wauplin

@Wauplin
Copy link
Contributor

Wauplin commented Sep 8, 2022

Will fix #996.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 8, 2022

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

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #1040 (6741c4b) into main (59873e7) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1040      +/-   ##
==========================================
+ Coverage   83.53%   83.69%   +0.15%     
==========================================
  Files          37       37              
  Lines        3918     3919       +1     
==========================================
+ Hits         3273     3280       +7     
+ Misses        645      639       -6     
Impacted Files Coverage Δ
src/huggingface_hub/fastai_utils.py 81.37% <100.00%> (+1.56%) ⬆️
src/huggingface_hub/file_download.py 86.72% <0.00%> (+0.92%) ⬆️
src/huggingface_hub/_snapshot_download.py 94.73% <0.00%> (+1.75%) ⬆️

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

Copy link
Contributor

@merveenoyan merveenoyan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for tackling this!

src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
@nandwalritik nandwalritik force-pushed the http_based_push_to_hub_for_fastai branch from e79744c to 6e45934 Compare September 8, 2022 17:39
Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks @nandwalritik for the changes ! I think we are almost good to go 🔥

What I see left (sorry if I just realize now):

  1. Update the first line of the docstring. Current is
Upload learner checkpoint files to the Hub while synchronizing a local clone of the repo in
    :obj:`repo_id`.

which is not correct anymore since we are not synchronizing a local clone of the repo.

  1. Add a description of allow_patterns and ignore_patterns arguments in docstring:
(...)
    Use `allow_patterns` and `ignore_patterns` to precisely filter which files should be
    pushed to the hub. See [`upload_folder`] reference for more details.
(...)

Args:
(...)
        allow_patterns (`List[str]` or `str`, *optional*):
            If provided, only files matching at least one pattern are pushed.
        ignore_patterns (`List[str]` or `str`, *optional*):
            If provided, files matching any of the patterns are not pushed.
  1. It seems you have a small merge conflict to solve. You should do something like this locally:
# Sync your fork
git fetch upstream
git checkout main
git merge upstream/main

# Merge into your branch
git checkout http_based_push_to_hub_for_fastai
git merge main

# and then resolve conflicts
# and then apply linters (`make quality` and `make style`)

For more details, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork.

@nandwalritik
Copy link
Contributor Author

Thanks @nandwalritik for the changes ! I think we are almost good to go fire

What I see left (sorry if I just realize now):

  1. Update the first line of the docstring. Current is
Upload learner checkpoint files to the Hub while synchronizing a local clone of the repo in
    :obj:`repo_id`.

which is not correct anymore since we are not synchronizing a local clone of the repo.

  1. Add a description of allow_patterns and ignore_patterns arguments in docstring:
(...)
    Use `allow_patterns` and `ignore_patterns` to precisely filter which files should be
    pushed to the hub. See [`upload_folder`] reference for more details.
(...)

Args:
(...)
        allow_patterns (`List[str]` or `str`, *optional*):
            If provided, only files matching at least one pattern are pushed.
        ignore_patterns (`List[str]` or `str`, *optional*):
            If provided, files matching any of the patterns are not pushed.
  1. It seems you have a small merge conflict to solve. You should do something like this locally:
# Sync your fork
git fetch upstream
git checkout main
git merge upstream/main

# Merge into your branch
git checkout http_based_push_to_hub_for_fastai
git merge main

# and then resolve conflicts
# and then apply linters (`make quality` and `make style`)

For more details, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork.

Thanks @Wauplin for guiding, I have added the changes, let me know if I missed something.

src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
@Wauplin
Copy link
Contributor

Wauplin commented Sep 9, 2022

@nandwalritik you're welcome, thanks for working on it.
Everything's good for me. Let's see if the CI passes. I suspect that a test might break since it's using a now-deprecated argument.

Otherwise I'll merge that on Monday.

@Wauplin Wauplin merged commit b9d8617 into huggingface:main Sep 12, 2022
@Wauplin
Copy link
Contributor

Wauplin commented Sep 12, 2022

Thanks for finalizing the details @nandwalritik . I just merged it :)

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.

HTTP-based push_to_hub for fastai integration
4 participants