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.

Sign up for GitHub

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

Jump to bottom

[Tensorflow Frontend] Add saved model support and infer shape w/o add_shapes=True #2493

Merged
merged 3 commits into from
Feb 9, 2019
Merged

[Tensorflow Frontend] Add saved model support and infer shape w/o add_shapes=True #2493

merged 3 commits into from
Feb 9, 2019

Conversation

yongwww
Copy link
Member

@yongwww yongwww commented Jan 23, 2019

  • Add a wrapper "tensorflow_parser" to handle Tensorflow frozen single pb file, tensorflow saved model and checkpoints
  • Infer shapes without "add_shapes=True", make the frontend more user friendly, much easier to support saved model
  • Upstream from Amazon team, if possible we'd like to keep the commit history for our further investigation

yongwww and others added 2 commits January 22, 2019 19:44
@yongwww
Add tf parser wrapper, infer shape automatically
c93e921
@zhiics @yongwww
Support TensorFlow saved model
ec1000e 
TF parser: return the consistent error message to error handler
@yongwww
Copy link
Member Author

yongwww commented Jan 23, 2019

@srkreddy1238 @yzhliu @icemelon9 would be great if you guys help review it

@srkreddy1238
Copy link
Contributor

@yongwww thanks for saved bundle support. Will review it.
I think we could keep commit history too as long as the individual commits logically differential and doesn't break CI :).
I think we may need to consider saved model support for Relay as well.

@yongwww
Copy link
Member Author

yongwww commented Jan 23, 2019 •
edited
Loading

I think we may need to consider saved model support for Relay as well.

Thanks, sure, will spend some time on that

@zhiics
Copy link
Member

zhiics commented Jan 23, 2019

@srkreddy1238 Yes, we will spend work on Relay part as well, but probably in a different PR. @yongwww It looks that there are some lint problems. Please fix them first.

@yongwww yongwww force-pushed the master branch 2 times, most recently from 4d0203d to ae78620 Compare January 23, 2019 21:00
@yongwww
Copy link
Member Author

yongwww commented Jan 23, 2019

@zhiics fixed lint issues, passed CI

srkreddy1238
srkreddy1238 requested changes Jan 24, 2019
View reviewed changes
nnvm/python/nnvm/frontend/util/tensorflow_parser.py Outdated Show resolved Hide resolved
nnvm/python/nnvm/frontend/util/tensorflow_parser.py Outdated Show resolved Hide resolved
nnvm/python/nnvm/frontend/tensorflow.py Outdated Show resolved Hide resolved
nnvm/python/nnvm/frontend/tensorflow.py Show resolved Hide resolved
nnvm/python/nnvm/frontend/tensorflow.py Show resolved Hide resolved
nnvm/python/nnvm/frontend/tensorflow.py Outdated Show resolved Hide resolved
@yzhliu yzhliu mentioned this pull request Jan 28, 2019
Closed
@yongwww
Copy link
Member Author

yongwww commented Jan 28, 2019

@srkreddy1238 incorporated your comments, pls help take a look at it again. Thanks!

@zhiics
Copy link
Member

zhiics commented Jan 30, 2019

@srkreddy1238 Could you please take a look at this PR when you have some cycles? @yongwww has addressed your comments. Thank you.

yzhliu
yzhliu reviewed Feb 2, 2019
View reviewed changes
Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

@srkreddy1238 please review again, thanks.

nnvm/python/nnvm/frontend/tensorflow.py Outdated Show resolved Hide resolved
@srkreddy1238
Copy link
Contributor

@yongwww sorry about the delay I was occupied on some other task for some days. Will conclude this PR by Tomorrow.

srkreddy1238
srkreddy1238 requested changes Feb 4, 2019
View reviewed changes
nnvm/python/nnvm/frontend/tensorflow.py Outdated Show resolved Hide resolved
nnvm/python/nnvm/frontend/tensorflow.py Outdated Show resolved Hide resolved
@yongwww
Copy link
Member Author

yongwww commented Feb 5, 2019

@srkreddy1238 resolved your comments, please take another look

yzhliu
yzhliu approved these changes Feb 7, 2019
View reviewed changes
Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

@srkreddy1238 Could you help to review again?

srkreddy1238
srkreddy1238 requested changes Feb 7, 2019
View reviewed changes
Copy link
Contributor

@srkreddy1238 srkreddy1238 left a comment

Choose a reason for hiding this comment

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

almost good to go, few changes.

nnvm/python/nnvm/frontend/tensorflow.py Outdated Show resolved Hide resolved
nnvm/python/nnvm/frontend/tensorflow.py Show resolved Hide resolved
nnvm/python/nnvm/frontend/tensorflow.py Outdated Show resolved Hide resolved
@yongwww
Get tags of saved model automatically
412f9fb 
Remove exception trail in tf parser error message

Fix lint

Fix comments
@yongwww
Copy link
Member Author

yongwww commented Feb 8, 2019

@srkreddy1238 could you pls take a another look?

@srkreddy1238
Copy link
Contributor

@yzhliu there was a request to keep commit history for this PR.
Not sure individual commits pass the CI. Please advice and handle the merging.

@yzhliu yzhliu merged commit f347b52 into apache:master Feb 9, 2019
@yzhliu
Copy link
Member

yzhliu commented Feb 9, 2019 •
edited
Loading

Thanks @yongwww @srkreddy1238 I did rebase-merge per request, while it is not recommended as it prevents github attach related PR # for these commits, also individual commit might not be valid as @srkreddy1238 mentioned. Please avoid if possible.

@tqchen
Copy link
Member

tqchen commented Feb 9, 2019

Squash merge is always the preferred approach. While it might prevent us from getting some of the precise contribution history, in general we do not count commits anyway so it is fine

@yongwww
Copy link
Member Author

yongwww commented Feb 10, 2019 •
edited
Loading

@tqchen @yzhliu @srkreddy1238 thanks all, guys! totally agree. I understand your concern, we will use squash merge the next time. My initial thought is keeping some history is helpful for us (aws) to upstream and downstream, anyway, we can handle it and avoid this case for sure.

@tqchen
Copy link
Member

tqchen commented Feb 10, 2019

usually rebase will resolve most of the problem, regardless of which type of merging method

@yzhliu yzhliu mentioned this pull request Mar 2, 2019
Closed
28 tasks
@yongwww yongwww mentioned this pull request Mar 12, 2019
Merged
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srkreddy1238 srkreddy1238 srkreddy1238 approved these changes

@yzhliu yzhliu yzhliu approved these changes

Assignees

@srkreddy1238 srkreddy1238

Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yongwww @srkreddy1238 @zhiics @yzhliu @tqchen