Sanitize names of input tensors in interface header #8720

grant-arm · 2021-08-11T11:04:25Z

This PR adds code to sanitize the names of input tensors (by replacing special characters with underscores) before using them in the header file generated by the generate_c_interface_header() function. In particular, this is necessary because using the Tensorflow API to create models can result in input names containing colons e.g. serving_default_x:0_int8 . Using these tensor names directly as variable names, results in invalid C code.

@Mousius @manupa-arm @areusch

leandron

A small nit, otherwise LGTM

python/tvm/micro/interface_api.py

manupak

@grant-arm @Mousius,

Is there a reason for interface_api to be in Python?
If not, shall we move the interface_api.py to C++ ?

I feel sanitizing and keeping alignment on names at two places to be a bit fragile.
We could use a same utility for sanitizing then.

Mousius · 2021-08-18T08:13:11Z

Hi @manupa-arm,

The answer is that writing it in Python was easier for the initial version of the interface API. I agree eventually it'd be good to port it to C++ but believe it to be out of scope of this PR, it appears to be easily tested so we can guarantee it works despite it being implemented twice.

manupak · 2021-08-18T12:43:10Z

Ack, agree that it being out of scope.

If we agree with the direction, any thoughts on maintaining a sanitize utility only in C++ exposed to python?

Mousius · 2021-08-18T17:15:08Z

I'd suggest we implement the test cases and the desired behaviour here and refactor it altogether in a later PR? It'll be a bit wonky either way and we can do a single refactor to C++ rather than a hybrid using the function registry now.

manupak · 2021-08-19T08:45:12Z

Cool! make sense if we are following up with that.

For better transparency, I'd suggest we place a TODO for that and get this in.

Mousius

I've raised #8792 to track the consolidation rather than adding a TODO. Otherwise LGTM!

manupak

LGTM.

cc : @areusch @jroesch @mbs-octoml

leandron

LGTM - lets leave it here a bit more, trying to get others to have a look. Will merge it tomorrow morning in case nobody disagrees.

mbs-octoml · 2021-08-19T15:44:40Z

LGTM. Agree the cc/py split is unfortunate and a comment cross-linking them with the TODO would be nice. Is the plan to move all code generation to the cc side at some point?

grant-arm · 2021-08-20T16:28:22Z

@grant-arm @manupa-arm @Mousius sorry for the late review here. i do think we need to consider the case that multiple distinct punctuation are mangled into identical variable names. could you propose a solution for this?

Thanks for pointing that out @areusch . I've pushed an update now that raises an exception in the case that 2 sanitized names clash. I've also added a test case for that.

tests/python/relay/aot/test_crt_aot.py

Change-Id: I7f02a993887bf84316262cd2586a734a9079c338

Change-Id: I157d8d8d607de2904285e403893f146e97b510d5

Change-Id: I9082ae32079a1a3924c06c7f26c757aafa46dec2

grant-arm · 2021-09-01T20:11:22Z

Hi @areusch , would you mind taking a look at this again and see whether you're happy with the change I made to address the case where multiple tensor names with punctuation are identical after sanitizing. If this happens now, an exception is raised during code generation.

manupak · 2021-09-03T13:38:10Z

@areusch can you take another look ?

areusch

thanks @grant-arm this looks good now

To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31).

* Sanitize names of input tensors in interface header Change-Id: I7f02a993887bf84316262cd2586a734a9079c338 * Update tensor name sanitizer tests to parameterize them. Change-Id: I157d8d8d607de2904285e403893f146e97b510d5 * Only test unpacked, C interface API, AOT case Change-Id: I9082ae32079a1a3924c06c7f26c757aafa46dec2

* Introduce centralised name transformation functions To address some of the concerns raised in #8280 and #8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31). * Improve error handling and error messages * Sanitize sanitise to sanitize This patch aims to sanitize uses of sanitise to the form of sanitize to be consistent with the overall codebases use of American English.

* Introduce centralised name transformation functions To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31). * Improve error handling and error messages * Sanitize sanitise to sanitize This patch aims to sanitize uses of sanitise to the form of sanitize to be consistent with the overall codebases use of American English.

* Sanitize names of input tensors in interface header Change-Id: I7f02a993887bf84316262cd2586a734a9079c338 * Update tensor name sanitizer tests to parameterize them. Change-Id: I157d8d8d607de2904285e403893f146e97b510d5 * Only test unpacked, C interface API, AOT case Change-Id: I9082ae32079a1a3924c06c7f26c757aafa46dec2

* Introduce centralised name transformation functions To address some of the concerns raised in apache#8280 and apache#8720 I've put together a series of functions to combine together names to re-use between these areas. These are meant to be a starting point to fix up the name generation to use the TVM C conventions and port the interface API header to C++. These functions will also be used for constructing the names in the C Device API (apache/tvm-rfcs#31). * Improve error handling and error messages * Sanitize sanitise to sanitize This patch aims to sanitize uses of sanitise to the form of sanitize to be consistent with the overall codebases use of American English.

grant-arm requested review from areusch, comaniac, jroesch, junrushao, kparzysz-quic, liangfu, masahi, merrymercy, tmoreau89, tqchen, vinx13, yzhliu and ZihengJiang as code owners August 11, 2021 11:04

grant-arm force-pushed the sanitize_input_tensor_names branch 5 times, most recently from ff94e42 to 82f24d3 Compare August 15, 2021 11:20

leandron reviewed Aug 16, 2021

View reviewed changes

python/tvm/micro/interface_api.py Outdated Show resolved Hide resolved

manupak reviewed Aug 17, 2021

View reviewed changes

grant-arm force-pushed the sanitize_input_tensor_names branch from 82f24d3 to b8384f9 Compare August 18, 2021 18:42

Mousius mentioned this pull request Aug 19, 2021

C Interface API Generation split between Python and C++ #8792

Closed

Mousius approved these changes Aug 19, 2021

View reviewed changes

manupak approved these changes Aug 19, 2021

View reviewed changes

leandron approved these changes Aug 19, 2021

View reviewed changes

grant-arm force-pushed the sanitize_input_tensor_names branch 3 times, most recently from b4afd3c to 3b0e7bf Compare August 20, 2021 13:34

grant-arm requested a review from areusch August 20, 2021 17:29

leandron approved these changes Aug 25, 2021

View reviewed changes

grant-arm force-pushed the sanitize_input_tensor_names branch 2 times, most recently from 5c44fcb to fddc3df Compare August 25, 2021 15:39

Mousius requested changes Aug 26, 2021

View reviewed changes

tests/python/relay/aot/test_crt_aot.py Outdated Show resolved Hide resolved

grant-arm force-pushed the sanitize_input_tensor_names branch from fddc3df to 0c18758 Compare August 31, 2021 16:00

grant-arm added 2 commits September 1, 2021 12:01

Sanitize names of input tensors in interface header

efb46ee

Change-Id: I7f02a993887bf84316262cd2586a734a9079c338

Update tensor name sanitizer tests to parameterize them.

0f3ad93

Change-Id: I157d8d8d607de2904285e403893f146e97b510d5

grant-arm force-pushed the sanitize_input_tensor_names branch from 0c18758 to 8c28118 Compare September 1, 2021 11:01

Only test unpacked, C interface API, AOT case

1e07a0e

Change-Id: I9082ae32079a1a3924c06c7f26c757aafa46dec2

grant-arm force-pushed the sanitize_input_tensor_names branch from 8c28118 to 1e07a0e Compare September 1, 2021 13:57

areusch approved these changes Sep 3, 2021

View reviewed changes

areusch merged commit 06a0d63 into apache:main Sep 3, 2021

Mousius mentioned this pull request Sep 23, 2021

Introduce centralised name transformation functions #9088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanitize names of input tensors in interface header #8720

Sanitize names of input tensors in interface header #8720

grant-arm commented Aug 11, 2021 •

edited

Loading

leandron left a comment

manupak left a comment •

edited

Loading

Mousius commented Aug 18, 2021

manupak commented Aug 18, 2021

Mousius commented Aug 18, 2021

manupak commented Aug 19, 2021

Mousius left a comment

manupak left a comment

leandron left a comment

mbs-octoml commented Aug 19, 2021

grant-arm commented Aug 20, 2021

grant-arm commented Sep 1, 2021

manupak commented Sep 3, 2021

areusch left a comment

Sanitize names of input tensors in interface header #8720

Sanitize names of input tensors in interface header #8720

Conversation

grant-arm commented Aug 11, 2021 • edited Loading

leandron left a comment

Choose a reason for hiding this comment

manupak left a comment • edited Loading

Choose a reason for hiding this comment

Mousius commented Aug 18, 2021

manupak commented Aug 18, 2021

Mousius commented Aug 18, 2021

manupak commented Aug 19, 2021

Mousius left a comment

Choose a reason for hiding this comment

manupak left a comment

Choose a reason for hiding this comment

leandron left a comment

Choose a reason for hiding this comment

mbs-octoml commented Aug 19, 2021

grant-arm commented Aug 20, 2021

grant-arm commented Sep 1, 2021

manupak commented Sep 3, 2021

areusch left a comment

Choose a reason for hiding this comment

grant-arm commented Aug 11, 2021 •

edited

Loading

manupak left a comment •

edited

Loading