One: [nnfwapi] Design an API for getting tensor model I/O index by tensor name

[wateret]

Initial Suggestion : #3340

It will be contained in nnfw_dev.h first(if possible) then we can move it to nnfw.h when it becomes mature.

[wateret]

Ideas...

Methods from #3340

Method 1 based

Add a name member to nnfw_tensorinfo.

typedef struct nnfw_tensorinfo
{
  NNFW_TYPE dtype;  // 4b
  int32_t rank;     // 4b
  int32_t dims[NNFW_MAX_RANK]; // 24b
  // EXTENDED PART BEGIN
  char *name;     // 4/8b
  char name[32];  // 32b
  // EXTENDED PART END
} nnfw_tensorinfo;

• (Probably?) backward-compatible as long as name is appended at last
• Should add it to nnfw.h directly as we are changing the existing struct

Two types for name

• name as a char pointer
  
  
  
  • Who manages this pointer?
  
  
• name as a fixed-length string
  
  
  
  • What if the name length exceeds the fixed-length?
  
  
  • Struct takes a lot of space
  
  

Method 2 based

✨This method has been chosen.

Changed to return model I/O index instead of tensor index.

NNFW_STATUS nnfw_input_tensorindex(nnfw_session *session, const char *tensorname, uint32_t *index);
NNFW_STATUS nnfw_output_tensorindex(nnfw_session *session, const char *tensorname, uint32_t *index);

Another method 1

✨This method has been chosen. Here's the reason - https://github.com/Samsung/ONE/issues/3391#issuecomment-668577807

The reverse of Method 2.

NNFW_STATUS nnfw_get_input_tensorname(nnfw_session* session, uint32_t index, char* /* out */ buffer, uint32_t buffer_size);
NNFW_STATUS nnfw_get_output_tensorname(nnfw_session* session, uint32_t index, char* /* out */ buffer, uint32_t buffer_size);

[glistening]

@wateret

Method 1 based

Add a name member to nnfw_tensorinfo.

I am thinking of Method 1 when I heard the API request.

name as a char pointer
Who manages this pointer?

I think runtime can manage name. names could be allocated and copied during loading model. name will be deallocated when the session is closed. We may allocate a chunk instead of allocation of [number of input + number of output].

name as a fixed-length string

name can be const char *.

[wateret]

name as a char pointer
Who manages this pointer?

I think runtime can manage name. names could be allocated and copied during loading model. name will be deallocated when the session is closed. We may allocate a chunk instead of allocation of [number of input + number of output].

That makes sense, I agree. Then the type must be const char * definitely.

---

And now the only concern about Method 1 is whether extending nnfw_tensorinfo breaks backward-compatibility or not.

• Offsets of the legacy members are going to be the same as before as we are appending a member, so it looks good.
• The struct size increases
  
  
  
  • nnfw_apply_tensorinfo will break since it accepts nnfw_tensorinfo as value (But it is deprecated, so it may be OK)
  
  
  • What if user wrote some code that relies on the struct size? → I'm still not sure
  
  

[glistening]

@wateret I didn't see backward compabitility issue.

What if user wrote some code that relies on the struct size?

Could you please give me an example for this case? If the developers are going to use new "nnfw.h`, his application code will be recompiled. I don't think there would be an issue.

[wateret]

If the developers are going to use new "nnfw.h`, his application code will be recompiled.

If that's the case, I agree that it has no problem. But I'm not sure if we can call it backward-compatible.

I think it should work without app recompilation to be backward-compatible. If an app is recompiled, I would say it is written for the new version. For example, you have an app that runs with ONE 1.6.0 which is installed on the system. Then the system updates ONE to 1.7.0 but no changes on the app. If it just works, I think we can say 1.7.0 is backward-compatible with 1.6.0.

Please correct me if I'm wrong, I'm still not sure about it.

[glistening]

I think it should work without app recompilation to be backward-compatible.

Then, most modules that I know is not backward-compatible (including tensorflw lite).

[wateret]

Well, NN API is.

most modules that I know

Could you be more specific?

(including tensorflow lite)

Isn't it C++ API? I think they aren't backend-compatible too. And I thought it is OK since their schema file is backward-compatible.

[glistening]

Could you be more specific?

As I mentioned, tensorflow lite is an example.

Isn't it C++ API?

No. It has C API. See tensorflow/lite/c/c_api.h. It has Python and Java API. I cannot find C++ API.

[wateret]

Thanks, I was not awaare of C API. FYI C++ API is here -https://www.tensorflow.org/lite/api_docs

[wateret]

I looked at tensorflow lite C API.

In https://github.com/tensorflow/tensorflow/blob/master/tensorflow/lite/c/common.h , it says

// NOTE: The order of values in these structs are "semi-ABI stable". New values
// should be added only to the end of structs and never reordered.

If assuming all the app code is recompiled, we don't have to keep the order of existing struct. I'm not sure about what "semi-ABI stable" means, but this implies it is not completely stable. And I think struct size changes matters.

---

I am not sure C API is official, or it is backward-compatible. There is only Java, Python and C++ in the document - https://www.tensorflow.org/lite/api_docs .

There is comment in https://github.com/tensorflow/tensorflow/blob/master/tensorflow/lite/c/common.h , it implies that this is not official yet.

// TfLiteTensor wraps data associated with a graph tensor.
//
// Note that, while the TfLiteTensor struct is not currently opaque, and its
// fields can be accessed directly, these methods are still convenient for
// language bindings. In the future the tensor struct will likely be made opaque
// in the public API.

Plus, TfLiteTensor was evolved(some data definitions have been appended) for a few times. But it looks like they are likely to change it to an opaque pointer sometime later, and IMHO that is because of backward-compatibility.

[wateret]

About recompilation, say we have nnfw 1.0.0 installed in the platform and there is an app that worked with nnfw 1.0.0. And the platform updates it to 1.1.0, the app would not work unless there is a compiled binary with 1.1.0.

[glistening]

@wateret

TensorflowLite C++ API reference guides us to include "interpreter.h", which includes many headers that expose implementation details including "c/common.h". I didn't imagine they call it C++ API. For me, it looks implementation itself.

Plus, TfLiteTensor was evolved(some data definitions have been appended) for a few times. But it looks like they are likely to change it to an opaque pointer sometime later, and IMHO that is because of backward-compatibility.

Yes, exactly. As you pointed out they know it should be updated.

[glistening]

About recompilation, say we have nnfw 1.0.0 installed in the platform and there is an app that worked with nnfw 1.0.0. And the platform updates it to 1.1.0, the app would not work unless there is a compiled binary with 1.1.0.

Yes. backward compatibility is common problem in platforms. It depends on each platform's policy.
What about creating another issue if you would like to talk more about backward compatibility.

[wateret]

What about creating another issue if you would like to talk more about backward compatibility.

So I guess what we need to decide is "whether NNFW API supports binary compatibility or compilation(?) compatibility". Let's bring the discussion offline.

[glistening]

If we are going to provide binary backward compatibility, which means all binaries that worked with old onert should be guaranteed to work without recompilation, it would be better to use opaque pointers instead of exposing structure itself. Once a structure is exposed, its layout and size cannot be changed.

(ADDED) nnfw_tensorinfo seems the only exposed structure in nnfw.h. nnfw_dev.h has more.

[wateret]

Conclusion from the offline meeting

We are going to keep the ABI(binary?) compatibility. So appending a member to a struct is not allowed, for now.

[wateret]

I am going to go with "Another method 1" since "Method 2" could be ambiguous. It looks like two or more tensors could have the same name. It is going to be added to nnfw_experimental.h first.

[glistening]

@wateret

1. I've assumed that tensor names are unique. When do we have same name tensors? I consider name means id when we see in netron.

I am not sure we need to consider the models with same name for different tensors.

1. assume some input or output tensors have same name. Why do we need to get buffer from user in this case?

[wateret]

When do we have same name tensors?

I don't know, just to avoid that situation just in case.

[glistening]

@wateret Could you please answer to my 2nd question assuming we have same name?

1. assume some input or output tensors have same name. Why do we need to get buffer from user in this case?

[wateret]

Why do we need to get buffer from user in this case?

I don't understand. Do you mean why would a user want to get the index with its name? Then I don't think that would be useful too. But if it is not guaranteed that all tensors are of distinct name, I must care. I just chose that way to avoid potential ambiguity.

I can go with "Method 2" if what you assumed is always true, but I just don't want to deep dive into it. Even if Tensorflow does, we still should decide the same thing for Circle, not that I can do alone.

[glistening]

@wateret I am asking about change:

 char* /* out */ buffer, uint32_t buffer_size

It requires user to pass _user-allocated_ buffer. I don't understand why it is changed by possibility of duplicated tensor name.

[wateret]

@glistening So you mean passing an output buffer is more inconvenient than passing an input buffer and you'd like go with "Method 2"? OK then, I will. And for duplicated names, the API will return the one at the earliest index.

[glistening]

@wateret Thank your for clarifying the two changes (duplicated names and user-allocated buffer) are irrelevant. There was concern of returning pointer to internal constant at early design of API last year. Thus, I would like to get the reason of requiring _user-allocated buffer_. I prefer method 2 because it is simple and convenient in client's perspective.

[wateret]

(This is not relevant to the main topic)

There was concern of returning pointer to internal constant at early design of API last year.

If we chose first way, it would return the internal pointer. Then would it have been an issue?

Thus, I would like to get the reason of requiring user-allocated buffer.

I thought passing user-allocated buffer is for avoiding internal pointer. Did I get something wrong?

[glistening]

I thought passing user-allocated buffer is for avoiding internal pointer. Did I get something wrong?

No. I just want how you think. I am not expert of security. I am not sure how much prohibiting internal pointer is effective.

Anyway, If you would like to avoid returning internal pointer, it should be the form of char* /* out */ buffer, uint32_t buffer_size.
In addition, all API should not use returning internal pointer. nnfw.h seems not have API that returns internal pointer till now.

[wateret]

In addition, all API should not use returning internal pointer. nnfw.h seems not have API that returns internal pointer till now.

That makes sense, but I asked since that seems like contradiction with the earlier comment. https://github.com/Samsung/ONE/issues/3391#issuecomment-662765866

name as a char pointer
Who manages this pointer?

I think runtime can manage name. names could be allocated and copied during loading model. name will be deallocated when the session is closed. We may allocate a chunk instead of allocation of [number of input + number of output].

I understood this returning internal pointers.

[wateret]

Turns out that we understood things differently. Resolved with offline discussion. 😄

[glistening]

@wateret, @sj0-park If you're going to use the index to set input and output, what about the form of:

Method 4

auto status = nnfw_set_output(session_dec, "input1", NNFW_TYPE_TENSOR_FLOAT32, input1, szInput1 * sizeof(float));
if (status != NNFW_STATUS_NO_ERROR)
 // handling error

It takes const char* instead of index.

Method 2

It would be done like the following with Method 2.

uint32_t input1Idx;
auto status = nnfw_output_tensorindex(session, "input1", &input1Idx);
if (status != NNFW_STATUS_NO_ERROR)
 // handling error
status = nnfw_set_output(session, input1Idx, NNFW_TYPE_TENSOR_FLOAT32, input1, szInput1 * sizeof(float));
if (status != NNFW_STATUS_NO_ERROR)
 // handling error

[wateret]

I would prefer Method 2. For method 4, I just found NNFW_STATUS nnfw_set_input_tensorinfo(nnfw_session *session, uint32_t index, const nnfw_tensorinfo *tensor_info); also uses the input index. So with Method 4, we will need a string version too. So rather than that, Method 2 looks better to me.

[glistening]

@wateret Yes, for nnfw developer's perspective, method 2 is preferred since only 1 additional API is enough. I suggested Method 4 in the nnfw user's perspective. Both work for me.

[sj0-park]

@wateret @glistening
Thank you very much for your efforts to help me.
The API of Method 4 is actually what I wanted to do, because it can make my codes simple.
So, I prefer Method 4, but if you have any problem, Method 2 is also fine.

[wateret]

Included in nnfw_experimental.h since 1.9.0.