Skip to content

Support bool as an attribute type #283

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.

Sign up for GitHub

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

Jump to bottom
Closed
justinchuby opened this issue Jan 5, 2023 · 4 comments · Fixed by #301
Closed

Support bool as an attribute type #283

justinchuby opened this issue Jan 5, 2023 · 4 comments · Fixed by #301
Labels
enhancement New feature or request module: torchlib Related to the torch/aten function lib in development

Comments

@justinchuby
Copy link
Collaborator

justinchuby commented Jan 5, 2023 •
edited
Loading

#281

@script()
def aten_logsumexp(self: TReal, dim: INT64["M"], keepdim: bool = False) -> TReal:
    # logsumexp(Tensor self, int[1] dim, bool keepdim=False) -> Tensor

    return op.ReduceLogSumExp(self, dim, keepdims=keepdim)

produces

ERROR onnxscript/test/function_libs/torch_aten/ops_correctness_test.py - TypeError: value "<onnxscript.values.Dynamic object at 0x7f2611f68640>" is not valid attribute data type.

Even though dim should be an input?

cc @gramalingam @fatcat-z @xiaowuhu
@justinchuby justinchuby added the module: torchlib Related to the torch/aten function lib in development label Jan 5, 2023
@justinchuby justinchuby moved this from Todo to Discussion in Torch Lib in onnxscript Jan 5, 2023
@gramalingam
Copy link
Collaborator

Quick question: is this with the latest opset of ONNX, which updates ReduceLogSumExp?

@justinchuby
Copy link
Collaborator Author

justinchuby commented Jan 6, 2023 •
edited
Loading

I was using opset18, onnx 1.13 and onnxruntime 1.13 I think

@gramalingam
Copy link
Collaborator

Ok, the problem is with keepdim. Currently, onnxscript doesn't recognize the "bool" type as a valid attribute type. (ONNX doesn't have bool attributes, it uses int attributes even for booleans.) So, if I use int it seems to work for now. We could extend the translator to support bool attributes and map them automatically to int attributes.

@justinchuby
Copy link
Collaborator Author

I see! That makes a lot of sense. I like the idea of supporting bool since it is a kind of int even in Python

@justinchuby justinchuby changed the title Error in ReduceLogSumExp Support bool as an attribute type Jan 7, 2023
@justinchuby justinchuby added the enhancement New feature or request label Jan 7, 2023
@BowenBao BowenBao mentioned this issue Jan 10, 2023
Merged
justinchuby pushed a commit that referenced this issue Jan 11, 2023
@BowenBao
feat(atenlib): topk; bool argument type annotation; node of multiple …
1b81421 
…outputs (#301)

~~Concern on the annotation of `k` as `int`. I assume this would imply
static value in onnx/onnxscript? Since scalars could be non-static in
torch, would we want a scalar type wrapper too?~~ Answer is use `INT64`.

* Add `aten_topk`.
* Extend onnx script type annotation for bool argument for attributes.
* Extend opinfo test for multiple outputs.

Fixes #283
@github-project-automation github-project-automation bot moved this from Discussion to Done in Torch Lib in onnxscript Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request module: torchlib Related to the torch/aten function lib in development
Projects
Torch Lib in onnxscript
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(atenlib): topk; bool argument type annotation; node of multiple outputs BowenBao/onnx-script
2 participants
@gramalingam @justinchuby