-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Generate pyplot wrappers at runtime #25422
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
base: main
Are you sure you want to change the base?
Conversation
This removes the need for a boilerplate and various templates that need to be generated and pass flake8. As a downside, the wrappers no longer ave explicit parameters, so argument errors are raised from the original methods, which may be confusing.
When we talked about doing this before we rejected it because we wanted to be kind to static analysis so we would probably need to generate the pyi files still? |
I don't think the pyi files necessarily need to follow pep8 (at least they may not need to be linewrapped)? |
I thought we had an earlier discussion about this, but could not find it. I went through again and did a better search, and I believe the longterm goal was to get rid of
At this point, we are far and away from still supporting Python 2.2, and this implementation correctly creates the signature:
The only inconsistency is the error raised for incorrect arguments as noted above, but we could fix that with |
I also could've sworn that yesterday I could get ipython/python to display the type annotations in signature, and thus confirmed that the wrapped versions were correct, but today I can't seem to get it to display anything, even for the original functions. But yes, it looks like running |
This will not have type hints for the generated functions. Type hints from pyi stub files are not available at runtime, and thus the signatures for any dynamic generation will not have them until and unless we have inline type hints. This behavior may also explain the "ipython sometimes has annotations". It will have them, I think, for pyplot in #24976, but not any of the underlying functions. I believe Ipython uses the runtime annotations. We could make boilerplate generate the pyi file instead of the py file, which is arguably easier (though not by much, honestly) and could be less strict about flake8... |
I’m feeling very uneasy about this. Dynamically generated code can behave like static code, but it is different, which can show up in a number of ways:
Overall, I think this will work well in 99% of the cases, but I expect several edge cases where we’ll run into trouble. This is an essential part of the library. The static generator has proven to work well. Changing that is risky. On the other hand, I do not fully understand the motivation. What are the benefits of this? Is this only because of flake8? If so, there have to be better ways to cope with it. |
My understanding is that this is in response to my proposed change as part of the type hinting effort. See that discussion for my in depth reply. The short version is that type hints made the auto generated code hit several edge cases on formatting that the existing naive I am not totally unwilling to rethink that, but the bar is pretty high, as outlined in my reply to the linked comment above. |
I'm fine with running black on the autogenerated code. |
PR Summary
When looking at #24976, I was wondering if we could drop
black
for it, but that seemed to be fairly difficult. But then I wondered if we really needed to generate all this text that needed to passflake8
in the first place.So this removes the need for a boilerplate and various templates that need to be generated and pass flake8. Now that module-level
__getattr__
exists, we don't need to pay the full penalty of generating everything on import, and can amortize that over every function call.As a downside, the wrappers no longer have explicit parameters, so argument errors are raised from the original
methods, which may be confusing. There is currently one test that fails because of this, but it could be fixed if we are okay with this.
This also includes #12743, in order to more explicitly define what's in the file. I did not yet re-review everything in
__all__
, but I did need to add these symbols to__dir__
in order to get tests to pass.There's also a small oddity that I need to explicitly call
__getattr__
in the file itself. I'm not sure if there is a better way to fix that.PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst