-
Notifications
You must be signed in to change notification settings - Fork 214
Create, save, load and run models using a new functional API #112
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
Conversation
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 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 ℹ️ Googlers: Go here for more info. |
@Shajan , I think you need to consent to the CLA, see message above, thank you |
@googlebot I consent. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noted a few small things, but overall it looks good.
runner.feed(t.getName(), tensor); | ||
}); | ||
|
||
Map<String, TensorInfo> outputToNode = signatureDef.getOutputsMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we guaranteed that the iteration order of the signature's outputs map is consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation, the ordering of signature's output map need not be consistent.
Here is why:
We depend on the ordering of output returned in the call List<Tensor<?>> resultTensors = runner.run()
. Which depends on the order we pass in at runner.fetch(t.getName())
, that happens to be the iteration order of signatureDef.getOutputsMap()
.
In runHelper, See creation of outputOpIndices
based on order of outputs.add(output)
, which gets passed into Session.run(..)
Session.run uses outputOpIndices
as the order of output, which is implemented in resolveOutputs
and TF_Output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it will be consistent within an execution of call
but could change across different VM executions if the signatureDef.getOutputsMap()
is backed by a regular HashMap rather than a LinkedHashMap?
I ask because we have a tendency to iterate the value set of maps in demo code, and I don't want users to think those will be consistent if they aren't. Usually it's because there is only a single output, but if we're not maintaining ordering then people might be confused if they try to apply what we do in the demos/tests to larger problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order we fetch the tensors does not really matter, as we don't return the outputs as a list but as a map, forcing the user to retrieve them by name instead of by ordinal position, like Session.Run
does. And since this is not demo code neither, I think it is safe enough to leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really correct? One line does outputToNode.values().forEach(t -> runner.fetch(t.getName()));
and the other for (String nodeName: outputToNode.keySet()) {
- is there any guarantee of consistent ordering between values() and keySet()!?
The code very much depends on this and there's no test to confirm (ConcreteFunctionTest.java only uses the simpler call()-method).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protobuf uses LinkedHashMap
internally, so yeah it's fine. Wouldn't be a bad idea to convert it to one manually though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use .values()
in both to raise fewer red flags? Though relying on a Map
producing the same values() order is perhaps also questionable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not if it's a LinkedHashMap
, that's the point of the class, it provides stable iteration over a Map with minimal overhead.
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/ConcreteFunction.java
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/SavedModelBundle.java
Outdated
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/SavedModelBundle.java
Outdated
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/SavedModelBundle.java
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/Signature.java
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/main/java/org/tensorflow/Signature.java
Outdated
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/test/java/org/tensorflow/ConcreteFunctionTest.java
Show resolved
Hide resolved
tensorflow-core/tensorflow-core-api/src/test/java/org/tensorflow/ConcreteFunctionTest.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,45 @@ | |||
Saved model created using Python @tf.function, tensorflow version 2.3.0. | |||
|
|||
Python code used to create saved model below: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just check this in as a python file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karllessard any guidance on having python files in the java repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shajan , what do you think? I think it could be a Python file, as long as we add a notice on top of that the file is only there for reference and is not being used anywhere in the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved forward and changed the file type to Python, please let me know if you think of any reason I should revert it to a text file.
Python models that contain tf.function is inconvenient to be consumed by Java clients. This proposal provides an API to (a) Invoke a tf.function, given the signature name (b) Retrieve the node name in the graph corresponding to a tf.function Co-authored-by: Shajan Dasan <[email protected]> Save models as functions (#103) * Draft: Java API to use tf.function available on SavedModel. (#89) Python models that contain tf.function is inconvenient to be consumed by Java clients. This proposal provides an API to (a) Invoke a tf.function, given the signature name (b) Retrieve the node name in the graph corresponding to a tf.function Co-authored-by: Shajan Dasan <[email protected]> * Change API for creating concrete functions and exporting them to a saved model Co-authored-by: Karl Lessard <[email protected]> Rename signature name to key Print function signature when converting to String Add method that returns the signature of all functions in a saved model Add unit tests for python created SavedModel with tf.function
09c136e
to
8cd74fc
Compare
@@ -0,0 +1,48 @@ | |||
# Saved model created using Python @tf.function, tensorflow version 2.3.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a copyright statement at the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -0,0 +1,45 @@ | |||
Saved model created using Python @tf.function, tensorflow version 2.3.0. | |||
|
|||
Python code used to create saved model below: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karllessard any guidance on having python files in the java repo?
Introducing a new
ConcreteFunction
class, which allows you to create graphs, load/save models and run inference using the same functional API. See documentation in this PR for some examples.Exporting SavedModels only supports session-centric models, i.e. models that has a single main graph. This means that if multiple functions are saved with the model, they should all share the same graph for now. That limitation will be addressed later.
CC\ @Shajan