Skip to content
This repository was archived by the owner on Jul 10, 2025. It is now read-only.

Conversation

@saxenasaurabh
Copy link
Member

@saxenasaurabh saxenasaurabh commented Dec 4, 2019

Comment period is open till 12/19/19.

Single python code path for eager and graph

Status Proposed
RFC # 184
Author Saurabh Saxena (srbs@google.com)
Sponsors Alex Passos, Gaurav Jain
Updated 2019-12-03

Objective

This proposal discusses merging the graph building and eager op-dispatch code-paths in python and moving the FuncGraph capturing logic and gradient tape bookkeeping into C++.

@brijk7 brijk7 changed the title Single python code path for eager and graph RFC: Single python code path for eager and graph Dec 5, 2019
@brijk7 brijk7 added the RFC: Proposed RFC Design Document label Dec 5, 2019
@saxenasaurabh
Copy link
Member Author

Design Review Notes:

  • Changes:
    • TF_EagerContextDeleteGraph API added to delete the graph and metadata such as captures
    • TF_NewAbstractOp extended to include the op_type
  • Discussion on whether to expose C or C++ APIs
    • Claim is that python language binding already uses C++ and thus results in a C++ -> C -> C++ switch
    • Language bindings such as Java & JS require a C API
    • C API could be of concern if it resulted in poorer performance due to API constraints. However that doesn’t seem to be the case.
    • ABI stability is important across versions which is provided by C
    • Consensus is to stick with a C API as the benefits of C++ are not clear and C API has critical use cases.
    • Short discussion that we can possibly deprecate APIs such as TFE_Execute due to limited usage (1 in courier, 1 in swift, some experimental). We could also make existing APIs call into new APIs
  • Question about API change TF_TensorHandleToTensor to be TF_TensorHandleToEagerTensor?
    • Unresolved. However, no strong objections to change the name.
  • Do we support different ordering of tape popping
    • Forward prop cares about higher level ops to not watch low level ops.
    • Agreement is to enforce a stack.
  • Python gradients will still be supported through callback mechanism.
    • New mechanism will be a gradual migration of python gradients.
    • Registration avoids gradient permanently associated in the name.
    • No being differentiable - op
      • Register Gradient function
      • Implicitly not a good idea because you can get the wrong gradient
      • Python Registry still needed for interop
  • We plan on using pybind11
  • Debugability
    • Don’t need the debuggability at this layer pretty low level
    • We might have compositions
      • Complicated debugging
      • Tfdbg might need to be extended to show the internal operations that it executed
  • Function Registration
    • Local graph and trickle it out - will just use a global registry since that’s what we do anyways
  • Outstanding Keras question
    • Protocol buffer constructor from a nodedef convenient but new APIs will offer a series of APIs for the same functionality
  • We should make it clear that this API is not final and subject to change.

@saxenasaurabh
Copy link
Member Author

@brijk7 could you help merge this RFC please? Thanks!

@brijk7 brijk7 merged commit 500aa14 into tensorflow:master Jan 13, 2020
@brijk7 brijk7 added RFC: Accepted RFC Design Document: Accepted by Review and removed RFC: Proposed RFC Design Document labels Jan 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes RFC: Accepted RFC Design Document: Accepted by Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants