Migrate ir_builder to use onnx_ir (take 2) #2665
Conversation
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| result = self._converter.op( | ||
| "Constant", [], attrs=[attr] | ||
| ) | ||
| if is_base_type_bool(attr): |
| # for pyvar, previous in val.outer_scope_variables: | ||
| # current = self._lookup(pyvar, self._source_of(expr)) | ||
| # if current.value != previous.value: | ||
| # self.fail( | ||
| # expr, | ||
| # f"Outer scope variable '{pyvar}' referenced by function " | ||
| # f"'{expr.id!r}' modified.", |
| # First separate inputs from attributes. This is needed because in Python | ||
| # it is possible to pass onnx inputs as kwargs | ||
| inputs, attrs = _separate_inputs_and_attrs(op_signature, args, kwargs) | ||
| onnx_inputs = [self._translate_opt_expr(x) for x in inputs] |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2665 +/- ##
===========================================
- Coverage 70.42% 10.79% -59.64%
===========================================
Files 224 214 -10
Lines 26729 25160 -1569
Branches 2673 2564 -109
===========================================
- Hits 18825 2715 -16110
- Misses 6973 22434 +15461
+ Partials 931 11 -920 ☔ View full report in Codecov by Sentry. |
| """ | ||
| self.name = name | ||
| self.is_castable = castable | ||
| class DynamicKind(IntFlag): |
Check failure
Code scanning / lintrunner
PYLINT/E0602 Error
| """ | ||
| self.name = name | ||
| self.is_castable = castable | ||
| class DynamicKind(IntFlag): |
Check failure
Code scanning / lintrunner
RUFF/F821 Error
| """ | ||
| self.name = name | ||
| self.is_castable = castable | ||
| class DynamicKind(IntFlag): |
Check failure
Code scanning / lintrunner
MYPY/name-defined Error
| def is_base_type_bool(attr: ir.Attr) -> bool: | ||
| """Check if the attribute is a boolean type.""" | ||
| # FIXME: Add meta to attributes | ||
| attr.meta[_SOURCEINFO_FIELD] |
Check warning
Code scanning / lintrunner
PYLINT/W0104 Warning
| self._converter = converter | ||
|
|
||
| def get_or_create_value( | ||
| self, var: str, info: sourceinfo.SourceInfo |
Check warning
Code scanning / lintrunner
PYLINT/W0613 Warning
| converter_: _converter.Converter, | ||
| op_schema: Optional[OpSchema], | ||
| args: Sequence[Optional[converter.Variable]], | ||
| args: Sequence[Optional[_converter.Variable]], |
Check failure
Code scanning / lintrunner
MYPY/name-defined Error
| """ | ||
|
|
||
| def get_type_info(x: Optional[converter.Variable]) -> Optional[converter.Variable]: | ||
| def get_type_info(x: Optional[_converter.Variable]) -> Optional[_converter.Variable]: |
Check failure
Code scanning / lintrunner
MYPY/name-defined Error
|
|
||
| def cast_like( | ||
| x: Optional[converter.Variable], y: Optional[converter.Variable] | ||
| x: Optional[_converter.Variable], y: Optional[_converter.Variable] |
Check failure
Code scanning / lintrunner
MYPY/name-defined Error
| # TODO: cleanup Converter interface/API, separating checker from | ||
| # converter | ||
| convert = converter.Converter( | ||
| convert = _converter.Converter( |
Check failure
Code scanning / lintrunner
PYLINT/E1120 Error
| global_names = module.__dict__.copy() | ||
| global_names.update(closure.nonlocals) | ||
| converter = converter_module.Converter( | ||
| converter = _converter.Converter( |
Check failure
Code scanning / lintrunner
PYLINT/E1120 Error
| def is_base_type_bool(attr: ir.Attr) -> bool: | ||
| """Check if the attribute is a boolean type.""" | ||
| # FIXME: Add meta to attributes | ||
| attr.meta[_SOURCEINFO_FIELD] |
There was a problem hiding this comment.
? Why is this related to SOURCE_INFO?
|
|
||
| class Variable: | ||
| """Represents an ONNX variable. | ||
| _CASTABLE_FIELD = "pkg.onnxscript.converter.castable" |
There was a problem hiding this comment.
nit: why not just call these _CASTABLE and _SOURCEINFO ?
There was a problem hiding this comment.
Also: I wonder about the use "pkg" ... while a conflict seems unlikely, it would be good to set and follow some conventions for the metadata key strings to avoid conflicts between different tools metadata. The ONNX spec says use reverse domain name, just like for custom domain names for opsets: https://github.com/onnx/onnx/blob/main/docs/IR.md#other-metadata
Of course, too bad we dont have onnxscript.ai domain, but we could in ONNX standard reserve names like that.
There was a problem hiding this comment.
I think pkg. is a legitimate reverse domain (style) name?
| attr.meta[_SOURCEINFO_FIELD] | ||
|
|
||
| @dataclasses.dataclass | ||
| class ASTMeta: |
There was a problem hiding this comment.
This is no longer needed
|
|
||
| class _ValueEnvironment: | ||
| def __init__(self, converter: Converter): | ||
| self._py_var_name_to_ir_values: dict[str, ir.Value] = {} |
There was a problem hiding this comment.
Not sure I see the value in splitting this map into three maps?
| self._current_fn: irbuilder.IRFunction = None | ||
| # TODO(justinchuby): Update ir version to be user defined | ||
| # TODO(justinchuby): Maybe just store a list of functions | ||
| self._model = ir.Model(ir.Graph((), (), nodes=()), ir_version=10) |
There was a problem hiding this comment.
nit (can be fixed later too): not sure we need to create an ir.Model ahead of time ... eg., if this is just intended to be used as a function/FunctionProto
Continuation of #2457