Skip to content

Conversation

adrgrondin
Copy link
Contributor

Summary

This PR introduces new overloads for the snapshot functions’ progress handlers that expose download speed (in bytes/s).

The new overloads are fully backward compatible, existing APIs remain unchanged.

Changes

  • Example overload:
    @discardableResult 
    func snapshot(
        from repo: Repo, 
        revision: String = "main", 
        matching globs: [String] = [], 
        progressHandler: @escaping (Progress, Double?) -> Void
    ) async throws -> URL {
        try await snapshot(from: repo, revision: revision, matching: globs) { progress in
            let speed = progress.userInfo[.throughputKey] as? Double
            progressHandler(progress, speed)
        }
    }

Download speed printed in code compared against Xcode’s Network Report:

swift-transformers-download-speed Medium

@FL33TW00D
Copy link
Collaborator

@adrgrondin Great contribution thank you!

Could we add a test that accumulates some recorded speed in an array or something for a given download? Just to be sure it doesn't get broken in the future :)

Also swiftformat'ing

@adrgrondin
Copy link
Contributor Author

Thanks @FL33TW00D

I added a .snapshot() test case with speed, similar to other tests in the repo (real download).

I can run swift-format but this led to 33 files changed, so not sure if I'm doing something wrong with the command or if the project has not been formatted in a while. If it is the latter I would suggest to separate the formatting in a PR and I will rebase.

@FL33TW00D
Copy link
Collaborator

LGTM cc: @pcuenca

@Vaibhavs10 Vaibhavs10 requested a review from pcuenca September 9, 2025 12:26
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

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

Looks great! Appreciate the carefulness in keeping it non-breaking 🙌

We use a pinned version of the formatter because we've seen great rule divergence (as you experienced too). I took the liberty to run it locally and commit to your PR.

Comment on lines +583 to +586
if let speed {
fileProgress.setUserInfoObject(speed, forKey: .throughputKey)
progress.setUserInfoObject(speed, forKey: .throughputKey)
}
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@discardableResult func snapshot(from repo: Repo, revision: String = "main", matching globs: [String] = [], progressHandler: @escaping (Progress, Double?) -> Void) async throws -> URL {
try await snapshot(from: repo, revision: revision, matching: globs) { progress in
let speed = progress.userInfo[.throughputKey] as? Double
progressHandler(progress, speed)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is just a shortcut for the value in .throughputKey 👍

@@ -64,18 +64,17 @@ struct Download: AsyncParsableCommand, SubcommandWithToken {
DispatchQueue.main.async {
let totalPercent = 100 * progress.fractionCompleted
let speedBps = progress.userInfo[.throughputKey] as? Double
let speedString: String
if let s = speedBps {
let speedString = if let s = speedBps {
Copy link
Member

Choose a reason for hiding this comment

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

honestly, I hate that the formatter gets involved with this at all 😭 The previous version was clearer in my opinion cc @FL33TW00D @adrgrondin

@pcuenca
Copy link
Member

pcuenca commented Sep 9, 2025

Apologies for the failed CI run, we are in the process of fixing it. Tests pass locally, so merging!

@pcuenca pcuenca merged commit 0e67916 into huggingface:main Sep 9, 2025
1 of 2 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