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

Cleanup shape inference implementation #5596

Merged
merged 16 commits into from Oct 5, 2023
Merged

Cleanup shape inference implementation #5596

merged 16 commits into from Oct 5, 2023

Conversation

gramalingam
Copy link
Contributor

@gramalingam gramalingam commented Sep 15, 2023 •
edited

Description

  • Change the handling of undefined operators (to produce an error message).
  • Stop special treatment of experimental ops.
  • [Both of above are very old special handling. Recommend removing this, unless we find a compelling reason to maintain compatibility.]
  • Cleanup the use of an unnecessary GraphProto object to act as container for inferred types.
  • Move methods (BindValuesOnCall/Return) that do not need to be class methods into functions
  • Change method names to comply with naming convention.

Motivation and Context

  • General cleanup
  • Also prepare this for feature extensions, like incremental inference for use by inliner.

@gramalingam
Shape inference implementation cleanup
c1b91c9 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Remove bind methods out of class
84738d3 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Use pascal-case identifiers
febc3a3 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Handle undefined ops
2bbd663 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam gramalingam requested a review from a team as a code owner September 15, 2023 23:29
@gramalingam
Handle experimental ops
2f87307 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Cleanup handling of experimental op
3b19da2 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Cleanup exception handling in node-processing
3243529 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Undo unknown op error
ed36aa9 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Merge branch 'main' into si-cleanup
caddcb0
@gramalingam gramalingam changed the title [DRAFT: WIP] Cleanup shape inference implementation Cleanup shape inference implementation Sep 27, 2023
justinchuby
justinchuby approved these changes Oct 2, 2023
View reviewed changes
onnx/shape_inference/implementation.cc Show resolved Hide resolved
onnx/shape_inference/implementation.cc Show resolved Hide resolved
onnx/shape_inference/implementation.cc Show resolved Hide resolved
onnx/shape_inference/implementation.cc Show resolved Hide resolved
onnx/shape_inference/implementation.cc Show resolved Hide resolved
onnx/test/model_inference_test.py Show resolved Hide resolved
justinchuby and others added 4 commits October 2, 2023 14:11
@justinchuby
Merge branch 'main' into si-cleanup
e60ce4a
@gramalingam
Merge branch 'main' into si-cleanup
ef05b19
@gramalingam
delete constructors
3bb2a02 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Address PR feedback
cdb29e9 
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam gramalingam added this pull request to the merge queue Oct 5, 2023
Merged via the queue into onnx:main with commit 2caeb6a Oct 5, 2023
35 checks passed
@gramalingam gramalingam deleted the si-cleanup branch October 5, 2023 05:00
@justinchuby
Copy link
Contributor

Looks like the Windows release build starts failing from this PR: https://github.com/onnx/onnx/actions/runs/6414801812/job/17415759304#step:7:12115

@justinchuby justinchuby mentioned this pull request Oct 11, 2023
Closed
@jcwchen jcwchen mentioned this pull request Oct 16, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 20, 2023
@jcwchen
Fix Windows Release CI failure due to unresolved external symbol (#5678)
9bf7833 
### Description
<!-- - Describe your changes. -->
Fix Windows Release CI failure : `onnx.lib(checker.obj) : error LNK2001:
unresolved external symbol "public: int __thiscall
google::protobuf::RepeatedField<int>::size(void)const "
(?size@?$RepeatedField@H@protobuf@google@@QBEHXZ)
[D:\a\onnx\onnx\onnx\.setuptools-cmake-build\onnx_cpp2py_export.vcxproj]`
by setting PATH properly. We should use prepend instead of append.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->
Try to fix #5659. Currently Windows
Release CI starts to fail after #5596.

---------

Signed-off-by: jcwchen <jacky82226@gmail.com>
jcwchen added a commit to jcwchen/onnx that referenced this pull request Oct 20, 2023
@jcwchen
Fix Windows Release CI failure due to unresolved external symbol (onn…
21931d2 
…x#5678)

### Description
<!-- - Describe your changes. -->
Fix Windows Release CI failure : `onnx.lib(checker.obj) : error LNK2001:
unresolved external symbol "public: int __thiscall
google::protobuf::RepeatedField<int>::size(void)const "
(?size@?$RepeatedField@H@protobuf@google@@QBEHXZ)
[D:\a\onnx\onnx\onnx\.setuptools-cmake-build\onnx_cpp2py_export.vcxproj]`
by setting PATH properly. We should use prepend instead of append.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->
Try to fix onnx#5659. Currently Windows
Release CI starts to fail after onnx#5596.

---------

Signed-off-by: jcwchen <jacky82226@gmail.com>
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 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gramalingam @justinchuby @titaiwangms