Implements both Tensor and NdArray interfaces from same instance

[karllessard]

Hi everone,

I've been playing around lately with different components of the core API to see if it could be possible that both the NdArray and Tensor interfaces be implemented by the same object. This way, we can avoid this transition from one to another.

For example, right now when we get hold on a tensor, we basically just wrap up the native pointer and expose a few methods to retrieve its shape and datatype. If we need to access its data directly from the JVM, we map its memory to an NdArray in a second step and keep a reference to that array for future access. This is done by invoking data() on a tensor instance. e.g.

try (Tensor<TFloat32> input = TFloat32.scalarOf(2.0f)) {
    TFloat32 inputData = input.data();
    System.out.println("Value of input is " + inputData.getFloat());

    try (Session s = new Session(someGraph)) {
        try (Tensor<TFloat32> output = s.runner().feed("input", input).run().get(0).expect(TFloat32.class)) {
            TFloat32 outputData = output.data();
            System.out.println("Value of output is  " + outputData.getFloat());
        }
    }
}
try (EagerSession s = EagerSession.create()) {
    Tensor<TFloat32> t = TFloat32.vectorOf(1.0f, 2.0f, 3.0f);
    TFloat32 tData = t.data();
    tData.scalars().forEach { s ->
        System.out.println(String.valueOf(s.getFloat()));
    }
}

This actual PR change the behaviour so that the TType itself (TFloat32 here) will implement both the NdArray and the Tensor interface (Tensor is actually a class so needed to be converted to an interface). The same code above would look like this:

try (TFloat32 input = TFloat32.scalarOf(2.0f)) {
    System.out.println("Value of input is " + input.getFloat());

    try (Session s = new Session(someGraph)) {
        try (TFloat32 output = s.runner().feed("input", input).run().get(0).expect(TFloat32.class)) {
            System.out.println("Value of output is  " + output.getFloat());
        }
    }
}
try (EagerSession s = EagerSession.create()) {
    TFloat32 t = TFloat32.vectorOf(1.0f, 2.0f, 3.0f);
    t.scalars().forEach { s ->
        System.out.println(String.valueOf(s.getFloat()));
    }
}

It is also important to noticed that I made the changes totally backward compatible with the actual implementation. For example these lines will still work since TFloat32 now implements Tensor<TFloat32>:

Tensor<TFloat32> t = TFloat32.scalarOf(2.0f);  // t is an instance of `TFloat32`
TFloat32 tData = t.data(); // `this` is returned by `data()`

Please take a look and let me know if you think I should cleanup this work and make it a real PR or not. To help understanding better the proposed changes, here are two diagrams comparing both implementations, taking TFloat32 as example:

Previous:

New:

[Craigacp]

Is it possible to have TType extend Tensor? Or get rid of it entirely? It seems like all the places where you'd want a TType you now want a TType & Tensor, and that suggests that the interfaces should have some kind of typing relationship.

[karllessard]

@Craigacp : unfortunately no, I thought of that but Tensor carries a TType as a generic parameter, while TType is not generalized. So if TType extends from Tensor, it will extends from it without parameter, like this:

public interface TType extends Tensor

But in this case, if I have an TType interface that wants to extends Tensor for a specific datatype, it would be conflicting with the unparamaterized version inferred by TType itself, i.e.

public interface TFloat32 extends TFloat, Tensor<TFloat32> // not allowed there because both `Tensor` and `Tensor<TFloat32>` are extended

A solution would be to have Tensor extending TType instead, so everywhere we see <T extends TType & Tensor>, we could change it to <T extends Tensor>. But I find it even more confusing because conceptually, a TType is a Tensor but not the other way around. Wdyt?

[Craigacp]

Maybe TType could get a recursive type bound? public interface TType<T extends TType> extends Tensor<T> and then public interface TFloat32 extends TFloat<TFloat32>?

Though that's starting to get messy as well, and I'm not clear it would compile.

[karllessard]

yes, I tried to leave the TType without parameter by purpose... I'm wondering though if we could get completely rid of it as it is just used as a marker telling that a given interface can be carried as a parameter by all our tensors, outputs, operands, ops, etc.

Maybe it can now be replaced simply by Tensor everywhere? (and I can have Tensor extending from a deprecated TType just for the sake of backward compatibility, unless we still don't care much at this point...)

It will still be strange to have Tensor<? extends Tensor>, unless we remove the parameter out of it but that comes to a price as well (e.g. tensor.data() and tensor.dataType() will become type-unsafe).

[Craigacp]

Could we not make Tensor recursive in that case, i.e. Tensor<T extends Tensor>? I think until we've made an alpha release that we should just make the changes without deprecation. Given all the people who are developing with it can comfortably fit in a video call it's not too big an ask. Or at least not from my perspective.

[karllessard]

Could we not make Tensor recursive in that case, i.e. Tensor?

Yes it looks like our solutions are converging to this and that we should get rid entirely of TType (TNumber & future friends can remain though). I'll give it a try

[karllessard]

Ok I did some tests. So dropping TType entirely leaves the API pretty neat by having all entities carrying a generic parameter extending from Tensor instead: Output<T extends Tensor>, Operand<T extends Tensor>, etc. which is a bit more explicit that we are carrying the type of a tensor.

The only "negative" impact I would say is that now, you cannot just filter out at compile time tensors of the wrong datatype in the signature of a method. For example, Shapes.flatten() used to accept only shape tensors of a numeric type by specifying:

static <U extends TNumber> flatten(..., Shape<U> shape, DataType<U> dtype)

Now, since type families are not extended by the tensor class, you need to specify both boundaries, otherwise classes like DataType will complain that type U is not bound to the Tensor type

static <U extends Tensor & TNumber> flatten(..., Shape<U> shape, DataType<U> dtype)

I still personally prefer this new approach, and declaring both boundaries in the signature of such method can also be seen as being more explicit (I want U to be a "Tensor" of "Number"s). If we all agree on this, I'll push the new version in this PR draft. I'll need to refactor a bit the op generator though

[Craigacp]

Could you push out that version so I can look through? I'm having some difficulty seeing the relationships between types, and its probably easier for you to push it than for me to make equivalent changes to my own fork.

[karllessard]

Here @Craigacp , I pushed it into this branch

So I ended up tweaking it a bit more to cover most cases that I find useful. Now, Tensor extends from NdArray itself, and carries a Java type, not a TType. e.g., for TFloat32, the hierarchy looks approximately like this:

// tensor/ndarray hierarchy
TFloat32 -> FloatTensor -> Tensor<Float> -> NdArray<Float>

// type family
TFloat32 -> TFloating -> TNumber -> TType

So the tensor hierarchy is used to map the data to Java (primitive) types while the type families are used to classify the tensor based on their TF datatype.

[Craigacp]

This is a public method now right? That's kinda unpleasant.

[karllessard]

I totally agree. But I did not found (yet) a way to avoid it.

[karllessard]

Ok I now have a third implementation of pretty much the same thing: https://github.com/karllessard/tensorflow-java/tree/tensor-as-ndarrays-3

The tensor and type family relationships remain pretty much like in my second draft, the main difference is that instead of having *TensorImpl extending from the class *DenseNdArray, it now extends from AbstractTensor and delegate all data I/O operations to a private instance of *DenseNdArray.

This allows us to keep, for example, nativeHandle() protected in AbstractTensor instead of exposing publicly. Also we can validate that the tensor is still valid before any I/O operations. Finally, there is no more need of having multiple bounds like ? extends Tensor & TType since now TType extends from Tensor directly

What is nice about it though is that I ended up extending the Operand interface from TType. What this means is that now, we can pass a tensor directly as an operand of a op when in eager mode, instead of converting it to a constant explicitly, i.e.

Ops tf = Ops.create();
TFloat32 tensor = TFloat32.scalarOf(1.0f);  // very simple tensor just for the sake of this example
TFloat32 result = tf.math.add(tensor, tf.constant(2.0f)).asTensor();
assertEquals(3.0f, result.getFloat(), 0.0f);

I still have to play around with generic types to make sure everything make sense and is inferred properly but before, I'll keep it like this for now and wait for your feedbacks or for the next community call to discuss more about it.

[zaleslaw]

Hi, @karllessard I've viewed this PR and the second one

A few moments, which give me ambivalent feelings

1. T extends Tensor & TNumber - in ops signatures. Do we have a chance to define common abstract class or interface here.
2. public methods for nativeHandler
3. // tensor/ndarray hierarchy
   TFloat32 -> FloatTensor -> Tensor -> NdArray - I'd like to the hidden generic in the descedant class approach, but TFloat32 (without any suffix) for Tensor could lead to the misunderstanding
4. Powerful RawTensor (very tempting to program in it)

I agree that proposed changes solves the problem of two worlds hierarchies merging (but I'm not sure about benefits).
Also, could you rewrite on it one of our models (like CNN) and convert it to java-11 with vars and without required types near the variables. Maybe it will help us to feel new changes deeply and fix possible problems. My propose here = use the examples like integration tests for new proposed API changes here.

What do you think?

[Craigacp]

One thing I'd like to note in passing about the name TFloat32, is that it's very similar to Nvidia's TF32 or "Tensor Float 32", which isn't actually 32-bits (it's 19-bits), which they've introduced in the new A100 GPUs. So we'll need to be careful about documenting it. Fortunately at the moment TF32 isn't a storage format, it's only used internally by the GPU, but that could always change.

[karllessard]

Hey @zaleslaw , thanks for reviewing, my comments below...

1. T extends Tensor & TNumber - in ops signatures. Do we have a chance to define common abstract class or interface here.

2. public methods for nativeHandler

I got rid of these two issues the latest version, did I left something or did you looked at a previous version?

3. // tensor/ndarray hierarchy
   TFloat32 -> FloatTensor -> Tensor -> NdArray - I'd like to the hidden generic in the descedant class approach, but TFloat32 (without any suffix) for Tensor could lead to the misunderstanding

The "problem" I find is that type is carried everywhere as a generic parameter by Operand, Output, Tensor, etc... and I find Float32Tensor a bit too verbose for being used so often for that purpose. It was already proposed to do such change in the but I was reluctant since tensors and TTypes were two separate concepts (one a native reference and one a data container) but here I'm merging both together so it could make more sense now.

4. Powerful RawTensor (very tempting to program in it)

Again the newest version don't have RawTensor anymore, I really need to clean that PR (or simply start a new one) once I'm done with it. But I realized yesterday that to make sure the proposed design will cover all use cases, I need a better idea if it could handle sparse and ragged tensors in the future.

My propose here = use the examples like integration tests for new proposed API changes here.

I totally agree. I realized though that my daily occupation requires me more time than expected and I might need some help to finalize the whole proposal and adjust the tests in consequence, please let me know if you think you can help me out with this.

Thanks,
Karl

[karllessard]

I’m putting on hold my work on this PR as I’m focusing more on the saved model export now. I’m also starting to have some doubts if the few benefits of these changes are worth it, I’ll try to figure that out with a clearer mind.

[zaleslaw]

It's a good solution, we could return to it later, it will not be lost, of course вт, 25 авг. 2020 г., 4:43 Karl Lessard <[email protected]>:

…

I’m putting on hold my work on this PR as I’m focusing more on the saved model export now. I’m also starting to have some doubts if the few benefits of these changes are worth it, I’ll try to figure that out with a clearer mind. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#92 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJEUHMKI3AV67IL2TGR2R3SCMJKZANCNFSM4PLWU6OQ> .

[karllessard]

It's a good solution, we could return to it later, it will not be lost, of course

It is true that if we keep that latest design, it is pretty much backward compatible with the actual implementation and therefore can easily come up as an improvement for later. If we decide to shuffle a bit more the types though, better do it before our first release. Let's brainstorm back about it in a near future.

[JimClarke5]

I have checked in a TFloating interface as part of the "activation" PR #123. Should there also add a TInteger family interface?

[karllessard]

@JimClarke5 since your PR only required TFloating in its logic, let’s keep it like that for now, thanks!

[karllessard]

Closing, in favor to #139