-
Notifications
You must be signed in to change notification settings - Fork 31.2k
Add overloads for PretrainedModel.from_pretrained #24035
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 overloads for PretrainedModel.from_pretrained #24035
Conversation
Fixes huggingface#23980; move output_loading_info from variadic kwargs to an explicit kwarg and create overloaded signatures based on its value; also add LoadingInfo TypedDict
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
sgugger
left a comment
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.
Again, any code using overload won't be accepted (you can search the source code, it's not present anywhere).
sgugger
left a comment
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.
Again, any code using overload won't be accepted (you can search the source code, it's not present anywhere).
Searching in the source code, you can see it here: https://github.com/search?q=repo%3Ahuggingface%2Ftransformers+%40overload&type=code Is there a specific part of this This overload makes it obvious to users that you only get the |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
@amyeroberts RE: #26125 (comment)
This PR was rejected with little explanation and without any discussion. I would be so grateful if you would be willing to hear me out.
The alternative would be to rewrite these functions as separate, distinct functions for each input/output type combination (not practical for existing functions). You will end up with the same number of signatures as if you just used Of course, I have to mention that the most important and ubiquitous function in all of import transformers
llama = transformers.LlamaForCausalLM.from_pretrained(...)
reveal_type(llama) # tuple[Unknown | LlamaForCausalLM, dict[str, Unbound | Unknown] | dict[str, Unknown | list[Unknown]] | Unknown] | Unknown | LlamaForCausalLMI'm really not certain on what basis the claim that PyTorch makes considerable use of Finally, and maybe most importantly, whether or not Hugging Face type checks its code, users are going to continue run type checkers on code that uses Hugging Face code. This is why the community keeps suggesting these changes. Refusal to adopt well-established Python typing features like In light all of this, how do you feel about revisiting this PR? I am sure so many users would be delighted to make use of such type hinting at such little cost to Hugging Face. Thank you for your consideration. For what it's worth, I think your choice of type checker makes a huge difference. I don't use or recommend mypy at all. I use the type checker built into VS Code, |
As commented here - #26125 (comment) - please feel free to open an issue where a solution (not necessarily relating to overloads) can be discussed.
Since #26125 around a year ago, I'm not aware of any issues or PRs opened addressing this. Given the number of issues we have opened daily, it doesn't appear to be something commonly raised or requested. Most of what I see is through github, so it's possible there are active discussions elsewhere e.g. on the forum. If so, please share links as this will help guide the discussion and decisions. To be clear, we're not saying that this doesn't provide any utility - it definitely does - the question is whether the additional code and its maintenance is worth it.
This is assumping we want to enable this feature! To provide some context, we previously had a push to increase complicance with type checkers about 2 years ago. This resulted in many issues and users complaining, so we eventually reverted this support #18485.
As you point out - the modeling file is already huge (something we want to address!). Doing this adds even more code. And, as @Narsil mentions, overloads can introduce bugs and make it harder to know what code is being called.
PyTorch is an amazing, but different library and which has different considerations and engineering choices. For one thing, the transformers Hugging Face team is small! For core logic like this there's really only 2 - 3 of us responsible for maintenance, and that's alongside our other commitments.
transformers is a very dynamic library - we don't plan these things out in advance!
You can't build software based on what people are doing with it downstream. People are of course welcome to run type checkers, but it's something we've decided not to support and so make no guarantees about how well it works. Python is ultimately not a great language for making typing assumptions. If the library was created and written now, fresh, then having more explicit and full coverage would be ideal. Sadly, that's not the case.
This is a strong assumption. |
What does this PR do?
Fixes #23980
This PR fixes the type hints that users see when calling
PretrainedModel.from_pretrainedbefore:
after:
output_loading_infofrom variadic kwargs to be an explicit kwargfrom_pretrainedbased on its valueLoadingInfoTypedDictBefore submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Documentation: @sgugger, @stevhliu and @MKhalusova