Skip to content

feat(atenlib): add ops (new_empty, new_empty_strided)#436

Merged
xiaowuhu merged 10 commits intomicrosoft:mainfrom
xiaowuhu:xiaowu/addOps(0214)
Feb 16, 2023
Merged

feat(atenlib): add ops (new_empty, new_empty_strided)#436
xiaowuhu merged 10 commits intomicrosoft:mainfrom
xiaowuhu:xiaowu/addOps(0214)

Conversation

@xiaowuhu
Copy link
Copy Markdown
Contributor

No description provided.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 14, 2023

Codecov Report

Merging #436 (0c98ee8) into main (a7dde7b) will increase coverage by 0.02%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
+ Coverage   71.27%   71.30%   +0.02%     
==========================================
  Files         108      108              
  Lines       10475    10489      +14     
  Branches     1085     1088       +3     
==========================================
+ Hits         7466     7479      +13     
  Misses       2699     2699              
- Partials      310      311       +1     
Impacted Files Coverage Δ
...s/function_libs/torch_aten/ops_correctness_test.py 90.33% <ø> (ø)
onnxscript/function_libs/torch_aten/ops/core.py 67.62% <84.21%> (+0.18%) ⬆️
onnxscript/evaluator.py 85.96% <100.00%> (+0.25%) ⬆️
...xscript/function_libs/torch_aten/graph_building.py 68.66% <100.00%> (-0.40%) ⬇️

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

@xiaowuhu xiaowuhu mentioned this pull request Feb 14, 2023
@xiaowuhu xiaowuhu changed the title (draft) (feat)atenlib: add ops (feat)atenlib: add ops (new_empty, new_empty_strided_ Feb 14, 2023
@xiaowuhu xiaowuhu requested a review from justinchuby February 15, 2023 00:11
@justinchuby justinchuby changed the title (feat)atenlib: add ops (new_empty, new_empty_strided_ feat(atenlib): add ops (new_empty, new_empty_strided) Feb 15, 2023
@xiaowuhu xiaowuhu requested a review from justinchuby February 15, 2023 04:20
Comment on lines +3940 to +3943
if dtype == -1:
result = op.CastLike(result, self)
else:
result = op.Cast(result, to=dtype)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dtype needs to be the same for both branches. this op may need to be trace only

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not got your point

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ONNX requires that both branches of an If node’s type to be the same. Since we are casting here the graph violates this constraint. I think Rama is looking at potential solutions, but for now we will need to mark the function traceonly, or else the graph would be invalid.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More info in onnx/onnx#4872

return result


@torch_op("aten::new_empty_strided")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially trace only

@xiaowuhu xiaowuhu merged commit 35fa2c9 into microsoft:main Feb 16, 2023
@xiaowuhu xiaowuhu deleted the xiaowu/addOps(0214) branch February 16, 2023 05:42
@justinchuby justinchuby mentioned this pull request Feb 16, 2023
skip("empty_like", reason="Using zeros_like to simulate empty_like"),
xfail("logcumsumexp", reason="naive implementation not numerically stable"),
xfail("logsumexp", reason="ONNX Runtime 1.13 does not support ReduceLogSumExp-18"),
xfail("new_empty", reason="Using zeros to simulate empty"),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaowuhu I also realized this can succeed unexpectedly. In that case a skip may be a better choice than xfail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants