Skip to content

feat(atenlib): index_select; trace_only ops #274

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 9 commits into from
Jan 7, 2023

Conversation

justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Jan 5, 2023

Implement aten_index_select; enable tests for transpose.

This change also added the trace_only argument to mark functions as trace only to work around functions that cannot be compiled. The logic can then be tested, but the argument should be removed later when onnxscript extends support for the syntax.

@justinchuby justinchuby marked this pull request as draft January 5, 2023 00:47
@justinchuby justinchuby mentioned this pull request Jan 5, 2023
14 tasks
@justinchuby
Copy link
Collaborator Author

cc @BowenBao

@justinchuby justinchuby added the module: torchlib Related to the torch/aten function lib in development label Jan 5, 2023
@BowenBao
Copy link
Contributor

BowenBao commented Jan 5, 2023

We need to fine a way to call the function that have the attribute dim before the input index.

Sounds like a restriction posed from onnx script function. Is there something fundamentally blocking from mixing the order of inputs and attributes? cc @gramalingam

@gramalingam
Copy link
Collaborator

We need to fine a way to call the function that have the attribute dim before the input index.

Sounds like a restriction posed from onnx script function. Is there something fundamentally blocking from mixing the order of inputs and attributes? cc @gramalingam

No, I don't think there is anything fundamentally blocking it. But we need to add the logic to onnxscript translator to process the parameters accordingly, so it is a missing feature.

@justinchuby justinchuby added enhancement New feature or request and removed enhancement New feature or request labels Jan 5, 2023
# index_select(Tensor self, int dim, Tensor index) -> Tensor

raise NotImplementedError()
if op.Size(op.Shape(index)) == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise, is index expected to be 1-dimensional? Or, can it have any other rank?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it should be 1d I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should I do op.Reshape(index, [-1]) instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, if we expect it to be 0d or 1d, I think reshape, as you mention, is a better option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. thanks!

@justinchuby justinchuby marked this pull request as ready for review January 5, 2023 23:40
Copy link
Contributor

@fatcat-z fatcat-z left a comment

Choose a reason for hiding this comment

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

Approved.

@codecov
Copy link

codecov bot commented Jan 7, 2023

Codecov Report

Merging #274 (e6b8a9f) into main (47fe75f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #274      +/-   ##
==========================================
+ Coverage   72.97%   73.04%   +0.07%     
==========================================
  Files          95       95              
  Lines        9113     9131      +18     
==========================================
+ Hits         6650     6670      +20     
+ Misses       2463     2461       -2     
Impacted Files Coverage Δ
onnxscript/function_libs/torch_aten/ops/core.py 59.07% <100.00%> (+0.57%) ⬆️
...nnxscript/function_libs/torch_aten/registration.py 88.57% <100.00%> (+0.69%) ⬆️
...t/function_libs/torch_aten/ops_correctness_test.py 93.79% <100.00%> (+0.08%) ⬆️

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

@justinchuby justinchuby added the topic: syntax onnxscript syntax label Jan 7, 2023
@justinchuby justinchuby changed the title feat(atenlib): index_select feat(atenlib): index_select; trace_only ops Jan 7, 2023
@justinchuby justinchuby merged commit dd3d747 into main Jan 7, 2023
@justinchuby justinchuby deleted the justinchu/index-select branch January 7, 2023 02:33
@justinchuby justinchuby mentioned this pull request Jan 7, 2023
@justinchuby justinchuby mentioned this pull request Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: torchlib Related to the torch/aten function lib in development topic: syntax onnxscript syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants