-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Define a Generator framework in Python #6764
Conversation
This is intended to facilitate a few things: - Move all Generators used in tests, apps, etc to a single directory to simplify the build rules (this is especially useful for the work in #6764) - Put all the test and apps stuff under a single directory to facilitate adding some Python packaging that can make integration into Bazel/Blaze builds a bit less painful @alexreinking, does this look like the layout we discussed before?
This is intended to facilitate a few things: - Move all Generators used in tests, apps, etc to a single directory to simplify the build rules (this is especially useful for the work in #6764) - Put all the test and apps stuff under a single directory to facilitate adding some Python packaging that can make integration into Bazel/Blaze builds a bit less painful @alexreinking, does this look like the layout we discussed before?
2f595ab
to
300c1a5
Compare
Per suggestion on #6764, this allows these type aliases. How does it look / feel? (Adding similar aliasing for, at least, NumPy types is probably desirable as well)
Per suggestion on #6764, this allows these type aliases. How does it look / feel? (Adding similar aliasing for, at least, NumPy types is probably desirable as well)
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.
Still reviewing _generator_helpers.py
, just publishing a couple comments now.
get_property(py_src TARGET ${ARG_FROM} PROPERTY Halide_PYTHON_GENERATOR_SOURCE) | ||
if (py_src) | ||
set(PYTHONPATH "$<TARGET_FILE_DIR:Halide::Python>/..") | ||
set(GENERATOR_CMD ${CMAKE_COMMAND} -E env PYTHONPATH=${PYTHONPATH} ${Python3_EXECUTABLE} $<SHELL_PATH:${py_src}>) |
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.
(1) If you're cross-compiling a Python extension, you will need to find a cross-target Python's libraries
(2) I have some ideas for this... but we can defer to a future PR
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.
Mostly quibbly stuff. Ping when when resolved and I'll be quick about re-reviewing.
Opened #7014 |
PTAL |
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 LGTM now, though I understand there are some pytype
issues?
Yeah... the issue is that pytype is the preferred type checker at Google, and it's a static type checker. Currently we use the This could be fixed by requiring that user Generators explicitly descend from
For the sake of argument, we could have it both ways: have the decorator verify that it is a subclass, and inject the baseclass only if it isn't, but that has a subtle downside: people coding without pytype would likely omit the explicit base class, but then they'd produce code that would need to be modified when getting imported and used in a pytype environment. This would be a huge nuisance in the long run, with the (likely) result that people end up adding the explicit inheritance always just to dodge complaints. At this point I'm inclined to just require the explicit inheritance in order to get around this problem, but I'm not happy about it (I find it especially unsatisfying that apparently PyType doesn't have a way to annotate dynamically-generated classes, because that's A Thing That Happens in dynamic languages like Python). |
* Allow (but don't require) explicit inheritance for Python Generators
This is intended to facilitate a few things: - Move all Generators used in tests, apps, etc to a single directory to simplify the build rules (this is especially useful for the work in halide#6764) - Put all the test and apps stuff under a single directory to facilitate adding some Python packaging that can make integration into Bazel/Blaze builds a bit less painful @alexreinking, does this look like the layout we discussed before?
* Define a Generator framework in Python
Work in progress, opening as draft for testing + sharing. More details + documentation to come soon.