[Rust] End-to-End testing for AOT Interface API

[Mousius]

(This includes #13707, which is being blocked)

• Adds functionality to generate Rust files within Model Library Format
• Adds tests for Rust AOT with USMP
• Updates AOT test utils to optionally generate Rust

We need to eventually clean up the bunch of if/else in the AOT test utils - this would be a good candidate for an RFC to manage AOT interfaces once this has landed.

Co-authored-by: Ashutosh Parkhi ashutosh.parkhi@arm.com

[tvm-bot]

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

• cc @ehsanmok _{See #10317 for details}

_{Generated by tvm-bot}

[tqchen]

Please come up with a proposal for resolving the goal of scoping different interface conventions. Specifically, given that there are multiple rust interfaces available in the TVM codebase, being able to do so would help to give space for general community.

[tqchen]

One possible minimum approach would be renaming the file to interface_embedded_rust.cc, or move to a subfolder that signals such intent, feel free to rename interface_c.cc together if there is an urge to keep things consistent

[tqchen]

Thank you for the contribution. As noted in the other thread and the RFC.

Please resolve the goal of scoping different interface conventions. Specifically, given that there are multiple rust interfaces available in the TVM codebase, being able to do so would help to give space for general community.

• G0: Avoid confusion with the normal rust API usage given there are multiple possibilities under a different API.

One possible minimum approach would be renaming the file to interface_embedded_rust.cc( or nostd_rust, or some other terms that can help to clarify the scope of the API), or move to a subfolder that signals such intent, feel free to rename interface_c.cc together if there is an urge to keep things consistent

[Mousius]

Thank you for the contribution. As noted in the other thread and the RFC.

Please resolve the goal of scoping different interface conventions. Specifically, given that there are multiple rust interfaces available in the TVM codebase, being able to do so would help to give space for general community.

• G0: Avoid confusion with the normal rust API usage given there are multiple possibilities under a different API.

One possible minimum approach would be renaming the file to interface_embedded_rust.cc, or move to a subfolder that signals such intent, feel free to rename interface_c.cc together if there is an urge to keep things consistent

Hi @tqchen,

I clearly stated that this is pending the PR you are blocking, please re-visit #13707 (comment) as to why the goal you've outlined is inappropriate and why the renaming is the wrong action to take. As evidenced by this PR, there are more things to resolve around AOT interfaces APIs than simply renaming one file.

I've also already stated I would be happy to do the follow up work to properly resolve this situation, you've articulated in many threads that you'd prefer a more agile culture yet it's clear that this isn't being applied in this case.

[tqchen]

Agility is realized based on proper scoping of the changes (which is a very common pre-condition in software developments to scale).

For example, the changes are reasonably scoped under a subfolder, sub-namespace, or branch that do not breaches into other things, then we would definitely love to enable such agility, knowing that changes won't create confusions in cases like different APIs(the generated embedded interface and the existing tvm rust api) that comes with the same terminology(rust interface).

Almost all other development practices in tvm follow such practice (meta_schedule to disambiguous from autotvm/auto-schedule, uma, tensorir, web, to name a few) and are cleanly organized in their respective namespace subfolder structures in the project.

That is why I made such suggestions from the beginning, so such followup developments can be proceeded without breaching into other interfaces. And give agility to such developments.

Happy to review what comes along as well once you think they are ready.

[Mousius]

Agility is realized based on proper scoping of the changes.

For example, the changes are reasonably scoped under a subfolder, sub-namespace, or branch that do not breaches into other things, then we would definitely love to enable such agility, knowing that changes won't create confusions in cases like multiple namespace.

That is why I made such suggestions from the beginning, so such followup developments can be proceeded without breaching into other interfaces.

Please refer back to my previous comment apache/tvm-rfcs#96 (comment), this is aligned with the existing architecture, demonstrating that this is reasonably scoped for the files that exist pending further changes to better remedy the situation. Please also refer back to that conversation as to why we shouldn't be introducing the inconsistency for one file.

Happy to review what comes along as well once you think they are ready.

I'm indeed intending to raise additional pull requests to finish upstreaming this work, you are welcome to request changes on them all - as unwelcoming as this has become, I want to surface the user value even if it remains in a fork. As I've illustrated, this work is ready for review, trusting in myself as a committer to follow up and fix the area of the codebase I have already contributed much to.

[tqchen]

In this case I would come one step further and say the particular existing item (if you were referring to target/interface_c) was also not properly scoped and would benefit from scoping. It was a small part of existing architecture, that was not properly scoped under the context of the bigger global architecture of the project. I did not review that particular change, nor did the scoping proposal included the RFC in the relevant changes.

I (lazily) approved the RFC(including previous ones related to such embedded interface) based on the assumption that there would be proper scoping, as it were a common practices in the TVM project(and any OSS project) for most of the modules. Apparently there wasn't such a consideration, and when an ask for scoping was raised the argument was being pinned on consistency one particular item of the codebase (and referring to that as the architecture), rather than the project as a whole.

That part of merged code however, can also be revisited, and in the course of continuous conversation -- rather than being used as a simple golden rule (or simply being use the single point to refer to the existing architecture). Remember it is only part of the code(that was not consistent with the rest of the existing architecture), and being consistent only with that one part of the code simply ignores the architecture as a whole.

Doing proper scoping would be necessarily for different contributors to operate in their subareas without having to step onto each other.

While the perhaps the code implements the feature that was intended in the area that you contributed to, the current scoping of the module breaches other concurrent APIs. As a result that impacts grows to the overall architecture level of the project. From the standpt of overall project architecture, I would kindly request a proper scoping for the proposed module only.

Please feel free to continue development in a way that is reasonably scoped, it can be a branch, a new namespace, sub folder or any other approach that would satisfied that goal. For the module to land together with other modules, it would need to be properly scoped, and that is independent from the quality of implementation, the feature itself, or user values. As we need multiple modules to co-exist together with clarity and follow reasonable scoping practices that we all follow.

[Mousius]

That part of merged code however, can also be revisited, and in the course of continuous conversation -- rather than being used as a simple golden rule. Remember it is only part of the code(that was not consistent with the rest of the architecture), and being consistent only with that one part of the code simply ignores the architecture as a whole.

I'm simply demonstrating that the code already exists, and it would be architecturally consistent rather than introducing another inconsistency. I never said it was a golden rule, nor did I state it was in a good state, in fact I suggested we should do a larger piece of work to remedy it properly.

Please feel free to continue development in a way that is reasonably scoped, it can be a branch, a new namespace, sub folder or any other approach that would satisfied that goal, which was the course of action that most developers in the community would happily take.

It's better to make any new interface as consistent with the old interface as possible, as I've highlighted in previous conversations, this is standard practice when interacting with existing code. I've also stated I would follow up to remedy the overall architecture, taking into account more than a single file, you've made it clear you do not trust me to do such a follow up.

[tqchen]

In this case, what was being refered to was an item x("old interface") in the overall codebase, which unfortunately was not scoped properly under the context of overall architecture A (including "other older interfaces"). As a result, x is not consistent with A in general.

When proposing a new item y here, the proposed interface y may be more consistent with x, but it is again not consistent with A, which is bigger part of the overall architecture globally. Nor was the change clearly scoped to have clarity in A.

Obviously there is a tradeoff here.

• We can make y consistent with A and bring proper scoping, that might come at cost of slightly inconsistency with x.
• Making y in a specific way that might be strongly consistent with x ignores the fact that there is a need of scoping and consistency with A in general.

The ask is that considering the overall architecture, please start with a way that is consistent with A (where all other modules are properly scoped), it would indeed introduce inconsistency with x, but from overall project pov that is better than introducing y that is again inconsistent with A.

From the overall project's pov this tradeoff is the right step forward, and what one would normally recommend to do so. The course of actions are also reasonably easily actionable.

[Mousius]

In this case, what was being refered to was an item x("old interface") in the overall codebase, which unfortunately was not scoped properly under the context of overall architecture A (including "other older interfaces"). As a result, x is not consistent with A in general.

When proposing a new item y here, the proposed interface y may be more consistent with x, but it is again not consistent with A, which is bigger part of the overall architecture globally.

Again, this is cyclic, I understand that we should aim for a wider consistency, you've made it clear you don't trust me to carry out such changes as a follow up and instead want the singular change done at the cost of further inconsistency. I've referred back to the original comment outlining the issues with the approach you've given - you have made it clear what you want to happen.

[tqchen]

Just to be clear, either actions being taken there will cause some levels of further inconsistency of some form in the project. Either in the form of

• F0: y being in consistent to A, but at slight inconsistency with x.
• F1: y being more consistent with x, but a the cost of introducing further inconsistency with A and another less scoped item.

I am merely stating that when considering the overall project, we should prioritize F0(while keeping both F0 and F1 in mind)

[Mousius]

Just to be clear, either actions being taken there will be some levels of further inconsistency of some form in the code base. Either in the form of

• F0: y being in consistent to A (adding inconsistency to the overall architecture) and another less scoped item.
• F1: y being slightly inconsistent with x (which was inconsistent with A).

I am merely stating that when considering the overall project, we should prioritize F0.

I would suggest that being welcoming of this change and encouraging the follow up would prioritise the health of the overall codebase, as you've suggested in other threads.

[tqchen]

I am not sure which other thread were referred, but I believe most of the other threads have changes that are properly scoped in their respective modules as a result such agility can be enabled.

I certainly try to accommodate the set of changes that was why the suggestion was minimum while still prioritizes F0. And with proper scoping agility can also be enabled for this set of changes

[leandron]

I've been following all these discussions about the Rust API proposed improvements and I'm surprised by how this is being dealt with.

The disagreement seems to be on the way files are named, and the blocking request is to make it explicit the proposed changes only applies to embedded devices and (subject to confirmation) not doing that would have a detrimental effect in causing confusion to developers.

My Rust knowledge is quite basic, so I'd like to understand how the proposed change is only applicable for embedded use cases, that it is being required to be explicitly named this way. Names of things are very subjective, unless we have some written naming convention written somewhere, which I'm not aware of.

I guess the important question is: @Mousius is there something proposed here that fundamentally only applies to embedded Rust? If not, then I think the proposed patch is fine to be approved and merged, and we should proceed this way.

As we've been discussing a lot recently in the threads (e.g. Unity) and in the forum (e.g. Code Review Guidelines thread), the argumentation line of trusting, empowering the community and welcoming ideas seems to be conditionally applicable to some proposals and not others. This is one case it seems we are not applying those, therefore, letting contributors down and making them less inclined to make future contributions.

So I suggest in the current direction of being welcoming and empowering the community to take decisions, that we accept and iterate over the proposed API, and in case we notice it is causing confusion to developers, we re-evaluate.

[tqchen]

Thanks, everyone, for chiming in so far. We have spent quite a bit of time over this. I think we are getting closer to more clarity so I would like to provide a digest of my verdict so far.

Importance of Scoping

Here scoping refers to clearly naming and differentiating the overall modules with other means. For example, when there is already a module (tvm rust api) that interfaces with the users. It would be appropriate to find ways to structure it clearly in the codebase so that it differentiates with existing modules.

• This does not mean that the proposed new module should not overlap with the features provided by existing modules.
• It does not limit the scope of what the API can do but asks for clarification of API and implementation such that they signal the difference to the user. It only means that the proposed API and related code sit in a way that signals the difference and structure (wrt to other modules).
• Scoping does not proscribe a specific the way the API is named, as long as the reasonable structure and name-spacing are applied, we can have great flexibility.

For example, naming it nostd_rust_interface would also be accurate, not restricting the setting to embedded but also signaling that it is a different module from the current rust API. Naming it tvm_awesome_xyz_rust.cc also works as long as awesome_xyz conveys the intent of the code.

There are many approaches to scoping, including, but not limited to:

• naming of the respective files
• subfolder structures
• namespace

These practices are also common practices in software development.

Scoping is a Necessary Precondition for Agility

We are a project that comes with a lot of different modules and developer communities. Each one of us may care about a different subarea. In order for the modules to be sufficiently developed concurrently. Clarity of scoping (again, not in terms of what they can do, but simply in terms of clear organization, naming, and namespacing along with existing architecture) is a necessary condition for us to empower each other so as to avoid one module stepping into another one.

Usually, proper scoping needs to be done once – e.g. placing things in a proper subfolder(and/or) namespace would allow agile development of any APIs under that namespace without much intervention from the broader community. This is because the scope of impact is also isolated in such a way.

After proper scoping, there will be great flexibility. Naming the API as a namespace_x::awesome_xyz is fine as long as the community members of respective areas agree. One would have great flexibility as long as the namespace differentiates it from existing namespace, and additional folder structure that helps to signal clear organizations.

That does not mean we cannot bring changes to the root namespace, they do mean that would certainly need more deliberations in consideration along with existing APIs and an evaluation from an overall architecture standpoint.

Accepting changes without scoping is problematic. As the scope of impact is not reasonably isolated as we run into issues of modules stepping into each other concurrent development. In such a particular case we might make one developer happier, but at the cost of the broader community. Most of our developer community members are open minded to do such a proper scoping consideration, and as a result making the development much more smooth not only in favor of one but for everyone. In short, proper scoping is a pre-condition for empowerment and agility.

Scoping is the Standard Practice in Almost All Modules in TVM

Scoping is a standard practice, and necessary condition for software to scale and is the standard practice in the TVM project. We can easily name a few: meta_schedule (so name differentiates from autotvm), uma, crt.

This is a standard practice that most of us follow in the project. They do not diminish the value, or what the module can do, they just help to bring a clear structure. I worked on these properly scoped modules daily. When reviewing and accepting RFCs, we always keep assuming that the code will be properly scoped, or feedback being taken to resolve that goal.

Verdict of This Particular Case

In this particular case, the items of interest includes three things
A: the overall architecture, which already includes a rust API that interacts with the standard tvm runtime.

y: proposed interface_rust, which has an scoping issue mainly to have possible confusions with the existing rust API.
x: interface_c , the “old architecture” that was referred to in this case, which unfortunately was not properly scoped, as it can cause confusion with the c_runtime_api. Since interface_c is not compatible with all variants of runtimes and is reasonably strongly coupled with crt(for a good reason). Naming it as something like interface_c_for_crt, interface_c_tiny would have been better.

I have not reviewed x’s code change unfortunately, and the respective RFCs did not discuss scoping. Still the priority in terms of architecture should be prioritizing y to be consistent with A, even if it may come at the cost of slight inconsistency with x.
The suggestion has nothing to do with whether or not to restrict the usage to embedded only. But instead to clearly signal the difference in the space. For example, nostd_rust would be another accurate description that also works, as it does not depend on the larger standard runtime (including object system) that TVM provides (for a good reason). Any other proper namespacing or folder structure also works.
There is also no inconsistency with the recent rationales for agility and empowerment. As all of the other threads are pre-conditioned on proper scoping of such related modules.
It is also not an issue that will arise in the future, as the rust API already exists in the codebase now. Putting the changes in a reasonable subfolder, namespace, or at least proper naming would enable us to iterate the proposed API in an flexible way. In this way we can empower not only this PR, and other community developers (who also follow the same scoping principle).

Because the proposal has a need for scoping, it becomes a case of overall TVM architecture. I have volunteered my energy over years to help coordinate the overall architecture and hopefully the overall input gives more clarity here. I also explained the rationale. Hopefully the recommendations are clear and they are made on the basis of the state of scoping and the overall architecture.

If there is a goal to continue code review and iterate without addressing the scoping, then a branch could be a good starting point. Other ways to achieve the same goal also helps here.

Again I am not being fixated on only one way to name the file, but instead offering suggestions to help the scoping of the module. If I come and do it personally I would certainly do more steps than just a file naming. Here I am offering a minimum step to empower the development while still have reasonable scoping, before we continue come and improve them. I have no doubt that the module could be useful to intended users and developers, just that for the module to co-exist with other modules in the project some minimum requirement of scoping is needed to clear it from overall architecture pov.

I would be more than happy to review future proposals. To give extra clarity, I marked the PR as need-scoping (as it cannot be merged as it is due to the need). Please copy me on future proposals related to the module as I would like to help on the scoping part.