-
Notifications
You must be signed in to change notification settings - Fork 63
feat(atenlib): ops 2/n #252
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
Merged
Merged
Changes from all commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
55f8dd9
fix: annotate script()
justinchuby 8a3a587
feat(atenlib): clamp, lt, gt
justinchuby f821b6a
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby aecc148
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 6555a55
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby f8385b0
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 468f86f
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 47b8380
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 00f1760
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 060f9db
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 497cb16
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 9bb4038
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby d24110a
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby cbfb867
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 875f235
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 27008e1
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 3a8737d
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby c5871c8
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 012905c
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 49be5ec
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 3a9c5f6
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby d4f09e8
Update base for Update on "feat(atenlib): implement aten functions 1/n"
justinchuby ee3143e
Update on "feat(atenlib): implement aten functions 1/n"
justinchuby 691772b
feat(atenlib): ops 2/n
justinchuby f160dfa
Update on "feat(atenlib): ops 2/n"
justinchuby c8e4a54
Update base for Update on "feat(atenlib): ops 2/n"
justinchuby 7cd967d
Update on "feat(atenlib): ops 2/n"
justinchuby e8f07c9
Update base for Update on "feat(atenlib): ops 2/n"
justinchuby c4c80a1
Update on "feat(atenlib): ops 2/n"
justinchuby 7c0e305
Update base for Update on "feat(atenlib): ops 2/n"
justinchuby 2db3170
Update on "feat(atenlib): ops 2/n"
justinchuby b7b03ee
Update base for Update on "feat(atenlib): ops 2/n"
justinchuby 5216435
Update on "feat(atenlib): ops 2/n"
justinchuby 93ed77b
Update base for Update on "feat(atenlib): ops 2/n"
justinchuby a5c9629
Update on "feat(atenlib): ops 2/n"
justinchuby 57676c8
Update base for Update on "feat(atenlib): ops 2/n"
justinchuby 3967967
Update on "feat(atenlib): ops 2/n"
justinchuby File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In current PyTorch exporter, if the rank of input is 2 and the node of bias is not None, it will return a result of addmm(). Code is here.
Why do we remove that logic here?
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 think we can leave this to downstream optimization to keep the graph as simple and expressive as possible. What do you think?
Also addmm is an aten op. I think we should avoid calling other ATen ops and instead extract logic to common functions only when needed (which for now is not well supported by onnxscript, so we don’t call other functions ever yet)
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.
For the optimization purpose, then I think we should add some comments here to mark this point so that we won't forget it when we start the optimization work.
Even these functions were named with an aten_ prefix, they are still ONNXScript functions which are essentially same to those op.Functions. I think we don't need to avoid calling each other within this aten functions lib. Do you have any examples/ideas to describe the cons?
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.
(1) Since they have ATen signatures, we have seen many cases where additional params need to be provided to make the call correct. (2) Conceptually, the ATen functions interface with ATen ops (external facing). And so calling them internally overloads the responsibility, which I hope to maintain a clear conceptual model/boundary of.
This also helps with the representation of the exported graph. When users see an ATen function, they will know there is a corresponding op in their model.
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.
Added a comment