-
Notifications
You must be signed in to change notification settings - Fork 78
Embedded Rust API RFC #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Ashutosh Parkhi <ashutosh.parkhi@arm.com>
|
|
||
| This largely aims to follow the APIs defined for `--interface-api=c` (see: [Embedded C APIs](https://discuss.tvm.apache.org/t/rfc-utvm-embedded-c-runtime-interface/9951)), but wraps each call to allow `safe` user facing Rust code. | ||
|
|
||
| It builds upon the [Model Library Format archive](https://discuss.tvm.apache.org/t/rfc-tvm-model-library-format/9121) to provide a complete package for Rust appication developers with all relevant files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: appication -> application
|
This is here for a a few weeks already. Before merging this, just wanted to ping @apache/tvm-committers for visibility. |
|
|
||
| This introduces a second embedded API alongside the C API, but fundamentally we want users to be able to create applications in the language that best suits their needs. | ||
|
|
||
| Whilst it is an innovation project, this will be supported with best efforts but may be lag in features behind the C interface API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to block this RFC but just curious, who will be on the hook to maintain and ensure that the embedded rust API is properly tested? Right now for example the Rust TVM bindings aren't really owned by anyone and the Go bindings aren't even run in CI and are potentially bit rotting away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add a section to the RFC that outlines the testing and regression prevention strategy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distinction here is features not robustness, as this lands in TVM I would expect it to be both initially tested and regression tested in line with https://github.com/apache/tvm/blob/main/docs/contribute/code_review.rst#id11 ?
|
sorry for the long delay in review, I've been on holiday for a while now. I'll take a look over this. |
|
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. |
Change-Id: Ib68fe5d8a25502d02dbefd266ea89f6b6869bfd1
|
New APIs are now documented and I've raised apache/tvm#13705 😸 |
|
|
||
| This introduces a second embedded API alongside the C API, but fundamentally we want users to be able to create applications in the language that best suits their needs. | ||
|
|
||
| Whilst it is an innovation project, this will be supported with best efforts but may be lag in features behind the C interface API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we clarify the scope of the support (e.g. what models can be supported and what are the restrictions)?
Given embedded part certainly contains more limitations and as a result do not support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we do this currently for the C Embedded API and if we want to, we should define it as part of the over-arching microTVM project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is mainly about scope of support of the current proposal just to bring some clarity, that is all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can provide that at the interface level, it is the same as the Embedded C interface which relies on the various functions of the AOT executor and C runtime to function - it's not specific to the Embedded Rust Interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular comment do not have things to do with interface, but instead the models that can be supported and possible restrictions.
In this case stating that "this particular module will support all the models that can be supported through embedded C API, because there is a gap in lacking behind C interface API A, B, C , we might miss models X, Y, Z" would be sufficient.
|
|
||
| ```rust | ||
| impl TVMDevice for MyDriver { ... } | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thakns for the RFC, after reading the proposed code, I think the main thing that would be worth clarifying is the overall scope, and scope limitations in relation with existing namespace. I will put my input in a followup comment section.
|
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
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 under the consideration of the specific scenario, but would be great to clarify "what is the interface" that the users are expecting when interacting with the project. For example, what are set of stable runtime interfaces(like those in allocator and device API) that users can expect and interact with the generated code, and for other cases, what are generated interfaces that can change. ScopingBecause 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. On rust in particular, the user can also have a differentiated understanding of a normal runtime that features the extended feature sets and object, and a embedded runtime that comes with other things. A typical approach to scoping is to clarify the scope in the RFC, and have proper namespace, folder structure and naming convention helps to bring the clarity, (For example, putting things under micro namespace and subfolder). This would bring clarity to the users and developers when looking at those modules. |
|
I acknowledge your concerns @tqchen, the diversity of API in TVM is a problem beyond this RFC though. This |
|
Hi @Mousius , thanks for your followup. 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, nor I am suggesting that have multiple APIs(like X0 and X1) is a problem since there are clear needs on different scenarios and clear community members who are maintaining them. 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 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. |
|
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 also comes with proper namespace support. It is also possible to have C FFI interface within a namesapce and define export name 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 -- which is a different approach from the proposed one. So the only thing I asked for was a proper way to group and clarify the API. I was suggesting the following actionable item for this particular RFC: Propose an approach to clarify the following things so users/developers clearly know the intent:
To incorporate that suggestion, this can be done through say
|
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. |
|
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 😸 |
|
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. |
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
Naming it |
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 do 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.
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. When we are using the term "C" or "rust", we are generally referring to set Ax and set Bx. In our case(users and developers of TVM), 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 can be bigger than B. 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. 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. |
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.
Your review is currently requesting changes, if those changes are not a strong requirement can you explicitly approve? |
|
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.
I was referring to the fact that the cost of maintaining namespacing is not a strong requirement for this particular proposal in terms of making such a change -- having a clear conventional naming such as 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. |
|
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, instead, they would bring more clarity in the case where namespacing was supported. The choice, however, would 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 both 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). |
|
@tqchen RFC rejected 😸 |
Co-authored-by: Ashutosh Parkhi ashutosh.parkhi@arm.com