-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fix bug in ONNX importer #3084
Fix bug in ONNX importer #3084
Conversation
cc @zhreshold |
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.
By looking at the failure tests, does requirement of shapes from input required when converting models? If not, we need to bypass these.
@jroesch @zhreshold please look into the CI case and decide an actionable item to move forward |
It looks like there was previously a bug in the old test case that was being silently ignored due the previous default. If there were no inputs in the constant fill case we still added an input to the graph even though we didn't pass one. The other fix would be we intended to pass an argument and to explicitly pass one. |
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.
lgtm
Thanks @jroesch @zhreshold @yzhliu , this is now merged |
Fixes small bug in the ONNX importer that is very confusing, fix for #3079.
The importer previously would default to scalars when input shapes were not provided. This instead requires the user to explicitly provide all input shapes.