enforce onnx conversion in CI #3628
Conversation
There was a problem hiding this comment.
The behavior is rather complex and I would like to see a docstring around what is the expected behavior. I think we will always try to convert to ONNX when possible and if we force it, we will raise an error when not possible.
There are 3 flags : ONNX_EXPORT_ENABLED, settings.convert_to_onnx and TEST_ENFORCE_ONNX_CONVERSION and I think they should have a comment associated to distinguish them
| @@ -18,6 +21,11 @@ | |||
| from tensorflow.python.framework import graph_util | |||
| from mlagents.trainers import tensorflow_to_barracuda as tf2bc | |||
|
|
|||
| if LooseVersion(tf.__version__) < LooseVersion("1.12.0"): | |||
There was a problem hiding this comment.
Should we also filter out tf 2.x ?
There was a problem hiding this comment.
I don't want to explicitly filter it out. A future version of tf2onnx should support f2 2.x, in which case the import should succeed.
| else: | ||
| if _enforce_onnx_conversion(): | ||
| raise RuntimeError( | ||
| "ONNX conversion enforced, but couldn't import dependencies." |
There was a problem hiding this comment.
This is not true, it could also be because the version does not match.
There was a problem hiding this comment.
Good point, I'll update the comment here. It's not something a user will ever see (unless they added the environment variable), though.
|
I realize now, this is for release, not master. |
Proposed change(s)
Cherry pick of #3600 to the release branch
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments