Skip to content

Expose input and output types from Signature #182

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

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

jbentleyEG
Copy link
Contributor

Currently you can get the names of the inputs and outputs from a Signature, but there is no publicly accessible method that allows you to get the return types other than toString().

I was unable to get the tests building on my machine, but hopefully this change is small enough that it won't be an issue.

@google-cla
Copy link

google-cla bot commented Jan 5, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@karllessard
Copy link
Collaborator

Thanks @jbentleyEG , looks good but do you have an example on how you actually use it? (maybe adding a small unit test could be great)

Also I'm wondering what is better: return types mapped to their tensor name, like you did, or simply return the whole TensorInfo input/output list, which would also expose their shape, wdyt? Would it also fit in your use case?

@jbentleyEG
Copy link
Contributor Author

Agreed, I will re-write it to expose the shape as well. That isn't required for my use case, but I imagine that others will need that.

How about just making it a pass-through to the signatureDef method:

/**
  * Returns the names of the inputs in this signature mapped to the expected tensor
  */
 public Map<String, TensorInfo> getInputs() {
   return signatureDef.getInputsMap();
 }

@jbentleyEG
Copy link
Contributor Author

@googlebot I signed it!

@karllessard
Copy link
Collaborator

karllessard commented Jan 7, 2021

Agreed, I will re-write it to expose the shape as well. That isn't required for my use case, but I imagine that others will need that.

How about just making it a pass-through to the signatureDef method:

/**
  * Returns the names of the inputs in this signature mapped to the expected tensor
  */
 public Map<String, TensorInfo> getInputs() {
   return signatureDef.getInputsMap();
 }

That is what I had initially in mind as well, but now I'm thinking that we might want to wrap up that TensorInfo class for our own so we can return a Shape instead of a TensorShapeProto, since the former is being used everywhere in the Java client.

For example we could wrap and convert this in a inner class, like Signature.TensorInfo or Tensor.Metadata? Just throwing some ideas. And to limit the amount of work, I would simply focus on datatype() (returns the DataType enum found in the protos) and shape() (returns a Shape build from a TensorShapeProto), leaving the other available fields for later. Wdyt?

@jbentleyEG
Copy link
Contributor Author

That makes sense. My original concern about just exposing the whole thing is that it's a mutable object, an inner class bypasses that. I'll submit an update later today.

@rnett
Copy link
Contributor

rnett commented Jan 18, 2021

For tests, did you try running maven with the target integration-test instead of test (in tensorflow-core-api, with the dev profile)? I ran into the same thing earlier.

@karllessard
Copy link
Collaborator

Btw @jbentleyEG , do you still intend to push a new PR for this?

@rnett
Copy link
Contributor

rnett commented Jan 26, 2021

@jbentleyEG @karllessard I can take this over, if necessary.

@jbentleyEG
Copy link
Contributor Author

Sorry, got pulled onto another task and got distracted away from this. Yes, I still plan to do a PR for this.

@jbentleyEG
Copy link
Contributor Author

@karllessard PR updated

@@ -32,6 +34,16 @@
/** The default signature key, when not provided */
public static final String DEFAULT_KEY = "serving_default";

public static class TensorDescription {
public final DataType dataType;
public final long[] shape;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jbentleyEG , looks good but as we discussed, can we return the shape as a Shape object to the users instead of a long[]?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jbentleyEG, just checking, do you agree to make the proposed change above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the flakiness, I keep getting pulled away by other tasks. I wasn't able to generate the Shape directly from the TensorShapeProto, but I was able to recreate it from the dimensions.

Copy link
Collaborator

@karllessard karllessard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jbentleyEG !

@karllessard karllessard merged commit 26e8dc3 into tensorflow:master Feb 4, 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