Skip to content

Metrics Add init(Ops) method #337

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

Closed
wants to merge 7 commits into from

Conversation

JimClarke5
Copy link
Contributor

This PR modifies Metrics to add an init(Ops) method from the org.tensorflow.framework.metrics.impl.Initializable interface. Also, provided optional CTORS without the Ops tf parameter. This will allow a Metric instance to be initialized at a later time, once the Ops instance is created, e.g. Model. This allows the Metric to be defined without a prior dependency on Ops. There is also an onInit(Consumer<Ops>) method to register a function that is invoked when init(Ops) is called.

This PR does not change any of the logic of the Metric classes, other than possibly overriding the init(Ops) method to create class specific Variables.
This PR does not modify Variable initializers and it is still required to do a session.run() on resetStates() before doing any other calls that reference an internal Variable. This should be resolved after #179 and #327 PRs are merged.

I still have not added the Ops parameter to call methods [call(Ops tf, ...)], it is still not clear to me what the semantic should be after init(Ops) or a previous call(Ops tf, ...) were already invoked, and the internal variables were created under a different scope than the Ops tf now being passed. If we can come to an agreement on the semantics, then I can add it.
Right now, the Metric.init(Ops) method will just return the original this.tf instance, if not null, and ignore the passed in Ops tf. When called the first time, Metric.init(Ops) returns a sub-scoped Ops based on the passed in Ops.

… an Ops parameter. This does not address adding an Ops parameter to any call methods.
@JimClarke5 JimClarke5 changed the title Add init(Ops) method and remove requirement to have an Ops parameter in the CTOR Metrics Add init(Ops) method Jun 14, 2021
@JimClarke5
Copy link
Contributor Author

I am aware of the build failure. For some reason my command line git was out of sync with IntelliJ git, so the new unit tests were never actually executed on my end. They were executed with the old code from the master branch. My command line git was stuck on master and I ended up rebooting and now things are back in sync (weird). I will fix the code as required and re-push any changes.

@rnett
Copy link
Contributor

rnett commented Jun 16, 2021

For reference, #179 is probably going to have to wait until tensorflow 2.6 releases for the resource variable gradients, unless we want to provide that API without gradients initially.

@rnett
Copy link
Contributor

rnett commented Jun 16, 2021

That or the custom gradients PR.

@karllessard
Copy link
Collaborator

@JimClarke5 , would it make sense to have only the Ops added to the updateStateList methods instead (and initialize the variables only on the first call)?

@JimClarke5
Copy link
Contributor Author

@karllessard the problem with initializing the variables in updateStateList is the issue with having to run session.run() on the variable initialization. I will look at modifying updateStateList to do an Assign rather than an AssignAdd on the first pass, that may clear that up. Another factor might be the reuse of the Operand from updateStateList. If I use Assign, then a second session.run(updateStateOp) would not be right. I am not sure if this is a real concern (Does it actually ever happen in the real world?) or not.

Also, there would an issue if result gets called before updateStateList. Right now it would throw a variable exception if the total and count variables are not initialized. I need to check this.

Obviously, things would be easier with @rnett 's updates, but that may be a while off.

@JimClarke5
Copy link
Contributor Author

I checked keras.Model and it re-calls update_state each training/test step, so I can use the Assign/AssignAdd logic. I will make the changes.

rnett and others added 2 commits June 19, 2021 16:09
resetStates and result. Also removed Ops from CTORs.

Also reworked variable assignment so that there is no need to initialize the variables as I added logic to use a tf.assign on the first pass rather than a tf.assignAdd.
@google-cla
Copy link

google-cla bot commented Jun 19, 2021

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
Copy link

google-cla bot commented Jun 19, 2021

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
Copy link
Contributor Author

@karllessard I got the signer's error again on this branch. All I did was a rebase against master. The rebase was clean with no suggestions to merge.

@JimClarke5
Copy link
Contributor Author

The latest push changes the Metric's Ops parameter. The Ops parameter has been removed from the CTORs, and added to updateStateList (updateState), resetState, and result. I also changed the logic so that the first assignment to variables is using tf.assign, and subsequent assignments use tf.assignAdd. Now there is no dependency on initiilizialing the variables outside of these methods.

@rnett
Copy link
Contributor

rnett commented Jun 19, 2021

I think it's cause you have the Use OP_NAME constant instead of hard coding commit in your tree.

@rnett
Copy link
Contributor

rnett commented Jun 19, 2021

@googlebot I consent.

@JimClarke5
Copy link
Contributor Author

This PR is being closed and will be replaced with a new PR based on the changes from InitScope that was recently merged.

@JimClarke5 JimClarke5 closed this Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants