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 windows machine for CI #1112

Merged
merged 73 commits into from
Jan 19, 2023
Merged

Add windows machine for CI #1112

merged 73 commits into from
Jan 19, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Oct 13, 2022

Fix #1074.

PR is finally green !! ✔️ ✔️ ✔️ 😄

It includes:

  • New action in GH build-windows: runs "only" normal tests (no TF/torch/LFS-specific stuff). Not so optimized so it takes 20 mins to run 😕 Could be improved to run Repository and non-repo tests separately. Only run on Python3.7 as I would expect the Python3.11 to be already fully tested on Ubuntu.
  • Windows tests are run without symlinks (even if machine supports it). Goal is to test as much as possible the "degraded" behavior of Windows.
  • Some fixes in Repocard module, especially for "\n" vs "\r\n" stuff
  • More robust temporary dir for tests (+ fixture)
  • Some tests are marked as "should fail" on windows: especially cache-related tests. I didn't want to rewrite all of them but I think we are still testing most of it. Worst case, we can update the tests in future PRs if we found the necessity.
  • file permission (umask) doesn't work the same on Windows. Should we investigate more on that ? (at the moment, test xfail)
  • not testing to kill a background process in Windows. Doesn't seem necessary + it's only for testing (running in background already works well)

and I think that's mostly it :)

TODO:

  • check again and delete pending_questions.md file
  • document known issues on Windows
  • update REGEX_YAML_BLOCK regex in moon-landing (added a \r in it)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 13, 2022

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

@Wauplin Wauplin marked this pull request as ready for review January 17, 2023 17:44
@Wauplin Wauplin marked this pull request as draft January 17, 2023 17:44
@Wauplin Wauplin marked this pull request as ready for review January 17, 2023 17:59
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 55.97% // Head: 83.58% // Increases project coverage by +27.60% 🎉

Coverage data is based on head (e342c79) compared to base (f173842).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head e342c79 differs from pull request most recent head 309b559. Consider uploading reports for the commit 309b559 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1112       +/-   ##
===========================================
+ Coverage   55.97%   83.58%   +27.60%     
===========================================
  Files          47       47               
  Lines        4700     4703        +3     
===========================================
+ Hits         2631     3931     +1300     
+ Misses       2069      772     -1297     
Impacted Files Coverage Δ
src/huggingface_hub/repocard.py 96.17% <100.00%> (+67.05%) ⬆️
src/huggingface_hub/repository.py 78.44% <100.00%> (+0.07%) ⬆️
src/huggingface_hub/__init__.py 75.75% <0.00%> (+3.03%) ⬆️
src/huggingface_hub/utils/_runtime.py 56.91% <0.00%> (+4.87%) ⬆️
src/huggingface_hub/utils/_chunk_utils.py 100.00% <0.00%> (+7.14%) ⬆️
src/huggingface_hub/_commit_api.py 92.44% <0.00%> (+12.20%) ⬆️
src/huggingface_hub/utils/_hf_folder.py 100.00% <0.00%> (+13.15%) ⬆️
src/huggingface_hub/utils/_validators.py 100.00% <0.00%> (+13.63%) ⬆️
src/huggingface_hub/_snapshot_download.py 92.45% <0.00%> (+16.98%) ⬆️
src/huggingface_hub/community.py 91.35% <0.00%> (+17.28%) ⬆️
... and 23 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Wauplin Wauplin added this to the v0.12 milestone Jan 19, 2023
Copy link
Member

@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.

Ok great! Thanks a lot for working on this super important PR @Wauplin.
Very reassuring to see that the majority of the changes had to take place within the testing code rather than within the library code.

Comment on lines 142 to 144
- `huggingface_hub`'s cache system relies on symlinks to efficiently cache files downloaded
from the Hub. On Windows, you must activate developer mode or run your script as admin to
enable symlinks. Please read [this page](./how-to-cache#limitations) for more details.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention here that if this isn't activated, it will still work, just in a non-optimized manner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea !

@@ -127,7 +128,9 @@ def save(self, filepath: Union[Path, str]):
"""
filepath = Path(filepath)
filepath.parent.mkdir(parents=True, exist_ok=True)
filepath.write_text(str(self), encoding="utf-8")
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, this didn't work as expected on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was "working" but encoding a newline was different on Windows ("\n\r") than Linux/Macos ("\n"). That's what the newline="" is for otherwise you get a big diff if you edit a README file from different platforms. And pathlib.Path.write_text doesn't have this parameter in Python 3.7 so I had to use the standard open(...).

(And yes, I discovered/learnt about all this while working on this PR :D)

@Wauplin Wauplin merged commit 070d5d6 into main Jan 19, 2023
@Wauplin Wauplin deleted the 1074-run-ci-on-windows branch January 19, 2023 15:55
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

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.

Run CI on a Windows machine as well
3 participants