API convention to take runtime type parameters in constructors of tensor-type-parameterized classes?

[deansher]

Our API design deliberately uses lots of tensor-type-parameterized classes. Here's a typical example:

public class BinaryCrossentropy<U extends TNumber, T extends TNumber>
    extends MeanMetricWrapper<U, T> implements LossMetric<T> {

Of course, Java generic type parameters are erased at runtime, so the implementation of this class doesn't necessarily have Class objects for types U and T if it needs them.

In a few cases, our initial implementation of such classes has already found itself in this situation, so we have required Class parameters in constructors. Here's an example:

public class MeanMetricWrapper<U extends TNumber, T extends TNumber> extends Mean<U, T> {
   . . .
  protected MeanMetricWrapper(Ops tf, String name, long seed, Class<T> type) {
    super(tf, name, seed, type);
  }

But suppose we didn't need a stored runtime class when we originally implemented this class, but then eventually made changes that required it? (Or suppose we need the runtime type for U!) To evolve the constructor API, we would have to choose between unpleasant alternatives:

• Add a new constructor that takes the runtime class, and make it a runtime error to use the new functionality if you didn't use that constructor.
• Punt on adding the new functionality to the existing class hierarchy, perhaps instead creating a MeanMetricWrapperV2 and then also forking subclasses as needed.
• Or make a breaking change by adding a runtime type parameter to existing constructors (which in this case would break a ton of subclass APIs).

I wonder whether we should make it a consistent pattern that classes parameterized by tensor types take the runtime types in their constructors?

In Java, this will make constructor invocation less convenient. Here's an example in existing code:

      BinaryCrossentropy<TFloat32, TFloat64> instance =
          new BinaryCrossentropy<>(tf, "BCE_testUnweighted", false, 0, 1001L, TFloat64.class);

Note the TFloat64.class at the end of the call. In Java 1.8, this is entirely an added burden: it is redundant with the second TFloat64 type parameter, but both are required. However, with the addition of local variable type inference in Java 10, this redundancy will be eliminated. The only extra burden then will be the .class. In Kotlin, this could be entirely smoothed over with inline factory methods.

[rnett]

In Kotlin, this could be entirely smoothed over with inline factory methods.

If we do go this route, it would mean adding a tensorflow-framework-kotlin package and code generation to generate the factor methods. Not that that's prohibitive or anything, but something to consider. It'll probably be necessary anyways, eventually.

Also, these utility methods won't really work well unless the constructor only takes one type, see #176 and my comment on the metrics PR (if you can infer the type, it would work, but something like val metric = BinaryCrossentropy<TFloat64>() wouldn't).

At least by your example, it seems like this is more "when classes have a persistent tensor state (a variable), take the runtime type in the constructor". We're not taking a class for U after all. Imo this is good, and necessary anyways for initialization. Is that what you meant?

[deansher]

I'm not thinking specifically of classes that have persistent tensor state, although that's one way the situation can arise. I am thinking more generally about situations where

• a class is parameterized by a tensor type
• and doesn't need a stored instance of the type's runtime Class<> counterpart at first,
• but will evolve to later need it.

If this happens at all often and can't always be anticipated, then we may want to consistently take all type parameters as runtime constructor parameters to allow for such an evolution.

When a class has a tensor type parameter that is only used to restrict the types of method parameters, and when the parameter types do store the tensor type, there is no need to store the runtime type ahead of time, because it can be obtained from a method parameter.

For example, consider this method at org/tensorflow/framework/metrics/impl/Reduce.java:118:

  public <V extends TNumber> List<Op> updateStateList(Operand<U> values, Operand<V> sampleWeights) {

Let's focus on U, which is a type parameter of Reduce that has no corresponding Class<U> runtime parameter in Reduce's constructors. In updateStateList that's no problem, because we can obtain the runtime type from values. And indeed, in the method's implementation, we do just that:

    Operand<U> lValues = values;
  . . .
      lSampleWeights = CastHelper.cast(getTF(), sampleWeights, lValues.type());

So far, so good. (Although we depend here on the fact that Operand stored its own runtime type parameter!)

The challenge arises if the class implementation needs a runtime Class<T> for one of its type parameters T in a situation where no method parameter provided an access path to the Class<T>. Here's an example at org/tensorflow/framework/metrics/impl/Reduce.java:82:

  private void setupVars() {
    if (total == null) {
      total = getTF().withName(totalName).variable(Shape.scalar(), resultType);
    }
    . . .

We needed the runtime resultType to create the variable total. Which is why Reduce takes that runtime type as a constructor parameter.

But it doesn't take the runtime type of its other type parameter, U, as a constructor parameter, because so far this situation does not arise with U. What happens if it arises in the future? We have no good API evolution path to accommodate it.

[Craigacp]

I don't think we should add unused arguments to constructors or methods. If the implementation changes sufficiently that we require a class argument then we should add it later, but let's not try and predict how things will evolve over time. At the moment we can make breaking changes to the API, it's not stable and won't be until we decide to anoint a 1.0 release. At the point after it's stabilised and we need to add a class parameter then I think we should document that it uses the first solution @deansher suggested (i.e. using new functionality via the old constructor throws a runtime exception), and deprecate the old way for removal in the next major release.

Having unused constructor arguments for conformity seems like bad practice as linters & IDEs will complain about unused arguments, so we'd need to silence them everywhere across the different tools people use.

[deansher]

We do have considerable time remaining before we have to decide this. And yes, @Craigacp's evolution path would be workable and follows common patterns. I guess the worst API-evolution situation would be if we discovered a bug that could only be fixed with access to a stored runtime tensor type. Our experience between now and 1.0 will tell us whether that's a common situation.

I do see another side to @Craigacp's concern about unused constructor arguments. The ideal pattern might be to always store those arguments and provide public accessors for them. Just as our framework code very frequently relies on the fact that Operand<T> stores the runtime Class<T> and provides a type() method to obtain it, clients of our framework code may later want to rely on our framework classes doing the same.

[Craigacp]

How do we decide which type parameters need the classes saving?

[deansher]

I am picturing a convention that whenever classes have tensor-type type parameters, we take those as runtime constructor parameters either first or right after the tf. Perhaps the convention would be narrower, but if so, I don't yet know in what direction.

[Craigacp]

Ok. Do we have any idea when such an evolution would be required? I expect that most of these classes will have fixed signatures because they implement a bit of math, and we'd add new classes to implement new math.

[deansher]

I spent a while looking through our existing framework classes to see if I can construct a compelling example, and I can't. I'll have to rely on the rest of you to decide whether that's lack of imagination on my part, or more fundamentally that it isn't going to happen.

My state of proficiency on deep learning right now could be described roughly as "I have read a whole lot, plus implemented MNIST." I have to work with whatever nose I have for code generally. In this case, it smells funny to me that our framework APIs provide our classes with the runtime types we already know we need, but leave us with no good way to obtain the runtime types of other type parameters if we wish we had them in the future.

[rnett]

I'd suggest letting Jim do his "move type params out of the class where possible" PR first like was mentioned in #180 (comment) before we evaluate this in detail. I suspect a lot of the extra type parameters that we might need classes for later, like U, won't be class type parameters afterwards.

[deansher]

Yes, I agree.

[karllessard]

Just to point out that @JimClarke5 's PR has been merged: #204