Add permute and pow ops and fix attribute value issue.

[fatcat-z]

1. Add 2 new ops: permute and pow.
2. Fix (partially) Inputs and attributes could not be separated by _adapt_to_eager_mode method. #287: convert the values of attributes from tensor back to numpy value when we call Op.

[justinchuby]

lgtm. I would wait for @gramalingam

[codecov]

Codecov Report

Merging #293 (8a6f406) into main (dfaf174) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
+ Coverage   73.04%   73.13%   +0.08%     
==========================================
  Files          97       97              
  Lines        9461     9472      +11     
==========================================
+ Hits         6911     6927      +16     
+ Misses       2550     2545       -5     

Impacted Files	Coverage Δ
...t/function_libs/torch_aten/ops_correctness_test.py	95.60% <ø> (ø)
onnxscript/function_libs/torch_aten/ops/core.py	63.15% <100.00%> (+0.16%)	⬆️
onnxscript/values.py	87.91% <100.00%> (+0.48%)	⬆️
onnxscript/type_annotation.py	97.14% <0.00%> (+11.42%)	⬆️

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

[gramalingam]

Hi, a quick question: if I understand right, the code change converts attribute values that are tensors into numpy arrays.

First, I don't think that would fix issue 287 (completely). That's not a problem, just want to clarify that the PR description is a bit misleading. I believe we will still need to do something extra to resolve 287 fully. Or, did I misunderstand?

Maybe this change fixes the issue for a particular example? Can you point me to the example where this is used? I don't see it in this PR's changes. Thanks.

[fatcat-z]

Maybe this change fixes the issue for a particular example? Can you point me to the example where this is used? I don't

In aten_permute() method, the value of dims will be changed to tensors which are not expected by onnx.helper.make_attribute() function.

In eager mode, when we call an aten_xxx() method, all of positional inputs of it will be changed to tensors by _adapt_to_eager_mode() method. But inside the function, we still need to keep some of inputs to be a Numpy value (in aten_permute, it is 'dims') so that we can pass it into ONNX op as an attribute.

This will only happen in eager mode because _adapt_to_eager_mode will only be called in such mode. Probably I need to update the #287 for a clarification?

[gramalingam]

In eager mode, when we call an aten_xxx() method, all of positional inputs of it will be changed to tensors by _adapt_to_eager_mode() method. But inside the function, we still need to keep some of inputs to be a Numpy value (in aten_permute, it is 'dims') so that we can pass it into ONNX op as an attribute.

Ok. My main question is whether this positional input of the aten_xxx() method is supposed to be an attribute (of aten_xxx) or an input (of aten_xxx)?

I understand the first case: the problem happens if attributes are passed in positionally instead of as a keyword-argument. We can fix this.

The second case, however, cannot be supported in ONNX. Specifically, in ONNX, there is no way to convert an input (which is not statically known) into an attribute. I just want to clarify this ... we should NOT be writing such functions.

[gramalingam]

In the first case (where dims is an attribute of aten_permute), the problem is the existing assumption/restriction in onnxscript that inputs are passed in positionally, while attributes are passed in as keyword-arguments. In this case, I can see that the proposed change will work as a temporary fix, but the right long term solution will be to generalize the way onnxscript handles parameters to separate out inputs from attributes. I do plan to do this generalization, and the current proposed fix can be used as a temporary fix until this happens.

[justinchuby]

Could you help me understand why this is needed better?

[fatcat-z]

This was not introduced by this PR, it was submitted by another PR #304