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

[tests/TF/build] enable missing classification onnx tests and set tensorflow lower bound to 2.11 #1182

Merged
merged 4 commits into from
Apr 29, 2023

Conversation

felixT2K
Copy link
Contributor

This PR:

  • upgrades the minor version of tf2onnx to >=1.14 (which supports TF 2.11)
  • upgrades the minor version of tensorflow to >= 2.11 (first version where the issue seems to be fixed)
  • enables tests

Closes:
#978 and #790 🚀

@felixdittrich92 felixdittrich92 added this to the 0.6.1 milestone Apr 21, 2023
@felixdittrich92 felixdittrich92 added ext: tests Related to tests folder framework: tensorflow Related to TensorFlow backend topic: onnx ONNX-related labels Apr 21, 2023
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #1182 (39ba21a) into main (70ff4fd) will not change coverage.
The diff coverage is n/a.

❗ Current head 39ba21a differs from pull request most recent head de5cf13. Consider uploading reports for the commit de5cf13 to get more accurate results

@@           Coverage Diff           @@
##             main    #1182   +/-   ##
=======================================
  Coverage   95.01%   95.01%           
=======================================
  Files         149      149           
  Lines        6417     6417           
=======================================
  Hits         6097     6097           
  Misses        320      320           
Flag Coverage Δ
unittests 95.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@felixdittrich92
Copy link
Contributor

@frgfm Are you fine with increasing the minimum versions to fix this ? I think it would be also the time to drop py3.6 and 3.7 support and focus on 3.9 and 3.10 (especially CI) with >=3.8,<4

@odulcy-mindee
Copy link
Collaborator

@felixdittrich92 Should it fix:

tensorflow.python.framework.errors_impl.InvalidArgumentError: {{function_node __wrapped__Conv2DBackpropInput_device_/job:localhost/replica:0/task:0/device:CPU:0}} Gradients for grouped convolutions are not supported on CPU. Please file a feature request if you run into this issue. Computed input depth 576 doesn't match filter input depth 1 [Op:Conv2DBackpropInput]

?

@felixT2K
Copy link
Contributor Author

felixT2K commented Apr 25, 2023

@felixdittrich92 Should it fix:

tensorflow.python.framework.errors_impl.InvalidArgumentError: {{function_node __wrapped__Conv2DBackpropInput_device_/job:localhost/replica:0/task:0/device:CPU:0}} Gradients for grouped convolutions are not supported on CPU. Please file a feature request if you run into this issue. Computed input depth 576 doesn't match filter input depth 1 [Op:Conv2DBackpropInput]

?

@odulcy-mindee
Unfortunately no, the problem should actually be fixed since TF 2.9. I also tested it locally and it persists. However, only during training on the CPU (should normally not the case 😅 ). Training on the GPU and inference on the CPU and GPU runs without any problems.

Ref.: https://discuss.tensorflow.org/t/error-message-for-grouped-convolution-backprop-on-cpu-is-uninformative/6323

But this PR fixes the failing onnx exports for TF mobilenet. After this we have finally all models available for exporting into onnx format (up to the logits)

The idea to drop py3.6 and py3.7 support is only to keep the repo + CI tests up to date

@felixT2K
Copy link
Contributor Author

felixT2K commented Apr 26, 2023

@odulcy-mindee fine with the changes in this PR ? I opened #1183 for the grouped conv issue 👍🏼

@frgfm
Copy link
Collaborator

frgfm commented Apr 29, 2023

@frgfm Are you fine with increasing the minimum versions to fix this ? I think it would be also the time to drop py3.6 and 3.7 support and focus on 3.9 and 3.10 (especially CI) with >=3.8,<4

Regarding the version specifiers, yes that's alright but for future debugging purposes, I suggest updating the PR description to specify which new features we want to get out of those recent releases (easier to track down problems in a specific version later on).

About the python version, let me open a quick PR to check that (Python version upgrade can sometimes be tricky when the project on large platform-specific builds like DL frameworks) 👍

Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks again! Only a few suggestions for later debugging 👌

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@felixT2K felixT2K changed the title [tests/TF] enable missing classification onnx tests [tests/TF/build] enable missing classification onnx tests and set tensorflow lower bound to 2.11 Apr 29, 2023
@felixdittrich92 felixdittrich92 added topic: build Related to dependencies and build type: misc Miscellaneous labels Apr 29, 2023
@felixdittrich92 felixdittrich92 merged commit 9aeb947 into mindee:main Apr 29, 2023
53 of 56 checks passed
@felixT2K felixT2K deleted the enable-missing-tf-onnx-tests branch April 29, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: tests Related to tests folder framework: tensorflow Related to TensorFlow backend topic: build Related to dependencies and build topic: onnx ONNX-related type: misc Miscellaneous
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[models] Replace current tf mobilenetv3 implementation with tf.keras.applications implementation
4 participants