-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix wrong terminology in tvmc source
#10320
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
|
cc: @areusch |
|
cc: @jwfromm |
gromero
left a comment
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.
@cconvey I think that's correct. For instance, we can now pass a Runtime() object and a Executor() object (that can be of aot or graph type) to relay.build, so runtime and executor are different concepts, although quite dependent when "running/executing" the runtime.
Please add the tag [TVMC] in front of the commit message title and write a commit message quickly explaining the change. Having an empty commit body messages is not a good practice in any case.
As default for the TVM project, when merged to the tree, a patchset gets the commit title from the PR title and a single patch like this one gets its title from the single patch associated to the PR, so in this case you need to push the commit again.
Thank you.
|
cc @leandron |
|
Thanks @gromero ! Would you mind clarifying part of your comment?
When you write My impression was that TVM doesn't actually use common base classes, etc. to ensure a common interface across the different executor implementations. |
@cconvey I had the https://github.com/apache/tvm/blob/main/python/tvm/relay/backend/executor.py#L27 and used, for instance, like in: https://github.com/apache/tvm/blob/main/tests/micro/zephyr/test_zephyr_aot.py#L69-L72 But more broadly I was just saying that if I'm not mistaken in the past the concepts of runtime and executor were not as clear as they are now, back when there wasn't the even the AOT executor and before the discussions/changes around: ... and I think your patch is fixing what looks a reminiscent code of that period. |
d381040 to
b4899c6
Compare
|
Thanks @gromero for all the background information, I find it very helpful! AFAIK I've addressed your feedback. |
leandron
left a comment
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.
LGTM
|
@leandron It seems the CI failed due to an unrelated issue (something related to Ethos-U55 AOT tests, but I can't follow exactly what) Have you ever seen this error before? Is it known to be a flaky test? hmm just realized that @areusch seems to wresting with some ethos-u55 test in here too: #10283 (comment) @cconvey The CI failed but I can't see how it can be related to your change, hence while we try to understand it better could you please re-trigger the CI? (you can, for example, just do |
areusch
left a comment
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 @gromero !
jwfromm
left a comment
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.
Makes sense to me, thanks @cconvey.
Renames variable from `runtime` with `executor` to better reflect current terminology and reduce confusion.
|
@tqchen Ready for merge |
Renames variable from `runtime` with `executor` to better reflect current terminology and reduce confusion.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.