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

Fix keras Conv2D BiasAdd fuse #1796

Merged
merged 6 commits into from
Jan 24, 2022
Merged

Conversation

hwangdeyu
Copy link
Contributor

fixes #1741.

Signed-off-by: hwangdeyu dejack953@outlook.com

tf2onnx/convert.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Dec 10, 2021

This pull request introduces 1 alert when merging 98f72d4 into 72d6460 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@hwangdeyu hwangdeyu force-pushed the conv2d_bias_add_fuse branch from 98f72d4 to d070b6d Compare December 13, 2021 10:03
@hwangdeyu hwangdeyu force-pushed the conv2d_bias_add_fuse branch 3 times, most recently from 008217e to 23b6712 Compare December 22, 2021 03:25
hwangdeyu and others added 4 commits December 23, 2021 04:04
Signed-off-by: hwangdeyu <dejack953@outlook.com>
Signed-off-by: hwangdeyu <dejack953@outlook.com>
Signed-off-by: hwangdeyu <dejack953@outlook.com>
Signed-off-by: hwangdeyu <dejack953@outlook.com>
Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>
Co-authored-by: fatcat-z <jiz@microsoft.com>
@hwangdeyu
Copy link
Contributor Author

Hi @guschmue,
after adding the tf_optimize() in `_from_keras_tf1', the optimize() may make it model change. So all the keras test models using model predict are failed.
Do you have any idea to fix this cases?

@guschmue
Copy link
Contributor

guschmue commented Jan 3, 2022

tf_optimize() sure will change the graph but should not change numerical results. We call it for every path (just not the one in this issue) - must be some problem how we call it in this case.

@hwangdeyu
Copy link
Contributor Author

tf_optimize() sure will change the graph but should not change numerical results. We call it for every path (just not the one in this issue) - must be some problem how we call it in this case.

Yeah, there may have been a misunderstanding in my comment before.

I mean that after adding the optimize change code, some tests are failed in model.predict test_api.py#L68 and throw the error:

tensorflow.python.framework.errors_impl.NotFoundError: PruneForTargets: Some target nodes not found: predict/group_deps 

But test_api.py #L65 L66 can be passed. So the onnx graph is ok and onnxruntime can be run successfully and get the same numerical results with original keras model.
Now I'm tring to figure out how the tf_optimize() change the graph and make model predict failed.

@hwangdeyu hwangdeyu force-pushed the conv2d_bias_add_fuse branch 3 times, most recently from 6aa6b5d to 036a3ce Compare January 20, 2022 09:01
Signed-off-by: hwangdeyu <dejack953@outlook.com>
Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>
Co-authored-by: fatcat-z <jiz@microsoft.com>
@hwangdeyu hwangdeyu force-pushed the conv2d_bias_add_fuse branch from 036a3ce to 50ea318 Compare January 21, 2022 09:20
tests/test_backend.py Outdated Show resolved Hide resolved
Signed-off-by: hwangdeyu <dejack953@outlook.com>
@hwangdeyu hwangdeyu merged commit 59cfa79 into onnx:master Jan 24, 2022
@hwangdeyu hwangdeyu deleted the conv2d_bias_add_fuse branch January 28, 2022 08:18
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.

TF1-Keras ZeroPadding2D + Conv2D Bias Term Not Fused.
3 participants