Adds a closeable session result

[Craigacp]

This PR changes the output of Session.Runner.run() from List<Tensor> to a Session.Result class which is autocloseable and has both string and index accessors. This is an intermediate step before we have proper resource scopes (#354) which we don't currently have time to work on.

One open question is if we should change the return type of TensorFunction.call(Map<String,Tensor>) so it returns the Result class instead of Map<String,Tensor>. If so, then it probably should be promoted into a top level class rather than a static inner class on Session.

I'll run it through the formatter after review.

[rnett]

One open question is if we should change the return type of TensorFunction.call(Map<String,Tensor>) so it returns the Result class instead of Map<String,Tensor>. If so, then it probably should be promoted into a top level class rather than a static inner class on Session.

IMO we should.

I'm not sure what happens if you double close a tensor but I think it's worthy checking that its nothing bad, or doing something to avoid it. Using the Result class shouldn't prevent closing individual tensors from it, and then the whole result.

[Craigacp]

I think I'd prefer that it returned Result rather than Map<String,Tensor> as well though the asymmetry with the method argument is a bit annoying. @karllessard thoughts?

[karllessard]

(sorry for the delay @Craigacp)

I definitely think that we need to support both TensorFunction.call and Session.run with the same Result class, for simplicity.

In addition, if that's possible, the same structure could be used also to pass the input tensors to a function call, meaning that a user could instantiate it by passing a list of tensors (so instead of Result should be called something like TensorMap?). The danger of doing this though is to make sure that tensors won't leak if one failed to be initialize. For example in here:

try (Tensor.mapOf(
    "x", TFloat64.tensorOf(...),
    "y", TFloat32.vectorOf(...),
    "z", TInt32.vectorOf(...)
)) {
   ...
}

What will happen with x and y if z fails to allocate? It will throw before the autocloseable map is created. This could be handled I guess using lambdas but I'm just throwing ideas here.

[Craigacp]

Yeah, so the separate lifetime issue is why it isn't used as a way of supplying arguments to a call. My justification is that users are in control of Tensor creation for arguments, so they can manage the lifetimes, however for the return values of Session.run and TensorFunction.call we create the tensors and so should provide a lifetime management mechanism. I can definitely see the other side of the argument though, as passing in Map<String,Tensor> and getting back Result which is near enough an AutoCloseable Map<String,Tensor> feels wrong. It feels wrong when I do it in ONNX Runtime too. But it does avoid resource leaks of the kind you mention.

[Craigacp]

I've refactored TensorFunction.call so it returns Result and moved Result so it's a top level class rather than an inner class on Session.

[karllessard]

I think it can still be useful in some cases for a user to retain the lifetime of a specific tensor beyond the scope of the Result, and it shouldn't be hard to do using reference counts internally.

What about adding a getAndRetain(...) endpoint to do this? If not being added as part of this PR, I can play around it after.

[Craigacp]

Rather than do reference counting it could have a BitSet denoting if the tensor should be closed? It would be a one way operation, you could getAndRetain and then the lifetime of the tensor is up to you with no way of putting it back under the Result management.

[karllessard]

I already use reference counting for managing sparse tensors, I think if we can stick internally to a single method for extending the life of a tensor, it would be better.

[Craigacp]

After some investigation we couldn't use the retainReference idiom to extend the lifespan of the Tensor as then the RawTensor's PointerScope loses track of it. So we're going to leave the getAndRetain method as a todo for when we have a look at managing Tensor lifespans better.

[karllessard]

Do you want to run spotless now @Craigacp ?

[Craigacp]

Done.