Skip to content

Change Loop op with maximum iterations input M equals to empty string #1971

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

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

hwangdeyu
Copy link
Contributor

@hwangdeyu hwangdeyu commented Jun 16, 2022

while_loop maximum iterations -1 means infinity in tf, but in tf2onnx it's set to max_int to get the same behavior so far.
Refer to Loop spec input M, pass empty string to skip should be better.

fixes #1969
Signed-off-by: Deyu Huang [email protected]

@hwangdeyu
Copy link
Contributor Author

no need to add new unit tests cause there are a lots of existing tests can cover it.

@hwangdeyu hwangdeyu requested a review from fatcat-z June 16, 2022 08:08
Copy link
Collaborator

@fatcat-z fatcat-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@hwangdeyu hwangdeyu merged commit b027bb2 into onnx:main Jun 17, 2022
@hwangdeyu hwangdeyu deleted the loop_M_inf branch June 17, 2022 01:43
@LoicDagnas
Copy link

@hwangdeyu I might be wrong but having an empty string for the loop M input make the symbolic shape inference of ONNX runtime failed here. Since M parameter is marked as optional in the official ONNX documentation, it might be fixed on onnxruntime side?

@hwangdeyu
Copy link
Contributor Author

hwangdeyu commented Aug 23, 2022

Yeah, M should pass empty string to skip in ONNX spec. I don't run into the shape inference error. It's ok to create an issue in ONNXRuntime. Or you share your case with us a simple reproduction script code then we will take a look.

@hwangdeyu Deyu Huang FTE I might be wrong but having an empty string for the loop M input make the symbolic shape inference of ONNX runtime failed here. Since M parameter is marked as optional in the official ONNX documentation, it might be fixed on onnxruntime side?

@LoicDagnas
Copy link

Running the following command:

python -m onnxruntime.tools.symbolic_shape_infer
    --input path/to/model.onnx
    --output output-model.onnx
    --auto_merge

work using a model converted with tf2onnx==1.11.1 but failed with the same model converted with tf2onnx==1.12.0

Here is the shape inference error:

Traceback (most recent call last):
  File "C:\Users\loic.dagnas\AppData\Local\Programs\Python\Python39\lib\runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Users\loic.dagnas\AppData\Local\Programs\Python\Python39\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 2424, in <module>
    out_mp = SymbolicShapeInference.infer_shapes(
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 2357, in infer_shapes
    all_shapes_inferred = symbolic_shape_inference._infer_impl()
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 2127, in _infer_impl
    self.dispatcher_[node.op_type](node)
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 1066, in _infer_Loop
    si.CopyFrom(self.known_vi_[node.input[i]])
KeyError: ''

Here is the model in ONNX format
https://drive.google.com/file/d/19jOktShuha40ib2CRGSugfhi_kIjZh_p/view?usp=sharing

If it is preferable, I'll open an issue on ONNXRuntime repo.

@hwangdeyu
Copy link
Contributor Author

Running the following command:

python -m onnxruntime.tools.symbolic_shape_infer
    --input path/to/model.onnx
    --output output-model.onnx
    --auto_merge

work using a model converted with tf2onnx==1.11.1 but failed with the same model converted with tf2onnx==1.12.0

Here is the shape inference error:

Traceback (most recent call last):
  File "C:\Users\loic.dagnas\AppData\Local\Programs\Python\Python39\lib\runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Users\loic.dagnas\AppData\Local\Programs\Python\Python39\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 2424, in <module>
    out_mp = SymbolicShapeInference.infer_shapes(
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 2357, in infer_shapes
    all_shapes_inferred = symbolic_shape_inference._infer_impl()
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 2127, in _infer_impl
    self.dispatcher_[node.op_type](node)
  File "C:\dev\ml\debug_arnaud\venv\lib\site-packages\onnxruntime\tools\symbolic_shape_infer.py", line 1066, in _infer_Loop
    si.CopyFrom(self.known_vi_[node.input[i]])
KeyError: ''

Here is the model in ONNX format https://drive.google.com/file/d/19jOktShuha40ib2CRGSugfhi_kIjZh_p/view?usp=sharing

If it is preferable, I'll open an issue on ONNXRuntime repo.

It looks ok for me that M is empty string, I think you can try to create an issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loop op with maximum iterations should match the onnx spec
3 participants