-
Notifications
You must be signed in to change notification settings - Fork 816
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
Arg name correction: auth_token -> token #1621
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
LGTM, thanks!
cc @ArthurZucker for a quick review (this fixes huggingface/transformers#33183)
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.
Thanks, actually you would need to modify the rust code that generate this init file!
Ah ok, sorry! I think this is the Rust file that should be updated then tokenizers/bindings/python/src/tokenizer.rs Line 593 in 14a07b0
|
.rs updated. Re-iterating my my bug report: I'm new here, don't know Rust, don't have the doc build env set up. |
@ArthurZucker could you check whether the rust code has been updated? It would be nice to merge this 🤗 thanks |
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.
It was not!
pub auth_token: Option<String>, |
is were you need to update!
if let Some(auth_token) = auth_token { | ||
kwargs.set_item(intern!(py, "token"), auth_token)?; | ||
if let Some(token) = token { | ||
kwargs.set_item(intern!(py, "token"), token)?; | ||
} | ||
let path: String = hf_hub_download.call((), Some(&kwargs))?.extract()?; |
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.
we just pass the kwargs to the hf_hub_download api, so IMO this is fine!
@rravenel do you want me to do it? |
Sure - thanks.
…On Tue, Oct 1, 2024 at 5:09 AM Arthur ***@***.***> wrote:
@rravenel <https://github.com/rravenel> do you want me to do it?
—
Reply to this email directly, view it on GitHub
<#1621 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATIILR2ZVD5QZ7BQSNZEODZZKGJFAVCNFSM6AAAAABNK64IFGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBVGYYDQMJRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
b6e03d7
to
15a36c3
Compare
hi @ArthurZucker, is this ready to be merged so we can close huggingface/transformers#33183? |
Yep Merging, this will be in 0.21.0 |
Simple correction.