-
Notifications
You must be signed in to change notification settings - Fork 64
Fix TensorType to return new TensorTypes for shapes instead of TensorType instances #228
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
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.
lintrunner found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov Report
@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 75.57% 75.66% +0.08%
==========================================
Files 89 90 +1
Lines 7248 7274 +26
==========================================
+ Hits 5478 5504 +26
Misses 1770 1770
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
87c8432
to
2fb12b6
Compare
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.
LGTM🚀
onnxscript/onnx_types.py
Outdated
from __future__ import annotations | ||
|
||
from typing import Optional, Tuple, Union | ||
from abc import ABC |
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 would import abc and use abc.ABC
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.
Any particular reason?
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 because of this https://github.com/microsoft/onnx-script/wiki/Coding-style#avoid-relative-imports-import-only-modules, as suggested by https://google.github.io/styleguide/pyguide.html#23-packages.
ABC
is usually unique enough, but I think maintaining a single rule of importing only modules can save us from deciding what to import as modules and what not to.
2fb12b6
to
8eb6213
Compare
4ec4b4c
to
b7a7c97
Compare
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.
LGTM, thanks. One thing I am not entirely clear is how well other tools, like mypy, will be when dealing with onnxscript code with type annotations, but that's a different question, may be worth talking about in our meeting. E.g., would using something like Annotated[TensorType, Dtype(dtype), Shape(shape)]
work better with other type-checkers and tools?
559f507
to
d4ef07a
Compare
As for tooling, I am very unimpressed with mypy in particular. There is an actual bug in mypy that is causing it to not see the Python 3.11 provides support for variadic generics and syntax sugar to promote Python literals to Today: def F(tensor: FLOAT[Literal[128], Literal["M"], Literal[1024]]): ...
def G(tensor: FLOAT[Literal[Ellipsis]]): ... With 3.11: def F(tensor: FLOAT[128, "M", 1024]): ...
def G(tensor: FLOAT[...]): ... |
b9df365
to
08dc10d
Compare
This ensures that TensorType is never actually instantiated and instead new TensorType subclasses are created (and cached) for each shape. This is a stepping stone to a fully generic TensorType where the dtype and shape are modeled into the type itself, but it appears that won't be easily realized until we can depend on Python 3.11. Importantly, this adds unit tests to ensure if we can move to a proper generic that behavior will remain the same. Fixes #219
08dc10d
to
fcd664e
Compare
This ensures that
TensorType
is never actually instantiated and instead newTensorType
subclasses are created (and cached) for each shape.This is a stepping stone to a fully generic TensorType where the dtype and shape are modeled into the type itself, but it appears that won't be easily realized until we can depend on Python 3.11.
Importantly, this adds unit tests to ensure if we can move to a proper generic that behavior will remain the same.
Fixes #219