-
Notifications
You must be signed in to change notification settings - Fork 131
Add repo revision option #202
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
Add repo revision option #202
Conversation
@@ -349,7 +350,7 @@ public extension HubApi { | |||
url = url.appending(component: repo.type.rawValue) | |||
} | |||
url = url.appending(path: repo.id) | |||
url = url.appending(path: "resolve/main") // TODO: revisions | |||
url = url.appending(path: "resolve/\(revision)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe best not to do direct string interpolation?
Also needs a null check I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String interpolation is the preferred method for this in Swift because it's faster and benefits from compile-time optimizations. We also don't need a null check here since revision is not marked as Optional
and always has a value. However, I can add something like assert(revision.isEmpty == false, "Revision cannot be empty")
if you'd prefer 🙂
Just tested:
Works fine with valid revision, incorrect error message if not. Would add a test with janky characters too, can never be too sure! |
…on-option # Conflicts: # Tests/HubTests/HubApiTests.swift
@FL33TW00D Hi! You're right 👍 I've fixed the error type to throw the correct error in case of invalid revision, and also added a few unit tests to test both positive and negative cases. |
Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for the great contribution and for iterating 🙌 !
I think we should maybe merge #200 first, just in case. Would you agree, @FL33TW00D?
Still seeing an issue here @petrukha-ivan, try the following: |
@FL33TW00D Ah, good catch — it's fixed now 👍 |
@petrukha-ivan Could you please rebase? We just merged another branch to fix an issue :) |
One last thing: |
…on-option # Conflicts: # Sources/Hub/HubApi.swift
@pcuenca Sure! I've just added a revision argument to those methods 👍 |
@FL33TW00D I'm not sure why the tests failed 🤔 I just ran it locally and everything is green. Could it be related to the runner somehow? |
@petrukha-ivan Nothing you've done, we are getting 429'd by the Hub |
@petrukha-ivan Appreciate the back and forth here! Thanks for the contribution 🤗 |
What
Added
revision
parameter to specify a branch, tag, or commit when downloading a model.