Skip to content

Conversation

@Mousius
Copy link
Member

@Mousius Mousius commented Jan 5, 2023

(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]

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 5, 2023

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

@Mousius Mousius mentioned this pull request Jan 5, 2023
7 tasks
@Mousius Mousius force-pushed the rust-interface-api branch from b41bb56 to acfb40b Compare January 5, 2023 14:29
tqchen
tqchen previously requested changes Jan 5, 2023
@Mousius Mousius closed this Jan 9, 2023
@Mousius Mousius reopened this Feb 9, 2023
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]>
@Mousius Mousius force-pushed the rust-interface-api branch from acfb40b to bd6c88a Compare February 9, 2023 09:23
@Mousius Mousius marked this pull request as ready for review February 9, 2023 09:23
@Mousius Mousius dismissed tqchen’s stale review February 9, 2023 09:24

Changes will be carried out in a follow up

@Mousius Mousius requested a review from areusch February 9, 2023 09:25
@Mousius Mousius force-pushed the rust-interface-api branch from 8801abe to b250c7b Compare February 9, 2023 11:11
Copy link
Member

@tqchen tqchen left a 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
Copy link
Member

@tqchen tqchen Feb 9, 2023

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 .

Copy link
Member

@tqchen tqchen Feb 9, 2023

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.

@tqchen
Copy link
Member

tqchen commented Feb 9, 2023

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 interface_embedded_rust would leave room and allow developers to not have confusion given there can be other interfaces of rust, as per discussion of rationales in this RFC apache/tvm-rfcs#96

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

To help move some of the things forward, here are possible ways that might help:

  • W0: Do the renaming directly, since I think it won't affect the functionality of the proposed feature and related users, but would also benefit other users/developers who use rust API in different ways. Feel free to bring followup changes to also align the c if necessary, although given the context of C(more in embedded i do not feel that is a requirement)

Alternatively:

  • W1: We can try to open a thread in the community to get a broad sense of what everyone's inputs are (wrt to the overall naming and scoping), which i would also be interested in hearing from.
  • W2: If we feel a sequence of changes is needed that goes beyond a single PR to bring the set of things into a ready state, we can also try to open a branch, in which case the change is also reasonably isolate, and we are more than welcome to commit the change to branch, continue followup developments, and we can re-access when we think things are at a ready state to address G0.

@Mousius
Copy link
Member Author

Mousius commented Feb 9, 2023

Hi @Mousius , it would still be great to take this action now mainly to have a clear scope of intent (for other developers), not necessarily on the particular code content itself

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?

@tqchen
Copy link
Member

tqchen commented Feb 9, 2023

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

@Mousius
Copy link
Member Author

Mousius commented Feb 9, 2023

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:

  • This is a non-issue, both Rust and C are general-purpose languages with embedded specific engineers, there is no need to discriminate.
  • If this is an issue, I have offered to fix this more broadly and fix both interface APIs which will likely result in a much broader discussion on how to make this clearer in future.

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.

For example use the term interface_embedded_rust would leave room and allow developers to not have confusion given there can be other interfaces of rust, as per discussion of rationales in this RFC apache/tvm-rfcs#96

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.

W1: We can try to open a thread in the community to get a broad sense of what everyone's inputs are (wrt to the overall naming and scoping), which i would also be interested in hearing from.
W2: If we feel a sequence of changes is needed that goes beyond a single PR to bring the set of things into a ready state, we can also try to open a branch, in which case the change is also reasonably isolate, and we are more than welcome to commit the change to branch, continue followup developments, and we can re-access when we think things are at a ready state to address G0.

  • W1 is just another place for the same cyclic discussion demonstrated both here and in the RFC
  • W2 moves the issue to the future, which is a shame for such a small change which could benefit real users - given the discussions here I don't see this ever landing in main unless I fulfil W0.

W0: Do the renaming directly, since I think it won't affect the functionality of the proposed feature and related users, but would also benefit other users/developers who use rust API in different ways. Feel free to bring followup changes to also align the c if necessary, although given the context of C(more in embedded i do not feel that is a requirement)

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.

@tqchen
Copy link
Member

tqchen commented Feb 9, 2023

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.

@Mousius
Copy link
Member Author

Mousius commented Feb 13, 2023

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.

@tqchen
Copy link
Member

tqchen commented Feb 13, 2023

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.

@Mousius
Copy link
Member Author

Mousius commented Feb 13, 2023

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 requirement, 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.

@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).

@tqchen
Copy link
Member

tqchen commented Feb 13, 2023

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.

@Mousius
Copy link
Member Author

Mousius commented Feb 13, 2023

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.

@tqchen
Copy link
Member

tqchen commented Feb 13, 2023

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.

@Mousius
Copy link
Member Author

Mousius commented Feb 13, 2023

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.

@tqchen
Copy link
Member

tqchen commented Feb 13, 2023

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.

@Mousius
Copy link
Member Author

Mousius commented Feb 13, 2023

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). Additionally, like being stated, we also welcome other possible suggestions that aligns with G0. Hopefully the rationales are pretty clear and I would encourage us to take the overall project into context here.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants