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(utils): try_to_load_from_cache now correctly handles refs/pr revisions #1316

Merged
merged 7 commits into from Jan 31, 2023

Conversation

OlivierDehaene
Copy link
Member

@OlivierDehaene OlivierDehaene commented Jan 31, 2023

When trying to load a file from cache with a revision of type refs/pr/<pr-number>, try_to_load_from_cache fails because the revision file is serialized to: HUGGINGFACE_HUB_CACHE/models--<object-id>/refs/refs/pr/<pr-number> which is not found by the current implementation using os.path.listdir since it only lists files at the current level.

This PR simply reads the file directly if it exists.

Maybe this should also mean that revision should use replace("/", "--") during download.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jan 31, 2023

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

@Wauplin
Copy link
Contributor

Wauplin commented Jan 31, 2023

Thanks for reporting the issue and proposing a fix @OlivierDehaene!
Would you mind adding a test similar to this one please ? 🙏
Something like that: (not tested myself)

    def test_try_to_load_from_cache_specific_pr_revision_exists(self):
        HfApi().create_pull_request(DUMMY_MODEL_ID)
        filepath = hf_hub_download(DUMMY_MODEL_ID, filename=CONFIG_NAME, revision="refs/pr/1")
        attempt = try_to_load_from_cache(DUMMY_MODEL_ID, filename=CONFIG_NAME, revision="refs/pr/1")
        self.assertEqual(filepath, attempt)

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.

Merci ! I'll merge it as soon as the CI is green :)

@Wauplin Wauplin merged commit eedd86b into main Jan 31, 2023
@Wauplin Wauplin deleted the fix/try_to_load_from_cache branch January 31, 2023 10:47
Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

(and yes i guess we hadn't planned for revision to be allowed to contain /... cc @LysandreJik , i guess we should replace it indeed?)

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.

None yet

4 participants