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

[Java] Graph environment decoupling in preparation of eager execution #24858

Merged

Conversation

karllessard
Copy link
Contributor

@karllessard karllessard commented Jan 11, 2019

This PR is the first of a series of pull requests that enables eager execution in the TensorFlow Java client. It might look ambitious at first but is pretty basic when understanding what is coming next.

This PR decouples the actual implementation for building operations, which is tightly bound to graph execution, from its public interface. Doing this in a single PR helps to get a better picture of the targeted architecture and discuss about it if needed.

So in brief:

  • Operation concrete class became a GraphOperation that implements the Operation interface by extending from the AbstractOperation abstract class
  • OperationBuilder concrete class became a GraphOperationBuilder that implements the OperationBuilder interface
  • Graph implements the ExecutionEnvironment interface to be used by the Ops classes

Once we are happy with this solution, we can start adding Operation, OperationBuilder and ExecutionEnvironment implementations for eager execution, which are pretty straightforward according to my previous POC.

I tried to preserve backward compatibility as much as possible. I ended up with the following exceptions:

  • Constructor in Output is now package-private and accepts only AbstractOperation instances
    ** Note: I could have play around to keep it public but for me, it sounds wrong to instantiate an Output directly instead of calling the available type-safe Operation.output method

  • Scope.graph() has been changed to Scope.env() to return an instance of ExecutionEnvironment instead instead of a Graph
    ** Note: This can only impacts clients already using the Ops API and even there, unless they did something tricky, the Ops classes will take care of the change flawlessly.

See diagram below for an overview of the targeted architecture:

eager-diagrams

@karllessard karllessard changed the title [Java] Graph environment decoupling [Java] Graph environment decoupling in preparation of eager execution Jan 11, 2019
@ymodak ymodak self-assigned this Jan 11, 2019
@ymodak ymodak added size:XL CL Change Size:Extra Large awaiting review Pull request awaiting review labels Jan 11, 2019
@ymodak ymodak added this to Assigned Reviewer in PR Queue via automation Jan 11, 2019
@karllessard
Copy link
Contributor Author

FYI, just applied a small change to this PR

@tensorflowbutler
Copy link
Member

Nagging Reviewer @asimshankar: You have been added as a reviewer to this pull request. Please add your review or reassign. It has been 44 days with no activity and the awaiting review label has been applied.

@karllessard
Copy link
Contributor Author

I think this should be assigned to @sjamesr instead.

@sjamesr sjamesr requested review from sjamesr and removed request for asimshankar March 5, 2019 21:12
sjamesr
sjamesr previously approved these changes Mar 5, 2019
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Mar 5, 2019
@ymodak ymodak added kokoro:force-run Tests on submitted change and removed awaiting review Pull request awaiting review labels Mar 5, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 5, 2019
@ymodak ymodak added the ready to pull PR ready for merge process label Mar 6, 2019
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Mar 10, 2019
@karllessard
Copy link
Contributor Author

Thanks for approving the PR, @sjamesr

Since it was created a while ago, I had to rebase the code on current branch to get it compile, you might need to reapprove.

Also, I took that opportunity to rename GraphNode* classes to GraphOperation* for consistency with the upcoming eager implementation (i.e. EagerOperation*), it was bugging me a bit. I updated the diagram above accordingly, sorry for the trouble.

@karllessard
Copy link
Contributor Author

Friendly reminder @sjamesr to please re-approve this PR,

Since it is a XL pull request, I'm just afraid that I'll need to rebase it more than once if we wait too long between approvals. Thanks!

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 21, 2019
@rthadur rthadur removed the ready to pull PR ready for merge process label Mar 21, 2019
@rthadur rthadur assigned rthadur and unassigned ymodak Mar 21, 2019
@rthadur rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 21, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 21, 2019
@tensorflow-copybara tensorflow-copybara merged commit 76be6ee into tensorflow:master Mar 21, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Mar 21, 2019
tensorflow-copybara pushed a commit that referenced this pull request Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:XL CL Change Size:Extra Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants