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.

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

Does inline_local_functions preserve ir_version? #6040

Open
xadupre opened this issue Mar 26, 2024 · 3 comments
Open

Does inline_local_functions preserve ir_version? #6040

xadupre opened this issue Mar 26, 2024 · 3 comments

Comments

@xadupre
Copy link
Contributor

xadupre commented Mar 26, 2024

Bug Report

onnxruntime complains about a wrong ir_version. Maybe the one from the orginal model is not preserved?

System information

  • OS Platform and Distribution (e.g. Linux Ubuntu 20.04): linux
  • ONNX version: 1.16
  • Python version: 3.10
@xadupre xadupre added the bug label Mar 26, 2024
@gramalingam
Copy link
Contributor

Based on a quick look, the code does not touch fields like ir_version when inlining. It seems unlikely to be happening during inlining. Is there an onnx model that causes a problem for reproduction?

@xadupre
Copy link
Contributor Author

xadupre commented Mar 27, 2024

Every model used in unit tests usuaaly specifies the ir_version to avoid onnxruntime to fail because it does not support the new one yet. Any model already used in the existing unit tests is fine. This is how I found it: https://github.com/sdpython/onnx-extended/blob/main/_unittests/ut_tools/test_onnx_inline.py#L58.

@gramalingam
Copy link
Contributor

@xadupre : I looked at the example in the above link. It is an invalid ONNX model: subgraphs cannot use outer-scope names as the output-names (and expect those values to be returned). Instead, it is expected that the subgraph will have at least one node such as subgraph_output = Identity(outer_scope_name).

I realize you do run the checker. I am guessing it doesn't catch it. If so, we should fix it in the checker and produce an error message.

I don't think this is connected to IR version.

github-merge-queue bot pushed a commit that referenced this issue Apr 12, 2024
The current checker does not check that all graph outputs are defined.
This PR adds such a check. (See issue
#6040 ).

Note: We probably also want a separate check to ensure that a
graph-input is not used as a graph-output. But leaving that for a
separate PR, since it is a distinctly different check (and perhaps might
require a different discussion).

Redo of PR #6070

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants