Add metadata support with tests #155

ardaatahan · 2025-01-15T17:17:25Z

This PR aims to bring over some of huggingface_hub Python library's capabilities to swift-transformers:

Adds file metadata support to prevent unnecessary redownloads, ensure file integrity, and update to the latest file versions when needed.
Disables automatic redirection during head requests to fix getFileMetadata, which currently doesn't return expected commit hash and etag for lfs files.
Adds additional tests to verify metadata handling and download logic.

pcuenca

Looks great, and sorry for the delay in getting to this PR 🙏

I think there are a couple of inconsistencies that should be straightforward to fix. I also suggested an additional test to verify metadata reuse.

More generally, I think we should exactly mimic the cache structure of the Hub CLI because we're almost there anyway, and it could potentially be useful down the line.

Great work, thanks a lot! 🙌

pcuenca · 2025-01-29T11:29:34Z

Sources/Hub/Downloader.swift

@@ -11,6 +11,9 @@ import Combine

 class Downloader: NSObject, ObservableObject {
    private(set) var destination: URL
+    private(set) var metadataDestination: URL


Is this used by the Downloader at all, or is it a remnant of deleted commits?

remnant of deleted commits, just reverted this to the original

pcuenca · 2025-01-29T11:30:10Z

Sources/Hub/HubApi.swift

+        case consistencyError(String)
+        case diskSpaceError(String)
+        case permissionError(String)


I think these are unused

Sources/Hub/HubApi.swift

pcuenca · 2025-01-29T11:31:59Z

Sources/Hub/HubApi.swift

+        }
+
+        func isValidSHA256(_ hash: String) -> Bool {
+            let sha256Pattern = "^[0-9a-f]{64}$"


Suggested change

let sha256Pattern = "^[0-9a-f]{64}$"

let sha256Pattern = "^[0-9a-f]{40}$"

I think?

64 should be correct. Reference: https://github.com/huggingface/huggingface_hub/blob/2fdc6f48ef5e6b22ee9bcdc1945948ac070da675/src/huggingface_hub/file_download.py#L81

Ah, you are right, but that's for SHA256 etags that represent LFS files. Commit hashes will never match this, we have to use this pattern instead.

Sources/Hub/HubApi.swift

Tests/HubTests/HubApiTests.swift

pcuenca · 2025-01-29T11:44:56Z

Sources/Hub/HubApi.swift

+        /// Reference: https://github.com/huggingface/huggingface_hub/blob/b2c9a148d465b43ab90fab6e4ebcbbf5a9df27d4/src/huggingface_hub/_local_folder.py#L263
+        ///
+        /// - Parameters:
+        ///   - localDir: The local directory where files are downloaded.


Suggested change

/// - localDir: The local directory where files are downloaded.

/// - localDir: The local directory where metadata files are downloaded.

Tests/HubTests/HubApiTests.swift

ardaatahan · 2025-01-30T23:00:46Z

Thanks for the review!

I'm considering adding offline mode support under HubApi, controlled by a boolean flag in the constructor. If the flag isn’t provided, it could default to using something like isExpensive as a fallback.

This would allow us to return already downloaded files after verification without requiring a new download when on cellular data.

Curious to hear your thoughts!

pcuenca · 2025-01-31T10:37:56Z

Tests/HubTests/HubApiTests.swift

@@ -167,7 +167,7 @@ class HubApiTests: XCTestCase {
 }

 class SnapshotDownloadTests: XCTestCase {
-    let repo = "coreml-projects/Llama-2-7b-chat-coreml"
+    var repo = "coreml-projects/Llama-2-7b-chat-coreml"


Let's move this back to let and update testDownloadSmolLargeFile to define a new ivar.

pcuenca

I think there's a confusion with the hash validation of the commit hashes versus the etags of LFS files.

I would suggest to:

Add another validation function for commit hashes (40 chars)
Write a test (if possible, it may be convoluted) that retrieves metadata from an LFS file and verifies that:
- The commit hash matches the commit validation function
- The etag hash matches the isValidSHA256 function

pcuenca · 2025-01-31T11:45:56Z

Sources/Hub/HubApi.swift

+        }
+
+        func isValidSHA256(_ hash: String) -> Bool {
+            let sha256Pattern = "^[0-9a-f]{64}$"


Ah, you are right, but that's for SHA256 etags that represent LFS files. Commit hashes will never match this, we have to use this pattern instead.

pcuenca · 2025-01-31T11:48:24Z

Sources/Hub/HubApi.swift

+            let remoteCommitHash = remoteMetadata.commitHash ?? ""
+
+            // Local file exists + metadata exists + commit_hash matches => return file
+            if isValidSHA256(remoteCommitHash) && downloaded && localMetadata != nil && localCommitHash == remoteCommitHash {


The isValidSHA256 check will always fail here on the remoteCommitHash.

pcuenca · 2025-01-31T11:48:55Z

Sources/Hub/HubApi.swift

+                // => means it's an LFS file (large)
+                // => let's compute local hash and compare
+                // => if match, update metadata and return file
+                if localMetadata != nil && isValidSHA256(remoteEtag) {


This isValidSHA256 will succeed when it's an LFS file.

pcuenca · 2025-01-31T11:56:57Z

I'm considering adding offline mode support under HubApi, controlled by a boolean flag in the constructor. If the flag isn’t provided, it could default to using something like isExpensive as a fallback.

This would allow us to return already downloaded files after verification without requiring a new download when on cellular data.

Sure, sounds great! Maybe get this merged first and then open a new PR, to make things simpler?

ardaatahan · 2025-01-31T19:12:47Z

I'm considering adding offline mode support under HubApi, controlled by a boolean flag in the constructor. If the flag isn’t provided, it could default to using something like isExpensive as a fallback.
This would allow us to return already downloaded files after verification without requiring a new download when on cellular data.

Sure, sounds great! Maybe get this merged first and then open a new PR, to make things simpler?

Sounds good!

ardaatahan · 2025-01-31T19:54:44Z

On a different note: after these changes get merged, calling snapshot will trigger a lot of redownloads on mobile devices due to the absence of existing metadata files. This is expected behavior, but please be mindful of the potential impact on bandwidth and storage usage.

pcuenca · 2025-01-31T20:39:44Z

calling snapshot will trigger a lot of redownloads

ah, you are right. Do you think we should add the offline mode first then? And/or tag this as a new version 0.2.0.

ardaatahan · 2025-02-02T18:19:45Z

ah, you are right. Do you think we should add the offline mode first then? And/or tag this as a new version 0.2.0.

I'm curious about your opinion on changing the download logic to first download metadata files if they're missing (and redownload non-lfs files), then only redownload lfs files if their checksums don't match the etag or if they're missing. This could safeguard against high bandwidth usage without enabling offline mode, which would completely restrict any new downloads

pcuenca · 2025-02-02T18:35:00Z

That sounds like a great idea!

ardaatahan · 2025-02-03T17:58:42Z

Just made the changes and added extra test cases!

ZachNagengast · 2025-02-03T18:39:37Z

With these changes, full LFS files will still potentially be downloaded when calling snapshot, but only if they don't match the remote hash, which is good because the local model is stale in that case. I think this is safe to merge before offline mode since current behavior of snapshot is already expected to make a decision on downloading the full file internally.

Once offline mode is implemented, devs will be able to safely call snapshot everytime with an extra offline mode parameter that simply checks if the local files are valid based on the stored metadata. This is useful for the case that a device is on an expensive or limited cell connection, and devs want to make a decision (or even prompt the user) if it is ok to download in this state, or just continue to use a stale model in offline mode until on a better connection.

ardaatahan and others added 12 commits December 28, 2024 20:33

add metadata and resumable download support with tests

02d2571

fix download progress handling

f590932

fix httpHead by disabling redirection for lfs

22b6892

remove unnecessary test case

bedfc7a

fix metadata location for lfs files and relative redirects

af26e60

add test case for large file metadata

b4e1c49

preserve original repo file structure for metadata files

26707b8

add test case for lfs and more comments

5839d33

Minor cleanup

9d39cf1

Merge branch 'main' into metadata

97b6163

Remove resumable downloads for future PR

fe2f32b

Cleanup unused variable

30adb75

pcuenca mentioned this pull request Jan 26, 2025

Enable tool use #151

Merged

pcuenca reviewed Jan 29, 2025

View reviewed changes

Tests/HubTests/HubApiTests.swift Show resolved Hide resolved

ardaatahan added 2 commits January 30, 2025 13:24

fix metadata download path and add tests

2869c8f

Merge branch 'main' into metadata

ff89442

ardaatahan requested a review from pcuenca January 30, 2025 23:00

pcuenca reviewed Jan 31, 2025

View reviewed changes

add and test separate validation for commit hashes

0e30b28

ardaatahan requested a review from pcuenca January 31, 2025 19:24

pcuenca approved these changes Jan 31, 2025

View reviewed changes

only redownload lfs files when missing or checksum does not match

7290768

ardaatahan requested a review from pcuenca February 3, 2025 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metadata support with tests #155

Add metadata support with tests #155

ardaatahan commented Jan 15, 2025

pcuenca left a comment

pcuenca Jan 29, 2025

ardaatahan Jan 30, 2025

pcuenca Jan 29, 2025

pcuenca Jan 29, 2025

ardaatahan Jan 30, 2025

pcuenca Jan 31, 2025

pcuenca Jan 29, 2025

ardaatahan commented Jan 30, 2025

pcuenca Jan 31, 2025

pcuenca left a comment

pcuenca Jan 31, 2025

pcuenca Jan 31, 2025

pcuenca Jan 31, 2025

pcuenca commented Jan 31, 2025

ardaatahan commented Jan 31, 2025

ardaatahan commented Jan 31, 2025

pcuenca commented Jan 31, 2025

ardaatahan commented Feb 2, 2025

pcuenca commented Feb 2, 2025

ardaatahan commented Feb 3, 2025

ZachNagengast commented Feb 3, 2025

	let sha256Pattern = "^[0-9a-f]{64}$"
	let sha256Pattern = "^[0-9a-f]{40}$"

	/// - localDir: The local directory where files are downloaded.
	/// - localDir: The local directory where metadata files are downloaded.

Add metadata support with tests #155

Are you sure you want to change the base?

Add metadata support with tests #155

Conversation

ardaatahan commented Jan 15, 2025

pcuenca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardaatahan commented Jan 30, 2025

Choose a reason for hiding this comment

pcuenca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcuenca commented Jan 31, 2025

ardaatahan commented Jan 31, 2025

ardaatahan commented Jan 31, 2025

pcuenca commented Jan 31, 2025

ardaatahan commented Feb 2, 2025

pcuenca commented Feb 2, 2025

ardaatahan commented Feb 3, 2025

ZachNagengast commented Feb 3, 2025