feat(atenlib): separate inputs from attributes #368

justinchuby · 2023-01-25T05:13:59Z

Create param_manipulation for separate inputs and attributes from python args and kwargs.
Raname typing.py to onnxscript/function_libs/torch_aten/tensor_typing.py

onnxscript/function_libs/torch_aten/param_manipulation.py

codecov · 2023-01-26T00:34:11Z

Codecov Report

Merging #368 (22e9b84) into main (2b84597) will decrease coverage by 0.17%.
The diff coverage is 61.86%.

@@            Coverage Diff             @@
##             main     #368      +/-   ##
==========================================
- Coverage   73.66%   73.50%   -0.17%     
==========================================
  Files          97       99       +2     
  Lines        9710     9821     +111     
==========================================
+ Hits         7153     7219      +66     
- Misses       2557     2602      +45

Impacted Files	Coverage Δ
...nxscript/function_libs/torch_aten/tensor_typing.py	`100.00% <ø> (ø)`
...ipt/function_libs/torch_aten/param_manipulation.py	`46.83% <46.83%> (ø)`
onnxscript/irbuilder.py	`80.00% <66.66%> (-0.31%)`	⬇️
...unction_libs/torch_aten/param_manipulation_test.py	`96.15% <96.15%> (ø)`
onnxscript/function_libs/torch_aten/ops/core.py	`64.98% <100.00%> (ø)`
onnxscript/function_libs/torch_aten/ops/nn.py	`59.93% <100.00%> (ø)`
onnxscript/function_libs/torch_aten/ops/special.py	`62.41% <100.00%> (ø)`
onnxscript/values.py	`88.35% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gramalingam · 2023-01-27T19:09:23Z

onnxscript/function_libs/torch_aten/param_manipulation.py

+    default: Any = _EmptyDefault
+    is_input: bool = True
+
+    def __repr__(self) -> str:


Is the return value supposed to be valid python or just a description?

Just wondering whether str is more appropriate or repr ?

Good catch, thanks!

gramalingam · 2023-01-27T19:13:03Z

onnxscript/function_libs/torch_aten/param_manipulation.py

+
+    def __repr__(self) -> str:
+        param_kind = "INPUT" if self.is_input else "ATTRIBUTE"
+        text = f"{self.name}<{param_kind}>: {self.type}"


nit (optional): f"{self.name} : {param_kind}({self.type})" or f"{param_kind} {self.name} : {self.type}" would be more readable?

a<INTPUT>: INT64 vs INPUT a : INT64 a : INTPUT(INT64)

I will go with a: Input[INT64]

gramalingam · 2023-01-27T19:16:44Z

onnxscript/function_libs/torch_aten/param_manipulation.py

+
+    Attributes:
+        name: The name of the parameter.
+        type: The type of the parameter.


Since we have multiple type representations, which one is this supposed to be? Eg., is this supposed to be python-types (like "int" and "List[FLOAT]") ?

I think Python types sg. For now not really enforced and it could be anything

gramalingam · 2023-01-27T19:21:29Z

onnxscript/function_libs/torch_aten/param_manipulation.py

+        return not self.is_input
+
+
+def extract_param_schema_from_function(onnx_func: values.OnnxFunction) -> List[ParamSchema]:


The current OnnxFunction IR loses the positional-order information between inputs and attributes (since we didn't care about it then given our assumptions that all inputs come first). We can't reconstruct it subsequently.

Perhaps we should consider updating OnnxFunction IR to store a List[ParamSchema] in the first place ... just thinking out aloud, haven't read the rest of the PR yet.

I like this idea! I was going to propose exposing ParamSchema somewhere in the function and op as the next thing to do so we have a common interface as #320

gramalingam · 2023-01-27T19:36:26Z

onnxscript/function_libs/torch_aten/param_manipulation.py

+                onnx_attributes[param.name] = kwargs[param.name]
+        else:
+            # input doesn't have default
+            onnx_attributes[param.name] = param.default


I haven't seen the usage of this function. Perhaps some usage contexts would not want default-values to be filled in. Anyway, I guess we can cross that bridge when we come to it.

I added a fill_defaults option to the function

) Generalize #368 to all eager backends split happens in the evaluator instead of the Op or OnnxFunction call methods so that evaluators can control behaviors like whether to fill in default values for attributes. Fixes #287, partially #305 (still need the scripted version)

justinchuby added 2 commits January 25, 2023 00:27

snapshot

b3df419

feat(eager): separate inputs from attributes

c255877

justinchuby requested review from gramalingam and titaiwangms January 25, 2023 05:13

Complete tests

2df9e09

justinchuby commented Jan 25, 2023

View reviewed changes

onnxscript/function_libs/torch_aten/param_manipulation.py Outdated Show resolved Hide resolved

justinchuby commented Jan 25, 2023

View reviewed changes

onnxscript/function_libs/torch_aten/param_manipulation.py Show resolved Hide resolved

justinchuby added 3 commits January 25, 2023 16:45

signature

6fd5c63

type

071dd6f

naming

4f2eb8d

justinchuby commented Jan 25, 2023

View reviewed changes

onnxscript/function_libs/torch_aten/param_manipulation.py Outdated Show resolved Hide resolved

justinchuby assigned titaiwangms Jan 25, 2023

justinchuby and others added 4 commits January 25, 2023 17:10

More tests

9507b8d

add param_manipulation-1 (#369)

7bfbfd7

Update irbuilder.py

69cac74

Merge branch 'main' into justinchu/param-manipulation

aedb99f

justinchuby marked this pull request as ready for review January 26, 2023 00:22

justinchuby changed the title ~~feat(eager): separate inputs from attributes~~ feat(atenlib): separate inputs from attributes Jan 26, 2023

titaiwangms approved these changes Jan 26, 2023

View reviewed changes

justinchuby added 3 commits January 25, 2023 16:46

Update param_manipulation.py

a3175e3

Update nn.py

0764da8

Update nn.py

22e9b84

justinchuby merged commit 3f433e7 into main Jan 26, 2023

justinchuby deleted the justinchu/param-manipulation branch January 26, 2023 01:56

justinchuby added module: torchlib Related to the torch/aten function lib in development topic: api labels Jan 26, 2023

gramalingam reviewed Jan 27, 2023

View reviewed changes

justinchuby mentioned this pull request Feb 3, 2023

feat(eager): generalized logic for splitting inputs and attributes #404

Merged

		return not self.is_input


		def extract_param_schema_from_function(onnx_func: values.OnnxFunction) -> List[ParamSchema]:

feat(atenlib): separate inputs from attributes #368

feat(atenlib): separate inputs from attributes #368

Uh oh!

Conversation

justinchuby commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinchuby Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinchuby Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinchuby commented Jan 25, 2023 •

edited

Loading

codecov bot commented Jan 26, 2023 •

edited

Loading

justinchuby Feb 3, 2023 •

edited

Loading

justinchuby Feb 3, 2023 •

edited

Loading