-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Rust] Initial generator for Embedded Interface #13707
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
|
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. Generated by tvm-bot |
b41bb56 to
acfb40b
Compare
This is based on the older API in the RFC, we're landing this now so as to upstream it iteratively and allow others to see it in action 😸 There'll be a follow up, as noted in apache#13705 to update this to the fully idiomatic Rust API Co-authored-by: Ashutosh Parkhi <[email protected]>
acfb40b to
bd6c88a
Compare
Changes will be carried out in a follow up
8801abe to
b250c7b
Compare
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.
Thanks @Mousius , it would still be great to take this action(of putting the file under a clear name) now mainly to have a clear scope of intent for other developers who might be interested in other forms of rust API and avoid confusion here.
| @@ -0,0 +1,381 @@ | |||
| /* | |||
| * Licensed to the Apache Software Foundation (ASF) under one | |||
| * or more contributor license agreements. See the NOTICE file | |||
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 be great to rename to interface_embedded_rust.cc, or place it in a separate namespace.
Both approach would enable flexibility to do some more code updates in followups if necessary while not disrupting existing understanding of the code base in the meantime .
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.
As an alternative, if desirable, you are also more than welcome to bring the set of changes to a branch, in which case feel free to continue and complete the work along with followups, and we can take a look again when you feel they are ready.
|
Just to summarize my comment here. It would be great to name the interface and file so to avoid confusion, For example use the term
To help move some of the things forward, here are possible ways that might help:
Alternatively:
|
Hi @tqchen, as someone who wrote the original code and worked heavily on embedded use cases in TVM this is not a good idea, it should be fixed properly without the discrimination of language choice. Am I to assume that "would be great" means you will block all work until I change the file name and have no interest in collaborating with the authors of these files as to what would be best practice? |
|
Thanks @Mousius i outlined the goal, and possible approaches. Technically they are all feasible and actionable. By listing these suggestions, of course that is my best intent of collaboration since we also need to reconcile the needs and overall architectural decisions in the project as a whole. Looking at your reply, you probably might could use W2 (so the changes are properly fixed together as a sequeqnce plus any followup fixes that you might prefer). Please feel free to also suggest other possible actions that can address G0 |
I have put forward suggestions, you have not included them:
I was more than happy to work on something to fix the Embedded APIs, as many C engineers work in system environments which aren't embedded; this is a broken window in our overall architecture that doesn't seem to be recognised and thus there appears to be no value in such an RFC.
Yip, I was hopeful that given responses elsewhere, there would be some interest in moving this forwards with an open mindset and encouragement to follow up with a broader discussion on the proper fix rather than being blocked on a file name. And we have fair precedent for moving forwards without an RFC so thought I would test the water here.
Overall, I do not want to introduce these inconsistencies in the codebase nor do I wish to endorse the view that both C and Rust are not general purpose languages, yet I feel forced to W0 in order to progress. |
|
Might indeed is true that G0 is non-issue for the intended audience of this PR. Again G0 is not only about intended audiences of the PR, but also the general TVM project and non-embedded uses(and differentiate them from the embedded interface), and how to make our overall namespacing structure clear so we can empower both cases. G0 exist because rust is a general purpose language. If there is a suggestion to clarifying the scope of embedded API in the project for both C and rust, that could indeed be a better outcome, W2 likely offer a way to do so. Personally I also think the name of embedded c interface could be further clarified along the same line, but we can take one step at a time and given all the background mentioned, I do not take that ask as a blocker. But if there is a feeling that such sync should be done, either first make the c change in main then move on to the new style, or doing them in arbitrary order in W2 are both feasible. Taking W0 then updating embedded C would also achieve that goal as well. When W2 path is taken, there is of course not blocked on whether we have to take W0, as long as G0 is achieved. For example, moving both C and rust generator under a different namespace would be a reasonable choice that is not consistent with W0 but still achieve the goal. Usually it is indeed OK to defer some decisions of a module, provided that they are reasonabley isolated and clarified wrt to the overall scope of the project. Such requirements usually can be easily satiesfied with proper namespacing and isolation, to allow developer to own that part of submodule/subfolder, as long as their relation is satisified. Again considering the small impact of W0 to the intended users of the PR, and the clarity it offers to the wider community. I do not see it as a necessary bad. But there are also other paths available like I outlined as well for G0 as well. |
Thank you for your clarification @tqchen, I've articulated why I do not want to introduce inconsistencies by implementing W0, and you've effectively summarised that this work is blocked until either the rename you want is done or completion of all the additional rework of the existing API. I can understand why others choose to fork TVM, this workflow makes it difficult to deliver value to end users with the limited resources I have as a contributor. |
|
hi @Mousius . As a project, we do want to empower different kinds of developers and diverse sets of users. In order to do so, we need multiple modules and standard software eng practices to provide such clarity among things, especially when it comes to user-facing interfaces when multiple variants co-exist. Naming spacing or proper naming is not a hard to fulfill, and we do it every day to both gain clarity and provide space for other developers/users. With proper name-spacing, likely most of us would be OK with partial, incomplete implementation first and continued evolution of the module -- this is a very flexible requirement that most of our developers follow. As such G0 being listed is an important consideration for the project as a whole. I am not insisting on one only way to achieve that goal but to provide suggestions on what is being possible. As I said, I am also open to other thoughts on achieving G0. |
@tqchen, this has now returned to the cyclic discussion seen both here and on the RFC, your reply suggests that it is not a hard requirement, yet you are continuing to block the work proceeding with insistence on the same outcomes which push towards the one you want (see #13707 (comment) for where I've summarised this previously). |
|
Sorry about my choice of terms as that causes confusion, what i meant is that adding name spacing is not hard to fullfill, updated to clarified the point. Again I am pointing out the goal G0 is something we need to consider(and fulfill as part of changes), which I think have been quite clear through out the thread. |
I have accepted it is not hard to fulfil, it is just the wrong thing to do, please see #13707 (comment) as to why it is the wrong thing to do based on the reality of both languages (see also: Rust and C). You have been quite clear on the goal you've created for this work and how you would like it carried out, there is no further need for you to attempt to push your agenda. I will contemplate where best to continue this development. |
|
A technical decision being depends on the context. There is always a tradeoff being considered taking the project as a whole into consideration, rather than having everything outright being right or wrong. For something that sits under global namespace that context would be the broader project. Hopefully through out this thread I have explained the rationales under the context of the broader project, and existence of other API interfaces than that are not for embedded purposes as a result a need for better clarity. I have no agenda to push, and merely provide guidance about an overall principle that can enable clarity and empowerment of different users/developers, along with suggestions, that is all. |
This is not merely providing guidance, you have clearly requested changes in lines with the goal you've created for this work and how you would like it carried out, there is no further need for you to attempt to push your agenda. I will contemplate where best to continue this development. |
|
I have outlined the goals that are derived from the general principle, and provide possible suggestions of options(rather than insisting on one single approach). Feel free to take some time and bring up possible thoughts, like being stated, we also welcome other possible suggestions that aligns with G0. |
@tqchen, please see #13707 (comment) to re-visit further options which are outside of the goal you have chosen. Please also take time to read the illustration of how the options you have provided are clearly pushing towards the outcome you would want or moving development elsewhere. As shown in #13707 (comment), I did take the overall project into context, as a general machine learning compiler we do not need to stereotype specific kinds of engineers to work in specific types of environments outside of the reality of those languages. Again, this is cyclic, I'm now just referring back to the original comment outlining the issues with the approach you've given - you have made it clear what you want. |
(Builds on #13706)
This is based on the older API in the RFC, we're landing this now so as to upstream it iteratively and allow others to see it in action 😸
There'll be a follow up, as noted in #13705 to update this to the fully idiomatic Rust API
Co-authored-by: Ashutosh Parkhi [email protected]