Metrics init scope #382

JimClarke5 · 2021-09-09T12:59:05Z

This is a refactoring of org.tensorflow.framework.metrics to use the latest merge for initScope.
In all the metric classes, the Ops parameter was moved out of the CTORs and moved to the access methods defined in a new Metric interface. The old Metric logic was moved to a new class BaseMetric.
None of each metric's logic was changed.

The advantage of this change is that the metrics can be constructed before the Ops is created, which is the case when defining a Model. The initScope feature facilitates this by removing the variable initialization obstacle.

A side effect of this push, is that many of the generated operations in tensorflow-core did not match what was stored in the master repository, so there is over a thousand files changed in that module. Based on what @rnett told me, the locally generated code was correct, but we should verify this.

There were a few changes in other packages to fix JavaDoc issues.

…e CTORs to the access methods defined in Metric, which is now an Interface.

rnett · 2021-09-09T20:31:26Z

For the op changes, you're probably going to want to wait for #378 and then rebase onto it. The local generation is "correct" in that it matches what we have, but some of the ops are misclassified.

karllessard · 2021-09-17T00:21:27Z

Hey @JimClarke5 , can you please rebase this PR? There has been some breaking change as we have upgraded to TF2.6.0, including a new signature for BatchMatMul (I've updated the framework code but maybe your changes here will also require the same update).

JimClarke5 · 2021-09-17T01:01:05Z

Ok, I will change BatchMatMul to MatMul as I found an error in Framework MatMul while testing Optimize minimize
Which is a future PR

…e CTORs to the access methods defined in Metric, which is now an Interface.

… instead of org.tensorflow.op.train.BatchMatMul

…itScope # Conflicts: # tensorflow-core/tensorflow-core-api/src/gen/java/org/tensorflow/op/data/DirectedInterleaveDataset.java # tensorflow-core/tensorflow-core-api/src/gen/java/org/tensorflow/op/math/CompareAndBitpack.java # tensorflow-core/tensorflow-core-api/src/gen/java/org/tensorflow/op/summary/WriteHistogramSummary.java # tensorflow-core/tensorflow-core-api/src/gen/java/org/tensorflow/op/train/BatchMatMul.java # tensorflow-core/tensorflow-core-api/src/gen/resources/ops.pb # tensorflow-framework/src/main/java/org/tensorflow/framework/op/FrameworkOps.java

google-cla · 2021-09-17T16:58:30Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-09-17T17:41:13Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

karllessard · 2021-10-05T00:27:08Z

@JimClarke5 , that PR contains the commits of the TF2.6 upgrade, can you please rebase your branch so that only your commits remain? Thanks

…itScope

google-cla · 2021-10-05T19:41:49Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

JimClarke5 · 2021-10-05T19:49:14Z

@karllessard see if this is better. I am still getting the dreaded @googlebot message, not sure what that is all about.

karllessard · 2021-10-20T01:54:01Z

@JimClarke5 , is there anything you want to update in this PR, since we just merged a few others PRs from @rnett (#376 and #302 ). If your code don't depend nor would benefit from these changes, then I'll start reviewing it, thanks!

…e CTORs to the access methods defined in Metric, which is now an Interface.

… instead of org.tensorflow.op.train.BatchMatMul

…itScope

JimClarke5 · 2021-10-20T21:28:10Z

@karllessard There was nothing specific that I needed from the recent checkins, but I did a rebase of MetricsInitScope using master, anyway. Assuming I did that right it is ready for review.

karllessard · 2021-10-21T13:50:47Z

...orflow-framework/src/main/java/org/tensorflow/framework/metrics/CategoricalCrossentropy.java

+      Operand<? extends TNumber> labels,
+      Operand<? extends TNumber> predictions,
+      Class<U> resultType) {
+    init(tf);


Just throwing an idea here but instead of remembering to call init(tf) before doing any work in a given metric, would it make sense to have the BaseMetric interception the call first, calling init and then an abstract protected method implemented by subclasses where the initialization is assumed to be completed? (e.g. doCall)

If you think that's a good idea but don't want to do it now, we can still merge without it

Do you mean moving the call method to BaseMetric and then calling a new abstract method in the subclass?

Also, it is not just this method, there are also result, resetStates that call init(tf)

Yeah that's what I meant, so you don't have to call init(tf) everywhere in the subclasses. Again, that's just a suggestion.

karllessard · 2021-10-21T13:56:22Z

tensorflow-framework/src/main/java/org/tensorflow/framework/metrics/BaseMetric.java

+  protected void setTF(Ops tf) {
+    checkIsGraph(tf);
+    this.tf = tf;
+  }


Do BaseMetric still needs to store and instance of Ops since it is already available in most endpoints? I can't see a case where it is needed in this PR (but it is not obvious to search in a PR neither)

I believe that once the Metric's variables are created, then subsequent operations would have to use the same Graph to manipulate them. That is why I am storing it. Let me know if my understanding is wrong.

Is there a unit test showing how the metrics are manipulated then after initialization?

karllessard · 2021-10-22T13:35:13Z

All right @JimClarke5 , let's merge this and we can review my previous comments later with another PR if required, not really important. Thanks!

JimClarke5 added 3 commits September 9, 2021 08:36

init scope changes regenerated the core ops.

3338605

Fix JavaDoc errors

0653299

change to use init scope for Variables. Moved Ops parameter out of th…

619fdea

…e CTORs to the access methods defined in Metric, which is now an Interface.

JimClarke5 added 6 commits September 17, 2021 10:44

Fix JavaDoc errors

d26e505

change to use init scope for Variables. Moved Ops parameter out of th…

ab19a39

…e CTORs to the access methods defined in Metric, which is now an Interface.

Fix framework MatMul to call raw op org.tensorflow.op.linacalg.MatMul…

0718d5e

… instead of org.tensorflow.op.train.BatchMatMul

Fix JavaDoc for FameworkOps

fd68feb

Code reformat

770b735

Code reformat

3bfa50c

JimClarke5 added 2 commits October 5, 2021 12:43

Code reformat

895aed5

Merge remote-tracking branch 'origin/MetricsInitScope' into MetricsIn…

d953538

…itScope

Merge branch 'tensorflow:master' into MetricsInitScope

d465240

JimClarke5 added 8 commits October 20, 2021 14:02

Fix JavaDoc errors

24c2ea5

change to use init scope for Variables. Moved Ops parameter out of th…

353d1bb

…e CTORs to the access methods defined in Metric, which is now an Interface.

Fix framework MatMul to call raw op org.tensorflow.op.linacalg.MatMul…

8be8a85

… instead of org.tensorflow.op.train.BatchMatMul

Fix JavaDoc for FameworkOps

74c5b8f

Code reformat

57e2467

Code reformat

1ede549

Fix JavaDoc errors

05cc957

Code reformat

e6ad388

JimClarke5 added 2 commits October 20, 2021 17:24

Merge branch 'tensorflow:master' into MetricsInitScope

1d250af

Merge remote-tracking branch 'origin/MetricsInitScope' into MetricsIn…

e62279c

…itScope

karllessard reviewed Oct 21, 2021

View reviewed changes

karllessard approved these changes Oct 22, 2021

View reviewed changes

karllessard merged commit 55547dd into tensorflow:master Oct 22, 2021

Metrics init scope #382

Metrics init scope #382

Uh oh!

Conversation

JimClarke5 commented Sep 9, 2021

Uh oh!

rnett commented Sep 9, 2021

Uh oh!

karllessard commented Sep 17, 2021

Uh oh!

JimClarke5 commented Sep 17, 2021

Uh oh!

google-cla bot commented Sep 17, 2021

Uh oh!

google-cla bot commented Sep 17, 2021

Uh oh!

karllessard commented Oct 5, 2021

Uh oh!

google-cla bot commented Oct 5, 2021

Uh oh!

JimClarke5 commented Oct 5, 2021

Uh oh!

karllessard commented Oct 20, 2021

Uh oh!

JimClarke5 commented Oct 20, 2021

Uh oh!

karllessard Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

JimClarke5 Oct 21, 2021 • 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!

JimClarke5 Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

karllessard Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

karllessard Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

JimClarke5 Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

karllessard Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

karllessard commented Oct 22, 2021

Uh oh!

Uh oh!

JimClarke5 Oct 21, 2021 •

edited

Loading