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 relative symlinks in cache #1390

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Mar 13, 2023

Latest release introduced a bug in the cache (because of removing relative symlinks). I first removed the relative symlinks because we are now creating symlinks to any directory (not only within the cache). So let's use absolute paths everywhere.

This PR fixes the bug.

Reported in #1388 and huggingface/diffusers#2614. Also on discord.

(bug introduced by #1360)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 13, 2023

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

@Wauplin Wauplin requested a review from LysandreJik March 13, 2023 17:19
@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 13, 2023

Failing tests are unrelated to this PR (should be fixed/skipped by #1381).

@@ -558,7 +558,7 @@ def cached_download(
token: Union[bool, str, None] = None,
local_files_only: bool = False,
legacy_cache_layout: bool = False,
) -> Optional[str]: # pragma: no cover
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrelated to the fix so I can remove it if you strongly feel against it but otherwise I'll keep it (it just fix a type annotation, without any breaking change)

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.

Good catch, and nice test!

I ran the test on current main and had a failure, which passed when checking out the PR.

LGTM

@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 13, 2023

@LysandreJik thanks for carefully reviewing it! I'll make a patch release

@Wauplin Wauplin merged commit cdaa410 into main Mar 13, 2023
@Wauplin Wauplin deleted the fix-cache-relative-symlink-issue branch March 13, 2023 17:41
Wauplin added a commit that referenced this pull request Mar 13, 2023
* fix relative symlinks in cache

* fix test

* fix test on windows
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