-
Notifications
You must be signed in to change notification settings - Fork 22
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
Re-enable eager mode #163
Re-enable eager mode #163
Conversation
ed5d8d8
to
88fdb70
Compare
b2267b0
to
8e15af5
Compare
e9728d4
to
2f465c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions and minor comments. Nothing that needs to be changed, though, so I'll approve this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me. I resolved all of Eric's comments that were done, but I think there's one left; are you still working on that @drasmuss or is that being made into a separate story? All suggested changes are done; fixing up then merging!
It could be non-obvious which input value was triggering this warning.
We had disabled eager mode for performance reasons, but there have been some recent performance improvements so we're trying it out again.
This also necessitated some changes to how the build system is set up. Since the
TensorGraph.call
function may be called multiple times in eager mode, rather than once per graph, we need to separate theOpBuilder.__init__
(which we only want to happen once) from the newOpBuilder.build_pre
(which we want to happen each timecall
is executed).Fixes #160 (in that in eager-mode you can create constants larger than 2GB; the problem persists in graph mode, but that's just a limitation of graph mode).
Fixes #156 (in eager mode; as above, problem persists in graph mode but I don't think it's worth looking into any further there as it's an underlying problem with TensorFlow's graph mode).
Fixes #109 (in eager mode, same as above).