Skip to content
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

Enable empty list of values as attribute #5559

Merged
merged 13 commits into from Sep 26, 2023
Merged

Enable empty list of values as attribute #5559

merged 13 commits into from Sep 26, 2023

Conversation

gramalingam
Copy link
Contributor

This PR makes two fixes:

  • The existing checker does not allow an attribute to have an empty list (of ints, or floats, or strings, etc.) as a value. The PR removes this check. It is better for op-specific checks to check for this where required.
  • There are similar checks in the shape-inference method of the Constant op, which are also removed.

This allows a Constant op to construct an empty 1D tensor of ints or floats, for example, conveniently.

@gramalingam
Allow empty list of values in Constant op
49c3dd8 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Update older versions of Constant
6f2668e 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam gramalingam requested review from a team as code owners September 1, 2023 03:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 1, 2023
View reviewed changes
onnx/defs/generator/utils.cc Fixed Show fixed Hide fixed
onnx/defs/generator/utils.cc Fixed Show fixed Hide fixed
onnx/defs/generator/utils.h Fixed Show fixed Hide fixed
@justinchuby
Copy link
Contributor

Just wondering: Do we know definitively which field is set (to know the dtype) when they are empty?

@gramalingam
Copy link
Contributor Author

Just wondering: Do we know definitively which field is set (to know the dtype) when they are empty?

Yes ... AttributeProto has a type field. (Further, the operator spec also constrains the type of a named attribute: eg., for Constant, value_ints is required to be ints type.)

@titaiwangms titaiwangms mentioned this pull request Sep 1, 2023
Merged
@justinchuby
Copy link
Contributor

I see!

@gramalingam
Run lint
70f477a 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Add newlines
5715952 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Merge branch 'main' into empty-list-check
afd9652
@gramalingam
Add test for value_floats
520653f 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Fix type
0ae59bd 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Merge branch 'main' into empty-list-check
1f032d2
@gramalingam
Merge branch 'main' into empty-list-check
ea2ac02
@gramalingam
Copy link
Contributor Author

@xadupre : any further comments on this PR? Thanks!

@gramalingam gramalingam added this pull request to the merge queue Sep 26, 2023
Merged via the queue into onnx:main with commit d14f721 Sep 26, 2023
35 checks passed
@gramalingam gramalingam deleted the empty-list-check branch September 26, 2023 16:12
@gramalingam gramalingam added this to the 1.15 milestone Sep 26, 2023
@abock abock mentioned this pull request Oct 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinchuby justinchuby justinchuby approved these changes

@titaiwangms titaiwangms titaiwangms approved these changes

@xadupre xadupre xadupre approved these changes

Assignees

@xadupre xadupre

Labels
onnx checker
Projects
None yet
Milestone
1.15
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gramalingam @justinchuby @titaiwangms @xadupre