RFC: String Tensor Unification

[gharibian]

Feedback period will be open until 2019-04-26

ABI Stable Unified String Tensors

Status	Proposed
Author(s)	Dero Gharibian (dero@google.com)
Sponsor	Gunhan Gulsoy (gunan@google.com)
Updated	2019-04-11

Objective

To unify and define the byte interface of a string tensor across TensorFlow’s C API (TF_STRING), TF Lite (kTfLiteString), and TF-Core/C++ (DT_STRING) with the purpose of enabling modular TensorFlow and mitigating the performance overhead of string tensor conversions.

[jdduke]

Just to be clear, we will have to remain backward compatible with the old layout. That is, we'll have to continue supporting execution of models exported with the old layout, as well as the new layout, and be able to distinguish between the two.

[gharibian]

I updated the doc to address this.

[jdduke]

Echoing the above point, we cannot rely on updating all models. We'll have to decide on an approach which gracefully handles the existing layout for models that exist in the wild.

[gharibian]

I included your solution of adding a new TFLite enum type for tstring, which should gracefully handle existing existing layouts.

[sjamesr]

Here are the notes from the design review session:

• Is the pre-alloc type superfluous? Or can we mark that type as reserved?
  • @mrry: If someone else allocated the memory for us, it would be useful to have pre-alloc. Maybe pre-alloc isn't the best way to do this, however. Possibly not crucial.
• @alextp: Do you still want to replace all occurrences of string within tensorflow with tstring?
  • DT_STRING tensors use tensorflow::tstring
  • Not all occurrences of string become tensorflow::tstring. The typedef std::string string is not touched.
• @m3bm3b: suppose you omit a variant, and you discover you need it, how hard is it to add a new?
  • 4 types are allowed, another bit will have to be sacrificed if you want to add a 5th type
  • Backwards compatibility with TF Lite becomes an issue, this is probably more difficult than actually adding the type, need to work with TF Lite to maintain backwards compatibility with serialized tensors.
• @alextp: wouldn't things be much simpler if lvalue assignment was not supported?
  • Lots of downstream kernels use lvalue assignment, to support those kernels we need lvalue assignment. Doesn't add too much complexity, string already supports it.
  • @alextp: lvalue assignments might cause you to have to convert away from your most efficient representation
  • @edloper: Is there an issue for lvalue assignment for pre-alloc type? No, we just decay into the large string type.
• @alextp : is this design document externalized? Yes.
• Can we have a string_view-style implementation? This is tough because tstring assumes that it has allocated the underlying memory. Converting between different tstring types would be awkward.

Apologies if I missed anything.

[ewilderj]

@gharibian please update the RFC with any changes coming out of the review meeting, and update the Status to "Accepted". Thanks!

[karllessard]

I don't know if it's too late to comment on this I just want to confirm one thing: when we talk about "language bindings expected to modify a raw buffer", we are talking about modifying the buffer directly from the Java code (for example) and not from its bridge to the C API (JNI in this case), I'm I right?

[gharibian]

Not quite; this is specifically about the C API, and the fact that accessors/mutators do not exist in the C-API for C string tensor construction.

In the Java case, string tensors are created in the bridge JNI code by directly manipulating a buffer (though nothing precludes one from creating it in Java and passing it through via JNI). With the purposed update, the string tensors will still be created in the JNI bridge code, but instead of directly manipulating a buffer as is currently done [1], the JNI bridge will use new methods in the TF C API for string tensor construction.

[1] https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/java/src/main/native/tensor_jni.cc?#L246

[karllessard]

Ok. I think it could still be useful though to have a string layout simple enough to let the Java client (and other language bindings) to write them directly into the tensor buffer, using some custom writers. We might achieve better performances by doing this and that is what we will probably do for all other datatypes.

Similarly, the language bindings could read a string value directly from the tensor buffer, useful in eager execution environment. What do you think?

[gharibian]

For the creation of string tensors, of the four string sub-types proposed, the kOffsetType fits the bill of being a "simple" layout which allows for direct writing to a buffer. Creating an kOffsetType string tensor would mitigate the current expensive conversion of TF_STRING -> DT_STRING. The layout is similar to the current C-String/TF_STRING layout (i.e. single block of contiguous memory), but has endianness defined and is compatible with eigen.

Things get a bit complicated for reading and modifying existing string tensors---since in those situations, the string could be any of the four types, and memory ownership comes into play with kLargeType.

Ideally, in order to future proof potential changes in byte layout, it would still be beneficial to utilize the new proposed functions in the C-API when reading/modifying/constructing string tensors. We will provide functions for creating string tensors from lists of strings in order to reduce function call overhead.

Alternatively, since we no longer will be using std::string as a string container, and the layout of strings will be well-defined and reside in TF, we could have duplicate implementations in java for better performance. For full duplication, kLargeTypes may need an additional field for a deallocator.

In short, this design does not preclude your suggestions of direct access to DT_STRING buffers, and, in fact, enables it (since the ABI of std::string is not defined). We can assess the potential performance gains vs additional complexity in having a duplicate Java implementation once the DT_STRING scalar is mapped to tensorflow::tstring instead of std::string.

[karllessard]

Perfect, thanks for the detailed answer

[gharibian]

@gharibian please update the RFC with any changes coming out of the review meeting, and update the Status to "Accepted". Thanks!

@ewilderj Updates are merged-in, and the Status has been set to "Accepted". Ready for review/merge.