Fix TF1-Keras Dilated Conv ONNX Exported #1744
Conversation
leimao
force-pushed
the
fix_dilated_conv
branch
4 times, most recently
from
fec81ca to
7768115
Compare
|
@TomWildenhain-Microsoft Could you please review once you get a chance? Thank you. Also because of NVIDIA's policy, I added license headers from NVIDIA in the files. If you think it violated the ONNX contribution rules, please let me know. |
|
Code looks great, really appreciate the unit tests. I'm thinking we probably aren't going to want every company that touches the file appending legal notices. I'll check what our policy is. Are you sure NVIDIA requires those for open source? |
Thank you @TomWildenhain-Microsoft. NVIDIA open source process requires me to add those headers. However, if it violates the Contributor License Agreement (CLA), we can remove them. |
|
Not sure if we can take it. All sources under github.com/onnx use only SPDX-License-Identifier: Apache-2.0@prasanthpul would know. |
|
@prasanthpul Please let me know and I believe similar things might also happen in the future. (I may make some other merge request to the repo again.) Thank you. |
|
@TomWildenhain-Microsoft When is the next tf2onnx release? I just want to make sure that this PR does not miss the release. Thank you. |
leimao
force-pushed
the
fix_dilated_conv
branch
2 times, most recently
from
7768115 to
c14e7c3
Compare
update patch skip cpu test for conv3d ncdhw skip CPU for Conv2D NCHW update Signed-off-by: Lei Mao <[email protected]> Remove NVIDIA license header
leimao
force-pushed
the
fix_dilated_conv
branch
from
0e70394 to
ddac442
Compare
|
@TomWildenhain-Microsoft The NVIDIA license header has been removed. Please review again. Thank you very much. |
|
@TomWildenhain-Microsoft After rebasing to the master branch and run CI tests again, I got many weird errors which did not seem to be related to my patch. Could you please give some insights here? Thank you. |
|
took a quick look - some issue with tf-2.6.1 that was released yesterday I think. |
Thank you very much. Please do let me know if there is any action item from my side. |
|
I think it fails now mostly tf-2.6 which should be fixed with my change. Some tf-2.5 ut fails with lstm errors. |
My pull request has nothing to do with LSTM unless the LSTM uses dilated Conv which is very less likely. Let's merge your pull request on the TF versions for CI to see what will happen. |
|
They released tf-2.5 as tf-2.5.2 nov-1 so maybe it is the same problem like tf-2.6 but I don't understand why my PR passed. |
I am not a CI expert. Is it even possible to run CI tests locally? |
|
@TomWildenhain-Microsoft It seems that there are CI test failings unrelated to this pull request. |
|
@TomWildenhain-Microsoft It seems that the CI is downloading models from an invalid URL. It seems that you guys are using fragile URLs in the repo which is not best practice. |
|
@TomWildenhain-Microsoft It seems that this time the CI works as expected. |
Fix issue #1728.