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

Fix tests involving temp directory on macOS #2052

Merged

Conversation

bmuskalla
Copy link
Contributor

Tests that use a mixture SoftTemporaryDirectory and the fx_cache_dir fixture may fail on Mac OS.

The failure usually says something like

FAILED tests/test_file_download.py::HfHubDownloadToLocalDir::test_with_local_dir_and_symlinks_and_overwrite - OSError: [Errno 30] Read-only file system: '/tmpgucu6pwx'

The problem is that the code trying to create a symlink in _create_symlink tries to find a relative path from src to dst in

relative_src = os.path.relpath(abs_src, abs_dst_folder)

The problem is that on Mac OS, abs_src and abs_dst have a different root path due to the way the OS handles temp directories.
The one from the fixture is a resolved path to /private/var/folders/… while the one from SoftTemporaryDirectory is still an unresolved path pointing to /var/folders/ (/var/ is actually var -> private/var). And thus, their common ancestor is / which we try use to to write the symlink to (which obviously fails).

I removed resolving the path from the fixture and instead resolve the path all the time in SoftTemporaryDirectory (which is used by the fixture) so all call sites get the same resolved path.

The problem is that the code trying to create a symlink in
`_create_symlink` tries to find a relative path from src to dst in
```
relative_src = os.path.relpath(abs_src, abs_dst_folder)
```
The problem is that on macOS, abs_src and abs_dst have a different root
path due to the way the OS handles temp directories.
The one from the fixture is a resolved path to `/private/var/folders/…`
while the one from `SoftTemporaryDirectory` is still an unresolved path
pointing to `/var/folders/` (`/var/` is actually `var -> private/var`).
And thus, their common ancestor is `/` which we try use to write the
symlink to (which obviously fails).

Removed resolving the path from the fixture and instead resolve the path
all the time in `SoftTemporaryDirectory` (which is used by the fixture)
so all call sites get the same resolved path.
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.63%. Comparing base (c32602a) to head (9425c90).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2052      +/-   ##
==========================================
- Coverage   80.64%   80.63%   -0.02%     
==========================================
  Files          71       71              
  Lines        8519     8519              
==========================================
- Hits         6870     6869       -1     
- Misses       1649     1650       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 for digging into it and fixing this! I like the suggested solution like this :)
As you may have noticed we sometimes have a mix of different solutions, mostly for legacy reasons. We tend to reduce those inconsistencies but often the time + maintenance it takes to update these is just higher than the effective benefit / impact, so that's why it is how it is ^^ Fixing tests is beneficial though! 😄

@@ -40,7 +40,7 @@ def SoftTemporaryDirectory(
prefix: Optional[str] = None,
dir: Optional[Union[Path, str]] = None,
**kwargs,
) -> Generator[str, None, None]:
) -> Generator[Path, None, None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's usually dangerous to update the return value of an helper but in this case it looks fine 👍

(+1 on using Path as much as possible. Codebase is currently a mix of str and Path for legacy reasons but 🤷)

@Wauplin Wauplin merged commit 47fc293 into huggingface:main Feb 28, 2024
14 checks passed
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