Optimizer package for Graph mode by Craigacp · Pull Request #28 · tensorflow/java

Craigacp · 2020-02-08T03:11:17Z

This adds a new subproject tensorflow-training which currently contains org.tensorflow.training.optimizers a package of gradient optimizers which apply the underlying gradient update ops to a TF Graph.

In addition to the optimizers it makes a few small changes in the tensorflow-core-api package: it adds a variables initialiser list to Graph, a method which constructs an initialiser node which initialises all the variables in the graph, plus a variableWithInit method which accepts an Operand which is used to provide the shape, type and initial value for the variable.

There's also an MNIST CNN test in there, which we can move out to wherever it needs to go, and be combined with the other ones.

Before this gets merged it needs better Javadoc in the Optimizer class, and the global variables used by some of the optimizers need wiring into the globals field in the base class. I'll fix those things next week.

karllessard · 2020-02-08T03:47:11Z

@Craigacp , just quickly like that, it looks like you'll need to rebase your PR to resolve a conflict

dhruvrajan · 2020-02-08T03:55:15Z

@Craigacp cool stuff! Did you mean to commit the files under src/annotations/gen? (those get regenerated at build time, if I'm not mistaken?)

Craigacp · 2020-02-08T04:37:59Z

I rebased the branch and the deterministic generation commits went away with the associated conflict, but I seem to have picked up the ci changes that got merged in earlier today.

karllessard

Thanks @Craigacp , this is a very great addition to TF Java. I didn't went through the logic behind the optimizers themselves yet but just dropped quickly a few suggestions you might want to review before applying your next changes.

.github/workflows/ci.yml

tensorflow-core/tensorflow-core-api/src/gen/java/org/tensorflow/op/core/VariableWithInit.java

tensorflow-training/src/main/java/org/tensorflow/training/examples/MNISTTest.java

karllessard · 2020-02-10T02:56:08Z

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/AdaDelta.java

@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.


2020, also shouldn't it be copyright to The Tensorflow Authors like all other files in the project?

We don't have an authors file in this repo, plus this is the text that Oracle Legal requires on all outbound source contributions. I'm not sure if putting it in the AUTHORS file will be sufficient for them. It looks like the main TF repo has an AUTHORS, but then it says to look at CONTRIBUTORS which doesn't exist, so maybe someone at Google should figure out what they want it to look like for all the TF related repos.

karllessard · 2020-02-10T03:00:04Z

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/AdaDelta.java

+
+  private final float rho;
+
+  private final float epsilon;


I don't think we need to enforce it but just FYI, pretty much all TF classes declare their members in this order, including both fields and methods: public, protected, default, private

Do you mean all the public methods & fields, then protected etc, or all the public methods, protected methods, ... then public fields, protected fields etc?

Well in this format, things are grouped per scope, meaning:

public fields

public methods

protected fields

protected methods

... and so on

Again, I don't necessarily want to continue enforcing this in our new repo but maybe we can apply it at least in the core, so that all classes of a single artifact follows the same pattern?

karllessard · 2020-02-10T03:03:50Z

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/Optimizer.java

+
+  protected Optimizer(Graph graph) {
+    this.graph = graph;
+    this.tf = Ops.create(graph).withName(getOptimizerName());


would it be easy to allow the user override the name of the scope for the optimizer ops (instead of getOptimizerName())? Also, maybe a user would like to pass its own instance of Ops (which might already be a subscope of another block or contains control dependencies, etc.)?

It's locked off to Graph at the moment as I don't think any of this works in eager mode, so the type system enforces that. I can allow a hook for the name, and we could relax the Graph check to an instanceof check throwing IllegalArgumentException to supply an ops. Should I keep the Graph entry point as well?

Yes I think it is ok that to have an entry point that accept a Graph but maybe add another that accept an Ops as well, so the user has full control on the name and control dependencies of the optmizers? Or maybe just a String for the name

For instance, does it make sense that a user would want to create two different optimizers of the same type (but with different parameters) for handling different variables? If so, then he'll need to give them different names or he will endup with a name conflict.

I think you understand my questioning here, I'll let you decide what would be the best approach to handle those corner cases.

I added an additional constructor which accepts a Graph and a String for the base name of the operations.

Craigacp · 2020-02-13T02:29:51Z

I've rebased on the latest master, fixed the style issues, put the try with resources in, and updated it to use the @Endpoint annotation for variableWithInit. I've also stubbed out using @Endpoint for the Adam optimizer, and I think it should work, but it would require the optimizers to be in the tensorflow-core-api project as otherwise there will be ugly circular dependencies. TODOs are update the optimizer constructor to allow an ops, rearrange the code so it's ordered like the other bits of the project, and check the behaviour of tf.assign in eager mode.

tensorflow-core/tensorflow-core-api/src/gen/annotations/org/tensorflow/op/Ops.java

karllessard · 2020-02-13T14:11:55Z

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/op/core/CoreOps.java

+ * and return one of them.
+ */
+@Operator
+public abstract class CoreOps {


This class name is a bit misleading, as all other *Ops classes are generated and exposes tf.* endpoints. Here, we are dealing with the endpoint implementations.

I've never been a huge fan of the *Ops name for the generated classes neither (even if that was my idea if I recall correctly...) and we could rename them instead. Like TensorFlowApi, TensorFlowLinearApi, TensorFlowSparseApi, etc. would be better picks.

But if we don't want to do this breaking change then I think we need to come up with something else for this new class.

I renamed it to Helpers, but that's also not a great name.

tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/op/core/CoreOps.java

tensorflow-training/src/main/java/org/tensorflow/training/examples/MNISTTest.java

karllessard · 2020-02-13T23:16:36Z

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/AdaDelta.java

+
+  private final float rho;
+
+  private final float epsilon;


Well in this format, things are grouped per scope, meaning:

public fields

public methods

protected fields

protected methods

... and so on

Again, I don't necessarily want to continue enforcing this in our new repo but maybe we can apply it at least in the core, so that all classes of a single artifact follows the same pattern?

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/Optimizer.java

karllessard · 2020-02-14T14:10:36Z

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/Optimizer.java

+
+  protected Optimizer(Graph graph) {
+    this.graph = graph;
+    this.tf = Ops.create(graph).withName(getOptimizerName());


Yes I think it is ok that to have an entry point that accept a Graph but maybe add another that accept an Ops as well, so the user has full control on the name and control dependencies of the optmizers? Or maybe just a String for the name

For instance, does it make sense that a user would want to create two different optimizers of the same type (but with different parameters) for handling different variables? If so, then he'll need to give them different names or he will endup with a name conflict.

I think you understand my questioning here, I'll let you decide what would be the best approach to handle those corner cases.

karllessard · 2020-02-14T14:13:55Z

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/Optimizer.java

+    List<Operation> variables = new ArrayList<>();
+    Iterator<Operation> opItr = graph.operations();
+    while (opItr.hasNext()) {
+      Operation op = opItr.next();


graph.operations().forEachRemaining maybe?

Also, is it OK that an optimizer is always applied to all variables in the graph? Is this true for all kind of graphs?

That's how they work in Python. It's really hard to make it work any other way without a lot of ceremony.

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/Optimizer.java

tensorflow-training/src/main/java/org/tensorflow/training/optimizers/RMSProp.java

karllessard · 2020-02-14T23:41:13Z

Please don’t forget to rebase your PR before pushing a new version as I just merged some other breaking changes (mainly renaming ˋtf.constant to ˋtf.val and adding ˋtf.array`), thanks

…type.

…raining.

…e MNISTTest.

… the constructors, removing the MNISTTtest.

…ssion.

karllessard requested changes Feb 10, 2020

View reviewed changes

Craigacp force-pushed the ndarray-optimizers branch from 195deff to 7e3459b Compare February 13, 2020 02:09

karllessard requested changes Feb 14, 2020

View reviewed changes

Craigacp and others added 16 commits February 25, 2020 08:34

Initial commit of gradient descent optimizers.

9e92687

Adding Apache 2.0 license header to all optimizer files.

68a353c

Bug fix for the MNISTTest.

b3f4be8

Refactor to uptake latest tensorflow-core changes.

d1868ea

Added type safety and updates for new api.

6d189cc

Small changes, plus a fix for DataTypes to include references to the …

53e438a

…type.

Repackaging the optimizers into tensorflow-training, org.tensorflow.t…

83140b4

…raining.

Initial commit of gradient descent optimizers.

b2ac923

Adding Apache 2.0 license header to all optimizer files.

e7eb2e8

Bug fix for the MNISTTest.

3d63564

Refactor to uptake latest tensorflow-core changes.

ed71dc5

Added type safety and updates for new api.

b054449

Repackaging the optimizers into tensorflow-training, org.tensorflow.t…

b29be50

…raining.

Delete pom.xml

6cdb55c

Googlify with IntelliJ's Google Java Style Guide formatter.

b9d64c5

Bumping the copyright year, and switching to try-with-resources in th…

6ae5ace

…e MNISTTest.

Craigacp force-pushed the ndarray-optimizers branch from 02c25c2 to 6ae5ace Compare February 25, 2020 13:41

Craigacp added 5 commits February 25, 2020 10:08

Updating variableWithInit to use @endpoint.

56429e8

Refactorings after code review.

5d8cb69

Adding a couple of lines to the gitignore.

51f5d47

Adding a bit of documentation, threading the named operations through…

66876ed

… the constructors, removing the MNISTTtest.

Adding a guard to prevent variableWithInit being called on an EagerSe…

7a2fd25

…ssion.

Update Ops.java

1b98f52

karllessard approved these changes Mar 2, 2020

View reviewed changes

karllessard merged commit 9107991 into tensorflow:master Mar 2, 2020

		@@ -0,0 +1,92 @@
		/*
		* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.

Conversation

Craigacp commented Feb 8, 2020

Uh oh!

karllessard commented Feb 8, 2020

Uh oh!

dhruvrajan commented Feb 8, 2020

Uh oh!

Craigacp commented Feb 8, 2020

Uh oh!

karllessard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Craigacp Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Craigacp commented Feb 13, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

karllessard commented Feb 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Craigacp Feb 10, 2020 •

edited

Loading