You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tvm.apache.org by Christopher Sidebottom <no...@github.com.INVALID> on 2022/10/20 15:55:29 UTC
[apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Co-authored-by: Ashutosh Parkhi <ashutosh.parkhi@arm.com>
You can view, comment on, or merge this pull request online at:
https://github.com/apache/tvm-rfcs/pull/96
-- Commit Summary --
* Embedded Rust API RFC
-- File Changes --
A rfcs/0094-embedded-rust-interface-api.md (275)
-- Patch Links --
https://github.com/apache/tvm-rfcs/pull/96.patch
https://github.com/apache/tvm-rfcs/pull/96.diff
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96
You are receiving this because you are subscribed to this thread.
Message ID: <apache/tvm-rfcs/pull/96@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Tianqi Chen <no...@github.com.INVALID>.
Hi @Mousius I think you misunderstood my comments. Perhaps due to the possible confusion "scoping". I am not suggesting to change the interface design or re-define the way to interact with embedded C APIs.
I am only referring to the clarifying the module with proper namespace, folder structure and naming convention helps to bring the clarity to developers and users.
For example, putting some of the API under `micro` namespace would help clarifying that the interface was intended for embedded use and works with the micro embedded runtime.
That would help the users and developers to differentiate it from say use of libtvm_runtime.so which comes with different set of interface conventions.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1372644643
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Tianqi Chen <no...@github.com.INVALID>.
Thanks for the RFC. The main thing that I think worth doing is scoping, so that the relation of the proposed module can be reasonably understandable by the overall community. Here are two things
## Clarifying the interface
- X0: TVM had a heavy focus on a fixed, small set of universal interface that users interact with, namely PackedFunc, Object system and extensions being defined using that system.
- For example, when a user seek to pass in memory allocator, or Map, they simply create an Object and pass that to a PackedFunc, as a result, the overall FFI API of TVM runtime remains stable.
- X1: MicroTVM introduced a special set of interface, namely unpacked API, additionally, there are explicit specifications such as workspace, device.
X1 brings different set of complexity to our overall pipeline, but is also an understandable set of design choices for low resource embedded environment as a result we include it to empower our broader community. One consequence of X1 was a generator-based approach to generate interfaces -- it was not very obvious in the proposal but becomes obvious when reading the PR.
That particular approach is fine, but would be great to clarify "what is the interface" that the users are expecting when interacting with the project.
## Scoping
Because X1 is a different set of runtime API designed with a different environment in mind. X0 and X1 do provide different kinds of runtime user experiences(for different category of users). So it would be helpful to scope the module in a way such that they are clear and not become point of confusion for the users. For example, having namespacing and folders would help, either using the current micro branding, or embedded. This would bring clarity to the users and developers when looking at those modules.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1372526217
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
I acknowledge your concerns @tqchen, the diversity of API in TVM is a problem beyond this RFC though. This
RFC is aligned with the Embedded C APIs, which have a multitude of examples within the TVM repo. If we want to re-scope the embedded interfaces then I'd suggest we do that in a separate RFC where we can take a more holistic view?
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1372632634
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
Closed #96.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#event-8183713990
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
New APIs are now documented and I've raised https://github.com/apache/tvm/issues/13705 😸
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1372222596
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
> I also stated reasonings on why the interface_c name was a OK choice under that the context of C, because C is a language that is mostly used for embedded space -- rust do not have that same profile, as a result when we do the development we need to consider it under the new context of rust along with the need of proposed targets.
>
> Under the context of rust , it is helpful to clarify the intend because rust's most common use is not only embedded language and do not come with name spacing. Rust also come with dynamic memory management, reference counting along with other things that makes other API style possible and perhaps more primarily used under non-embedded settings.
C is used for a variety of use-cases and is foundational to much of the software used in larger systems (not only embedded), it includes dynamic memory management and reference counting - Python itself would not exist if C wasn't capable of this. The choices in the embedded C API make it better for environments where dynamic allocation isn't favored and with a subset of the C standard library, which is similar to `no_std` in Rust - they are targeted at the same subset.
> As a result a clear naming, like interface_embedded_rust would be a clear way to signal the intent. I also do not see any inconsistency applying such organization might bought.
> When looking at the consistency of the codebase. We consider the architecture in a wholistic way.
> Instead we say give a proper naming and organization considering so it have clarity under the context it is in. That is overall better for the wholistic architecture and consistency in the codebase.
Naming it `interface_embedded_rust`, is clearly inconsistent with `interface_c` which serves the same audience, as documented above we're using a subset of both languages in similar ways. I'm actively encouraging you to take a holistic approach and consider that both the C and Rust APIs are both designed as resource constrained variants of the broader languages.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1373696783
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Tianqi Chen <no...@github.com.INVALID>.
I think we have all made our points pretty clear.
From technical pov I do not think have clear namespacing or scoping would bring inconsistency, and if any, that would be more of a minor issue by looking at the embedded API. They would be significant when looking at the broader community as a whole due to potential confusion and differences in set Bx and B. e.g. what if a non-embedded rust developer look at interface_rust.cc and think how this is related to my way of interacting with TVM and relation with the existing libtvm rust API.
The suggestion was made by putting the broader TVM community into consideration along with the audiences of the proposal (embedded API users), who are of course also valuable members of the community. That is why the suggestion was made reasonably actionable so will be able continue to emable both the developers in B and (Bx-B).
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1375925390
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
@tqchen RFC rejected 😸
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1375932226
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
> To incorporate that suggestion, this can be done through say
>
> * On compilation part, have proper namespace, file or folder structure to indicate the grouping (that we are looking at embedded rust) so to avoid confusion with other rust APIs.
> * (a) introducing a new namespace; (b) use a particular folder structure that indicate it is embedded API (c) other possible approaches that helps the calrifications on the generated API
`interface_c.cc` exists and is not called `interface_embedded_c.cc` or `microtvm/c_interface.cc`, therefore we follow the existing convention to add `interface_rust.cc`.
> To be absolutely clear, I did not ask for a change of the embedded C interface.
I do not want to introduce more inconsistency to the TVM codebase by only changing one of the APIs, and we should therefore change both together to whatever is the desired state - which is not in scope of this RFC.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1373402221
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
I'd suggest your concerns might be better raised alongside https://discuss.tvm.apache.org/t/pre-rfc-api-change-formalizing-c-backend-api/10380 :smile_cat:
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1373406483
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Tianqi Chen <no...@github.com.INVALID>.
> C is used for a variety of use-cases and is foundational to much of the software used in larger systems (not only embedded)
I am not denying that fact. Assembly language can also be used to build up a broader spectrum of systems --just most people don't do that, and normal audiences of tvm don't use things in that way. Most of the developer community that interfaces with tvm through c are from the base that normally interacts with resource constraint settings. Likely a smaller portion of the rust developer commuinty likely will take that view.
> which serves the same audience
- set A: The set of audiences who would interact with the embedded C API
- set B: The set of audiences who would interact with the embedded Rust API
- set Ax: The set of audiences who would normally use C language
- set Bx: The set of audiences who would normally use Rust language
You are stating that set A and set B are the same. Which is of course not wrong. But when we design for the project, we want to not only consider the area of audiences that we are targetting, but also the audiences to the project in general.
In our case, set Ax ~ A (most people who uses Ax would also be using or prefer A), as a result there will be less confusion for audiences in Ax. In the case of rust, set Bx is bigger than B.
The suggestions are made on that basis not only considering embedded C and rust API, but also general TVM project(and broader tvm community) as a whole for this specific context of rust language in this RFC -- I am only making that statement because the proposed change is not very properly scoped and can affect audiences in Bx that are not in B.
Having proper namespacing or calrification is not a strong requirement, since it is not excluding the particular module because of existence of other rust APIs, and also helps to clearly signal the overall the intent helps to empower the community members both in A, B, Ax and Bx.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1373871671
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
Heads up, we're working on an iteration on this RFC with more idiomatic Rust 😸 though anyone willing to take a first pass would be appreciated still.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1344537878
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Andrew Reusch <no...@github.com.INVALID>.
sorry for the long delay in review, I've been on holiday for a while now. I'll take a look over this.
cc @mehrdadh @alanmacd @mkatanbaf
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1324139260
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Tianqi Chen <no...@github.com.INVALID>.
We both agree that there are other aspects such as documentation of C that can be further improved, and also agree that they should not be part of this RFC.
> strong requirement
I was referring to the fact that the cost of maintaining namespacing is not a strong requirement for this particular proposal -- having a clear conventional naming such as `interface_embedded_rust` won't cause the feature to stop functioning, nor will to likely cause the developers in B to stop using this feature, or have confusions in developing the particular feature.
It will however, come with benefit of clarity for developers who are interested in Bx but not in B. As a result the requested change, which i believe would be net positive for developer users both in (Bx - B) and B. They are pretty actionable with reasonings that listed above.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1375868703
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
> The suggestions are made on that basis not only considering embedded C and rust API(we should of course take that into consideration as part of the process like you suggested), but also general TVM project(and broader tvm community) as a whole for this specific context of rust language in this RFC -- I am only making that suggestion because the generic term rust was used to normally referring to Bx and the fact that (unlike A~Ax) Bx can be a bigger set than B.
I would suggest that we don't model TVM as the centre of the universe, the project exists to serve the needs of a far broader development community with language users that better understand their own needs. I've agreed with your sentiments that we should be namespacing and documenting this better; we should make it clear for a C developer to choose the larger dynamic or the dinkier embedded variant of the API in the same way we should make it clear for a Rust developer. This is not the right RFC for that activity.
> Having proper namespacing or calrification is not a strong requirement, since it is not excluding the particular module because of existence of other rust APIs, and also helps to clearly signal the overall the intent helps to empower the community members both in A, B, Ax and Bx.
Your review is currently requesting changes, if those changes are not a strong requirement can you explicitly approve?
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1375394453
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Leandro Nunes <no...@github.com.INVALID>.
This is here for a a few weeks already.
Before merging this, just wanted to ping @apache/tvm-committers for visibility.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1313355179
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Tianqi Chen <no...@github.com.INVALID>.
I still think you still misunderstood my comment :) The particular comment is suggesting possible approaches that would help give clarity of the rust embedded API in this RFC.
You might feel that I am implying changes to embedded C API. While it is OK to for myself or others to give suggestions in the context of reviewing this RFC. If any, they are just helpful context for discussion and future developent. They will not be used as a pre-condition to merge this RFC though.
To be absolutely clear, I did not ask for a change of the embedded C interface.
Coming back to the embedded C interface for example, most of the apis are organized under a specific folder (crt). Because C language do not come with a namespace, that was a proper choice we made to help the developer know which group of APIs they are targetting. Additionally, because C language is mostly associated with embedded space, there is less possibility of confusion.
Rust, on the other hand, is commonly being used beyond embedded settings. Rust on the other hand comes with proper namespace support. It is also possible to have [C FFI interface within a namesapce](https://stackoverflow.com/questions/38948997/is-it-possible-to-declare-extern-c-functions-without-polluting-the-namespace) and [define export name](https://doc.rust-lang.org/reference/abi.html#the-export_name-attribute) for a function with C mangling.
Additionally, rust is more commonly used as non-embedded language, and it is possible to for rust to interact with libtvm_runtime or libtvm through object system. So that creates a point of confusion for developers and users.
So what I was suggesting is the following actionable item: Propose an approach to clarify the embedded API so the user/developer clearly know the intent.
- This can be done through say (a) introducing a new namespace; (b) use a particular folder structure that indicate it is embedded API (c) other possible approaches that helps the calrification.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1372730794
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Tianqi Chen <no...@github.com.INVALID>.
That particular RFC you were referring to is about C interface and not rust.
I also stated reasonings on why the interface_c name was a OK choice under that the context of C, because C is a language that is mostly used for embedded space -- rust do not have that same profile, as a result when we do the development we need to consider it under the new context of rust along with the need of proposed targets.
Under the context of rust , it is helpful to clarify the intend because rust's most common use is not only embedded language and do not come with name spacing. Rust also come with dynamic memory management, reference counting along with other things that makes other API style possible and perhaps more primarily used under non-embedded settings.
As a result a clear naming, like interface_embedded_rust would be a clear way to signal the intent. I also do not see any inconsistency applying such organization might bought.
When looking at the consistency of the codebase. We consider the architecture in a wholistic way.
That means that we apply name-spacing, naming and folder organizations for the context that is suitable for better architectural clarity and we do our best effort in our development.
Such approach is not exclusive nor diminish the value of the work or an area. As we are not stating that because of other modules we should stop accepting the module.
Instead we say give a proper naming and organization considering so it have clarity under the context it is in. That is overall better for the wholistic architecture and consistency in the codebase.
Under the context of the proposal (which stated as embedded rust). This seems to be a quite reasonable actionable item to take.
We are also more than welcome to suggest a change of the context to say let us discuss the general rust usage, and what the majority of such interactions are should be like. Under that context the name rust without other refinement might have been proper. And we can evaluate the architectural considerations under that context.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1373465521
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
> It will however, come with benefit of clarity for broader TVM communty members who are interested in Bx but not in B, or both in (Bx-B) and B. As a result the requested change, which i believe would be net positive for developer users both in (Bx - B) and B. They are pretty actionable with reasonings that listed above.
We should make it clear for a C developer to choose the larger dynamic or the dinkier embedded variant of the API in the same way we should make it clear for a Rust developer. I am not going to introduce inconsistencies in how we treat embedded interfaces for languages that support embedded environments as part of this RFC. If this is unacceptable I will consider this RFC rejected and close it.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1375907195
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>
Re: [apache/tvm-rfcs] Embedded Rust API RFC (PR #96)
Posted by Christopher Sidebottom <no...@github.com.INVALID>.
> I am only referring to the clarifying the module with proper namespace, folder structure and naming convention helps to bring the clarity to developers and users as they start to interact with the APIs.
>
> For example, putting some of the API under `micro` namespace(actually any namespace that the community might prefer) would help clarifying that the interface was intended for embedded use and works with the micro embedded runtime.
>
> That would help the users and developers to differentiate it from say use of libtvm_runtime.so which comes with different set of interface conventions.
I'm not disagreeing with the sentiment, I'm suggesting it's out of scope within this RFC which enables the Embedded API in Rust alongside C. If we want to make changes which impact both the existing Embedded C Interface and the Embedded Rust Interface, I'd suggest we do that in a separate RFC which we can consider both rather than inflating the scope of this RFC to encompass all Embedded Interfaces.
--
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/96#issuecomment-1372661932
You are receiving this because you are subscribed to this thread.
Message ID: <ap...@github.com>