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

Add a script to profile the conversion of a model #1077

Merged
merged 18 commits into from Sep 30, 2020
Merged

Conversation

xadupre
Copy link
Collaborator

@xadupre xadupre commented Aug 26, 2020

No description provided.

@@ -0,0 +1,101 @@
# coding: utf-8
Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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...

Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dependency removed.

@guschmue
Copy link
Collaborator

Isn't this PR seems to be included in here #1060 ?

@lgtm-com
Copy link

lgtm-com bot commented Aug 27, 2020

This pull request introduces 1 alert when merging febb438 into 6ec695b - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@sdpython
Copy link
Contributor

I merged it to check it was working. I was about to remove the merge.

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request introduces 1 alert when merging feab45c into 6ec695b - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request introduces 1 alert when merging 6f22abf into 6ec695b - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request introduces 1 alert when merging db25e5d into 6ec695b - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 28, 2020

This pull request introduces 1 alert when merging b9985c6 into 6ec695b - view on LGTM.com

new alerts:

  • 1 for Unused import

@xadupre xadupre requested a review from guschmue August 31, 2020 16:03
@xadupre xadupre merged commit d5e56d5 into onnx:master Sep 30, 2020
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.

None yet

3 participants