-
Notifications
You must be signed in to change notification settings - Fork 653
feat: Add python stub files #4692
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
feat: Add python stub files #4692
Conversation
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.
Here are my takeaways from this effort:
- While
std::optionalis generally a win in terms of reducing the need for maintaining manual fixes to the stubs, there are many other scenarios where we cannot pass enough information through to pybind11 to guide the stubgen process via C++, such as unbounded tuples (tuple[T, ...]vstuple[A, B, C]) or type unions (T | list[T] | tuple[T, ...]). If we want good stubs then we're signing up for permanantly maintaining these manual corrections external to our C++ code. - While I was hoping to publish a separate
types-OpenImageIOwith just the stubs, it will have a snowball effect on the CI build and release process, so for now I'd like to set a simpler goal of adding the__init__.pyifile to the existingOpenImageIOdistribution.
|
Idea: modifying |
|
Here's a progress report.
Stepping back a bit, one of our goals for adding stub generation to this repo is to create a tight feedback loop between changes to C++ and the resulting stubs. With this goal in mind, I set out to add the stub generation process to the cmake build script, in the hopes that, by making stubs every time that the python bindings were built, developers could easily discover the result of their changes. However, I'm realizing that this exposes us to a lot of complication in CI. It feels complex and brittle to have to build the stubs on every platform and python version (ensuring that Another problem is that there's nothing that forces a developer to review the effects of their C++ changes on the stubs, so while my current design succeeds at creating a tighter feedback loop between C++ and stubs, changes are likely to go unnoticed. Here's an idea for a solution to this problem:
|
|
@zachlewis and other heavy Python users, can you keep an eye on this PR and provide feedback? I've never generated (or even used) these stub files, so I don't feel qualified to pass any judgment on it. |
This may be the cleanest and simplest thing to do; and, like you mentioned earlier, it allows for type-checking in extremely light environments. Ostensibly, we could even run a building and publishing step for the types-OpenImageIO package immediately after publishing the OpenImageIO package, assuming we're not too quick for pypi. Alternatively, if we limit our scope to only generating stubs when building wheels with
I like this, too. There's something to be said for not attempting to be too clever about all this. We can simply say, "If you're updating the Python bindings, make sure to run the script that generates new stubs". If it makes things easier for folks, we could even expose a stub-generating script as part of the OpenImageIO package itself. |
|
I messed around with the cibuildwheel "repair-wheel" step hack as described above... I haven't tested on windows, but it seems like it's doing what it's supposed to be doing at first glance... If you're interested, I posted a proof of concept here: |
3e6e675 to
6258dd3
Compare
Thanks for the great suggestion @zachlewis. I had a look and it gave me some ideas to improve my approach. After some consideration, I think Have a look at the current state of things. I'm happy with where things landed. These are the requirements I was trying to hit when designing this workflow:
I left two goals for later PRs:
|
1dfa3f4 to
e5150f7
Compare
|
@zachlewis @lgritz This is ready for review. I've removed the C++ changes, so I think what I have is quite safe. It would be good to test out the local workflows on a platform other than MacOS, which is what I'm using. |
|
This is fantastic. Just a couple of questions inline, but I'm quite happy with everything you've put together. Thank you! This was quite an effort, and I appreciate the time you put into making stub generation as simple as possible to slot into OIIO's existing CI workflows. I'd definitely like to do something similar for OCIO! |
|
@zachlewis did you have some comments you were going to add? Maybe you forgot to complete the review? Now that we’ve got a workflow design for this that show work for C++ projects I doubt it would be much work to add this to OCIO. |
|
There seems to be a commit with missing DCO. Aside from that, where do we stand on this? I'm not enough of a Python power user or native speaker to really judge it. What say you, @zachlewis? Is this complete as far as you're concerned, @chadrik? |
| # SPDX-License-Identifier: Apache-2.0 | ||
| # https://github.com/AcademySoftwareFoundation/OpenImageIO | ||
|
|
||
| from __future__ import annotations |
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.
Just out of curiosity, at which Python version is the annotations stuff included and no longer needs to come from __future__?
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.
This can be removed once OCIO drops support for python 3.9, and only supports 3.10+
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.
OK, that will be a while. I think for 3.1 (~Sept release), we will probably raise our minimum from 3.7 to 3.9. But I think we can't go beyond that. Even at my company, I anticipate needing active 3.9 support through the end of the calendar year to be safe.
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.
The longer answer is that there are ways to avoid adding this import, but IMHO they are not worth the effort. For example, if you commit to using typing.List[X] instead of list[str], and using typing.Union[X, Y] instead of X | Y. It's (understandably) difficult for people to keep track of which version introduced which feature, so using from __future__ import annotations is a catchall that lets us not care so we can use these newer features even in python 3.7.
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.
No, I'm not worried about the import being there at all. It was mostly a curiosity of when in the future we need to remember that they are no longer needed.
|
|
||
|
|
||
| def print_deep_imagebuf (buf, prefix) : | ||
| def print_deep_imagebuf (buf: oiio.ImageBuf, prefix: str) : |
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.
Note to mostly-self: we should remember to take a pass over the docs and any python examples in the docs and change all the function declarations to use modern python3 syntax with type annotations.
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.
Just added this: #4752
| # | ||
| # This file is auto-generated. DO NOT MODIFY! Run `make pystubs` to regenerate | ||
| # | ||
|
|
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.
I'm ok with checking this into the repo, but do we have an automated way to detect that something has changed about the python bindings in a way that necessitates a re-generation? Should we add a note to the RELEASING.md guide reminding us to regenerate on every release as part of the release staging process?
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.
If a developer changes anything in the code that results in the type stubs changing, the cp311-manylinux_x86_64
and cp311-manylinux_aarch64 wheel build jobs in CI will fail, and it will print a diff and the instructions on how to update the stubs. I also updated the INSTALL.md with the instructions on how to update the stubs.
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.
ok, good enough
Signed-off-by: Chad Dombrova <chadrik@gmail.com>
Signed-off-by: Chad Dombrova <chadrik@gmail.com>
|
Yes, I think this is ready to go. I've also started porting this workflow over to OCIO. |
|
OK, I'd love to just get a very last ok from Zach, then I'm ready to merge. |
I think everything about this PR is fantastic. |
|
One last question: is it worth cherry-picking this onto other release branches, so that folks can benefit from this even if they're stuck on older versions? If yes, would I introduce an MR for each release branch? |
I was planning to backport to 3.0, the currently supported release family. You don't need to do it. All new code goes into main, and I cherry-pick the things that look like they can safely merge into the release branch and make sure CI verifies that they don't break anything. |
|
BTW, don't worry about the failing "bleeding edge" test. That is the test that builds against top-of-tree of several of our dependencies, and it frequently breaks when dependencies change or momentarily introduce bugs on their side. It looks like something changed in freetype today that is responsible. I'll let that keep failing for a few days to see if it's just a freetype regression that they fix on their own. |
e787dc6
into
AcademySoftwareFoundation:main
Add python stub files for code completion and static analysis. Fixes AcademySoftwareFoundation#4682 No new test cases, since there is not any change to the behavior. I do have an idea for a kind of test that might validate the stubs against the python unit tests, but I don't know if it's worth it. --------- Signed-off-by: Chad Dombrova <chadrik@gmail.com>
|
@chadrik I tried cherry-picking into dev-3.0 but it fails (you were right about it catching changes!) because the stubs aren't quite right and need to be built separately for that branch. I tried the So I'd like to request two things from you, if you don't mind:
|
|
@chadrik Not a huge rush on this, BTW. I think we are just aiming to be able to cherry-pick it into dev-3.0 successfully before the next scheduled release from that branch, on June 1. |
|
Yeah, that all seems reasonable. If you have a chance to post your error I’d love to check it out. I’d like to make it more bulletproof. |
|
Sure. Here's what happened for me. Keep in mind I have some unclean set of multiple pythons, whatever is on the system, whatever homebrew installed, etc. |
|
Oh yeah, this part sure isn't going to work on my mac: |
Add python stub files for code completion and static analysis. Fixes AcademySoftwareFoundation#4682 No new test cases, since there is not any change to the behavior. I do have an idea for a kind of test that might validate the stubs against the python unit tests, but I don't know if it's worth it. --------- Signed-off-by: Chad Dombrova <chadrik@gmail.com>
Add python stub files for code completion and static analysis. Fixes AcademySoftwareFoundation#4682 No new test cases, since there is not any change to the behavior. I do have an idea for a kind of test that might validate the stubs against the python unit tests, but I don't know if it's worth it. --------- Signed-off-by: Chad Dombrova <chadrik@gmail.com>
Add python stub files for code completion and static analysis. Fixes AcademySoftwareFoundation#4682 No new test cases, since there is not any change to the behavior. I do have an idea for a kind of test that might validate the stubs against the python unit tests, but I don't know if it's worth it. --------- Signed-off-by: Chad Dombrova <chadrik@gmail.com> Signed-off-by: Scott Wilson <scott@propersquid.com>
Add python stub files for code completion and static analysis. Fixes AcademySoftwareFoundation#4682 No new test cases, since there is not any change to the behavior. I do have an idea for a kind of test that might validate the stubs against the python unit tests, but I don't know if it's worth it. --------- Signed-off-by: Chad Dombrova <chadrik@gmail.com> Signed-off-by: Scott Wilson <scott@propersquid.com>
Add python stub files for code completion and static analysis. Fixes AcademySoftwareFoundation#4682 No new test cases, since there is not any change to the behavior. I do have an idea for a kind of test that might validate the stubs against the python unit tests, but I don't know if it's worth it. --------- Signed-off-by: Chad Dombrova <chadrik@gmail.com>
Description
Add python stub files for code completion and static analysis.
Fixes #4682
Tests
No new test cases, since there is not any change to the behavior. I do have an idea for a kind of test that might validate the stubs against the python unit tests, but I don't know if it's worth it.
Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.