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

[Frontend][TF] Fix Placeholder issue #2834

Merged
merged 2 commits into from
Apr 21, 2019
Merged

[Frontend][TF] Fix Placeholder issue #2834

merged 2 commits into from
Apr 21, 2019

Conversation

yongwww
Copy link
Member

@yongwww yongwww commented Mar 16, 2019

This pr is a fix for issue "Math op expect 2 inputs, 1 given" in tf frontend. This frozen pb in the repo can be used to reproduce this issue. The placeholder node is at the end of the graphdef for some reason, so we need to preprocess these ops during first traversal before they are used.
@srkreddy1238 @wweic @kazum please take a look.

@yongwww yongwww force-pushed the relay_tf branch 4 times, most recently from e053e52 to c358f4d Compare March 17, 2019 16:26
@kazum
Copy link
Contributor

kazum commented Mar 18, 2019

@yongwww Can you add a small regression test to tests/python/frontend/tensorflow/test_forward.py to prevent the problem from happening again?

@yongwww
Copy link
Member Author

yongwww commented Mar 18, 2019

@kazum sure, I'll add a test case with a small code snippet or a small .pb graphdef

@srkreddy1238
Copy link
Contributor

srkreddy1238 commented Mar 20, 2019

@yongwww
In general tensorflow export happens by optimisation which internally does a topological ordering. Not sure how this graph was handled.

Anyway this change is good to have.

@yongwww
Copy link
Member Author

yongwww commented Mar 21, 2019

@srkreddy1238 yeah, the graphdef is kind of weird, but it should be valid, probably caused by ProtocolBuffer. We also met this issue when handling control flow stuff since there are cycles in graph, which makes topological ordering behave abnormally.

@yongwww yongwww force-pushed the relay_tf branch 2 times, most recently from 78a2cde to 1a43d06 Compare March 25, 2019 15:34
@kazum
Copy link
Contributor

kazum commented Mar 30, 2019

@yongwww, can you fix the CI error?

@kazum kazum added the status: need update need update based on feedbacks label Mar 30, 2019
@yongwww
Copy link
Member Author

yongwww commented Mar 31, 2019

@kazum I am working on it, will update soon

@yongwww
Copy link
Member Author

yongwww commented Apr 16, 2019

@kazum test case should be okay, works on my local env. In order to pass CI, need to get the pb file (dmlc/web-data#172) uploaded first, could you please take a look at the PRs

@kazum kazum merged commit 51e2e31 into apache:master Apr 21, 2019
@kazum
Copy link
Contributor

kazum commented Apr 21, 2019

Thanks @yongwww @srkreddy1238. This is now merged.

wweic pushed a commit to wweic/tvm that referenced this pull request May 13, 2019
* [Frontend][TF] Fix Placeholder issue

* Add test cases
wweic pushed a commit to neo-ai/tvm that referenced this pull request May 13, 2019
* [Frontend][TF] Fix Placeholder issue

* Add test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants