[jit] Add support for class annotations #21379
Conversation
torch/jit/annotations.py
Outdated
| return_type = ann_to_type(as_ann(sig.return_annotation)) | ||
| return arg_types, return_type | ||
|
|
||
| def try_to_infer_type(value): |
There was a problem hiding this comment.
Could consider erroring here if we see a None value, and say we need an annotated type for Optionals
| # Copy annotations, pull types from `__annotations__` or try to infer | ||
| # the type if possible | ||
| class_annotations = getattr(original, '__annotations__', {}) | ||
| for name in dir(original): |
There was a problem hiding this comment.
doesn't dir(value) gets a ton of arbitrary python implementation details ? these are all nn.Modules, we could try register a hook in setattr or something else
There was a problem hiding this comment.
As far as I know we don't really have any other choice since things like __dict__ only have the current class' info (not anything from superclasses), plus not everything goes through __setattr__ (e.g. static members). This is mostly safe since the only things it takes are types it can infer, so it would pick up the __module__ and __doc__
There was a problem hiding this comment.
Looks like it would also pick up _version inplace dump_patches
There was a problem hiding this comment.
why doens't it work if you add it here ?
pytorch/torch/nn/modules/module.py
Line 584 in c25e337
There was a problem hiding this comment.
That'd be mostly fine but if something is defined at the class level it doesn't go through __setattr__, it's just on the class type itself, we need to copy those too (including any up the inheritance chain) so it doesn't work generally
|
you re-requested review but my existing comments are unaddressed |
pytorchbot
added
module: build
pytorchbot
added
the
module: pybind
|
is this ready for re-review? |
|
@suo Yeah it's ready |
Summary: This refactors pybind_utils so we can have all our type-inferring stuff in 1 place (e.g. for pytorch#21379) There is some follow up work to make the error messages better, but I think that's fine to save for another PR. ](https://our.intern.facebook.com/intern/diff/15727002/) Pull Request resolved: pytorch#21550 Pulled By: driazati Differential Revision: D15727002 fbshipit-source-id: a6974f2e1e5879f0503a18efc138da31cda7afa2
pytorchbot
added
the
module: tests
|
looks like there is a py2 error, please resolve that before landing |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This adds support for inferred attributes (everything except empty lists, dicts, and tuples) as well as using the PEP 526 style annotations on a class, so this eliminates the need for
torch.jit.AttributeDifferential Revision: D15718537