Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: utils
-
Labels:None
-
Story Points:4
-
Epic Link:
-
Sprint:DRP S17-3
-
Team:Data Release Production
Description
Our codebase is full of template classes that are wrapped into a set of Python classes with suffixes derived from their template type (e.g. ImageF, ImageD, ImageU).
In some places, we've defined a module scoped dictionary with the un-suffixed name to relate these, e.g. afw.table.Key["F"] is afw.table.KeyF.
To enhance both the usuability of our Python interfaces and the ease of writing wrappers, we should:
- Add an Abstract Base Class for each of these template classes (https://docs.python.org/2/library/abc.html), making it appear as though all template instantiations inherit from the same class (in e.g. isinstance checks).
- Make these ABCs inherit from a custom metaclass (which in turn inherits from abc.ABCMeta) that allows dict-like access to the registered instantiations from both the string suffixes and the corresponding numpy types (e.g. numpy.float32 for "F"), preserving the dict behavior mentioned above.
- Add a __new__ method to each ABC that accepts a dtype argument and forwards all other arguments to the constructor of the wrapped template the dtype key points to. In other words, make e.g. Image(bbox, dtype=numpy.float32) call ImageF(bbox). We could probably have the metaclass inject this automatically.
New users to the stack should be able to mostly get by without seeing the templated types at all - they'd only interact with the ABC. And devs writing the Python wrappers for templated classes could define pure-Python extensions directly in the ABC instead of monkeypatching them into each wrapped template.
This issue covers adding the metaclass to utils and demonstrating its use in at least one template class wrapper. Using it for other classes could be done on other pybind11 branches, as it strictly represents an addition to the API, and it could be useful to do it there as to get the monkeypatching-avoidance mentioned above.
In addition to the above, this ticket should include an implementation for the decorators described in RFC-270, which have significant code in common with the metaclass (as implemented currently) and a related purpose.
Attachments
Issue Links
- blocks
-
- Done
-
DM-8716 Use visitor pattern to clean up afw::table pybind11 wrappers
- Done
- is triggered by
-
- Implemented
- relates to
-
- Implemented
Activity
| Field | Original Value | New Value |
|---|---|---|
| Story Points | 1 | 2 |
Pim Schellart [X], I've put my WIP implementation of the metaclass on tickets/DM-8805, along with the decorators discussed in RFC-270, and I'm reassigning this issue to you since I won't be able to work on it again soon. They seem to do everything they need (unless my last round of changes broke things), but they need a few more tests and that may turn up some edge case issues. And as we discussed, we may want to consider a C++ interface for the metaclass.
I think the code for the decorators and the metaclass should stay together (in the same repo and probably file, but not necessarily the same ticket), but I'll let you lead the rest of the discussion on whether these should go in utils, pybind11, or a separate third-party repo.
| Assignee | Jim Bosch [ jbosch ] | Pim Schellart [ pschella ] |
Can you please not document the Python 2 approach to metaclasses?
| Description |
Our codebase is full of template classes that are wrapped into a set of Python classes with suffixes derived from their template type (e.g. {{ImageF}}, {{ImageD}}, {{ImageU}}).
In some places, we've defined a module scoped dictionary with the un-suffixed name to relate these, e.g. {{afw.table.Key\["F"\] is afw.table.KeyF}}. To enhance both the usuability of our Python interfaces and the ease of writing wrappers, we should: - Add an Abstract Base Class for each of these template classes (https://docs.python.org/2/library/abc.html), making it appear as though all template instantiations inherit from the same class (in e.g. {{isinstance}} checks). - Make these ABCs inherit from a custom metaclass (which in turn inherits from {{abc.ABCMeta}}) that allows dict-like access to the registered instantiations from both the string suffixes and the corresponding numpy types (e.g. {{numpy.float32}} for "F"), preserving the dict behavior mentioned above. - Add a {{\_\_new\_\_}} method to each ABC that accepts a {{dtype}} argument and forwards all other arguments to the constructor of the wrapped template the {{dtype}} key points to. In other words, make e.g. {{Image(bbox, dtype=numpy.float32)}} call {{ImageF(bbox)}}. We could probably have the metaclass inject this automatically. New users to the stack should be able to mostly get by without seeing the templated types at all - they'd only interact with the ABC. And devs writing the Python wrappers for templated classes could define pure-Python extensions directly in the ABC instead of monkeypatching them into each wrapped template. This issue covers adding the metaclass to utils and demonstrating its use in at least one template class wrapper. Using it for other classes could be done on other pybind11 branches, as it strictly represents an addition to the API, and it could be useful to do it there as to get the monkeypatching-avoidance mentioned above. |
Our codebase is full of template classes that are wrapped into a set of Python classes with suffixes derived from their template type (e.g. {{ImageF}}, {{ImageD}}, {{ImageU}}).
In some places, we've defined a module scoped dictionary with the un-suffixed name to relate these, e.g. {{afw.table.Key\["F"\] is afw.table.KeyF}}. To enhance both the usuability of our Python interfaces and the ease of writing wrappers, we should: - Add an Abstract Base Class for each of these template classes (https://docs.python.org/2/library/abc.html), making it appear as though all template instantiations inherit from the same class (in e.g. {{isinstance}} checks). - Make these ABCs inherit from a custom metaclass (which in turn inherits from {{abc.ABCMeta}}) that allows dict-like access to the registered instantiations from both the string suffixes and the corresponding numpy types (e.g. {{numpy.float32}} for "F"), preserving the dict behavior mentioned above. - Add a {{\_\_new\_\_}} method to each ABC that accepts a {{dtype}} argument and forwards all other arguments to the constructor of the wrapped template the {{dtype}} key points to. In other words, make e.g. {{Image(bbox, dtype=numpy.float32)}} call {{ImageF(bbox)}}. We could probably have the metaclass inject this automatically. New users to the stack should be able to mostly get by without seeing the templated types at all - they'd only interact with the ABC. And devs writing the Python wrappers for templated classes could define pure-Python extensions directly in the ABC instead of monkeypatching them into each wrapped template. This issue covers adding the metaclass to utils and demonstrating its use in at least one template class wrapper. Using it for other classes could be done on other pybind11 branches, as it strictly represents an addition to the API, and it could be useful to do it there as to get the monkeypatching-avoidance mentioned above. In addition to the above, this ticket should include an implementation for the decorators described in |
| Summary | Metaclass for wrapped-template ABCs | Metaclass for wrapped-template ABCs and class extension decorators |
| Assignee | Pim Schellart [ pschella ] | Jim Bosch [ jbosch ] |
| Story Points | 2 | 4 |
| Sprint | DRP S17-3 [ 360 ] |
Now that DM-9063 is complete, I'm restarting this work (I think it's at least halfway done already, and it's a big blocker for other pybind11 cleanup work).
| Status | To Do [ 10001 ] | In Progress [ 3 ] |
Krzysztof Findeisen, are you up for reviewing this?
I'm also quite open to suggestions from other reviewers, as I believe this will ultimately be some broadly-used code and the implementation has a few interesting wrinkles (which I hope I've commented adequately).
| Reviewers | Krzysztof Findeisen [ krzys ] | |
| Status | In Progress [ 3 ] | In Review [ 10004 ] |
Jim Bosch, I've taken a look at the PR, but I think you'd be better off asking someone more familiar with Python metaprogramming. Even with the comments, I'm not sure I can follow all the subtleties. Sorry!
Krzysztof Findeisen
added a comment - Jim Bosch , I've taken a look at the PR, but I think you'd be better off asking someone more familiar with Python metaprogramming. Even with the comments, I'm not sure I can follow all the subtleties. Sorry! Krzysztof Findeisen, thanks for the quick reply.
So, we've had a request for a Python metaprogramming expert. I wonder if Nate Lust is up for a review 
| Reviewers | Krzysztof Findeisen [ krzys ] | Nate Lust [ nlust ] |
I put up a bunch of review comments, if you have any questions please ask. I would like to see some of them addressed or at least discussed before I mark this as reviewed.
I believe I have now addressed all review comments.
As a side note, I am now seriously considering a __getattr__/__setattr__ approach to making TemplateMeta attributes appear in registered subclasses, but not on this ticket; that will require real thought and care to do properly, and probably isn't worth the effort right now. Aside from fixing the mro so that ABC methods no longer override subclass methods, the big motivation is making it possible to have the ABC inherit from another class. This would be useful in e.g. afw.image where many classes Exposure, MaskedImage, Mask, Image all share some pure-Python methods from slicing.py that they could be inheriting from a common base class that sits above their template ABCs. I can work around that just fine for now, though.
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
I'm all in favor of a dtype option (I pushed for such a thing in May 2015). Be careful about the differences between Python 2 and 3 when using metaclasses.