Conversation
in case they will trigger warnigns in the `transformers` side. Even if the model is perfectly fine, core maintainers fear an influx of opened issues. This is perfectly legit. On the `transformers` side fixes are on the way: huggingface/transformers#20042 We can wait for this PR to hit `main` before communicating super widely. In the meantime this script of convertion will now prevent converting models that would trigger such warnings (so the output of the script **will** depend on the `transformers` freshness.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
bindings/python/convert.py
Outdated
| pt_model = pt_model | ||
| sf_model = sf_model | ||
| if pt_infos != sf_infos: | ||
| raise ValueError(f"different infos {model_id}") |
There was a problem hiding this comment.
Maybe add some info on the differences?
There was a problem hiding this comment.
It's quite noisy which is why I didn't include it (I mean infos can be quite numerous even on pure PT for transformers).
Doing a proper diff could help mitigate that but also relatively verbose code. I'll give it a stab.
|
|
||
|
|
||
| def convert(api: "HfApi", model_id: str, force: bool=False) -> Optional["CommitInfo"]: | ||
| def convert(api: "HfApi", model_id: str, force: bool = False) -> Optional["CommitInfo"]: |
There was a problem hiding this comment.
Didn't catch this before but we shouldn't use any api object and just the top level methods (create_commit is directly in the main init).
There was a problem hiding this comment.
Why ?
I figured it was actually better to pass around an HfApi codewise, so you can include a specific token (for instance to create conversions directly).
The original space convertion was passing around a Token, but I figure HfApi was even easier to transport around and there's less risk to forget to include token=token in whatever call.
There was a problem hiding this comment.
I don't think anyone who uses hugginface_hub uses the HfApi object.
There was a problem hiding this comment.
You should include a kwarg for token instead IMO
There was a problem hiding this comment.
I don't think anyone who uses hugginface_hub uses the HfApi object.
I do :)
There was a problem hiding this comment.
(this feature of setting a token on HfApi is quite recent though, but it's useful IMO)
There was a problem hiding this comment.
Let me rephrase, I don't think many people except Julien use HfApi :-p
There was a problem hiding this comment.
I did use it naturally actually an I think it's pretty cool to encapsulate common client options (most important of which the token).
I really think the "oh but you forgot to pass the token" is a real failure case, and it did happen when I used get_repo_discussions where I failed to pass the token and it would fail on private models on the space.
There was a problem hiding this comment.
I'll go ahead and merge this.
Please open an issue if you feel so strongly about the HfApi object :)
Narsil
deleted the
updating_convert_script_to_include_warnings_as_failures
branch
4 participants
in case they will trigger warnigns in the
transformersside.Even if the model is perfectly fine, core maintainers fear an influx
of opened issues.
This is perfectly legit.
On the
transformersside fixes are on the way: huggingface/transformers#20042We can wait for this PR to hit
mainbefore communicating super widely.In the meantime this script of convertion will now prevent converting
models that would trigger such warnings (so the output of the script
will depend on the
transformersfreshness.