-
Notifications
You must be signed in to change notification settings - Fork 434
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
Add a script to profile the conversion of a model #1077
Conversation
@@ -0,0 +1,101 @@ | |||
# coding: utf-8 |
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.
We want to limit top level directories - can we stick this under tools?
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.
Done.
@@ -14,3 +14,14 @@ steps: | |||
condition: succeededOrFailed() | |||
env: | |||
CI_ONNX_OPSET: '${{ onnx_opset }}' | |||
|
|||
- bash: | | |||
export TF2ONNX_TEST_BACKEND=$CI_ONNX_BACKEND |
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.
Our ci pipleline is already slow, don't think we want to add benchmarks to it.
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.
Fixed
import fire | ||
import tensorflow as tf | ||
from tf2onnx import tfonnx | ||
from tensorflow.keras.applications import MobileNet |
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.
Iffy to depend on tf.keras since we must support older tf-1.x versions.
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.
We can remove the script from CI, no issue with that, it takes 30 seconds. My plan was to only run it with the latest version of tensorflow. However, the script is failing and it was working a month ago. So I was wondering what change could have caused that. Either my script is wrong, either there is something not covered by the unit tests...
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.
can we not start using fire ? We don't use it anywhere else and want to limit the dependencies.
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.
Dependency removed.
Isn't this PR seems to be included in here #1060 ? |
This pull request introduces 1 alert when merging febb438 into 6ec695b - view on LGTM.com new alerts:
|
I merged it to check it was working. I was about to remove the merge. |
This pull request introduces 1 alert when merging feab45c into 6ec695b - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 6f22abf into 6ec695b - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging db25e5d into 6ec695b - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging b9985c6 into 6ec695b - view on LGTM.com new alerts:
|
No description provided.