You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/07/26 20:12:09 UTC
[GitHub] [tvm] guberti opened a new pull request, #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
guberti opened a new pull request, #12189:
URL: https://github.com/apache/tvm/pull/12189
Fixes a small bug causing `test_arduino_workflow.py` to fail. Also updates a comment in `boards.json` to reflect a GitHub issue that has since been fininshed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on pull request #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
Posted by GitBox <gi...@apache.org>.
gromero commented on PR #12189:
URL: https://github.com/apache/tvm/pull/12189#issuecomment-1198633247
@leandron @guberti @mehrdadh The major issue here I see is that I have reviewed the change once, then two additional commits where added overnight (due to timezone differences) to the PR after the approvals and even one of them (https://github.com/apache/tvm/pull/12189/commits/d8472189dbca6ece5687cb299daa47bf5bb57975) touched a more generic micro code. Then no PR title or body message were updated to reflect the changes. Ideally a new PR should be created to accommodate the two additional commits in this case, with another round of reviews. Encouraging that kind of review workflow in the community is bad practice in my view and committers should guard against it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] guberti commented on pull request #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
Posted by GitBox <gi...@apache.org>.
guberti commented on PR #12189:
URL: https://github.com/apache/tvm/pull/12189#issuecomment-1199899208
In retrospect, I should have written a new message that tagged all the previous reviewers, as well as re-requesting reviews with the GitHub API for that. It would be awesome to have that rule explicitly written, but either way it is good practice. Would be happy to discuss further in a GitHub issue or on the forum.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on pull request #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
Posted by GitBox <gi...@apache.org>.
gromero commented on PR #12189:
URL: https://github.com/apache/tvm/pull/12189#issuecomment-1198670353
@guberti No worries! I think that there are still some blindspots in the review flow that needs to be discussed by the TVM community and maybe be materialized into docs to avoid confusion, and that's is an example of it in my view, so really I just would like to raise the issue here. I don't even know how you (or anybody else) would have made sure all the original reviewers would have signed off again. Pretty sure you could have posted a message tagging us, but what if it was a new contributor less familiarized with the process (not a reviewer nor a committer) that was submitting the additional changes? I think that's an issue which must be addressed on the reviewer / committer / maintainer side, after a proper discussion and consensus of course. The contributor / author can't act as an "arbitrator" here :)
That issue is also related somehow to this section https://github.com/apache/tvm/blame/main/docs/contribute/code_review.rst#L52 about given sometime for other reviewers to review a change. Obviously that change is small, not an architectural, but I wonder if we should wait at least 24h for a change not trivial (a typo fix would be a trivial change) to be merged. Of course the case here is even more special because @mehrdadh merged it and he was indeed one of the initial reviewers. That's another blindspot in the review flow, I think. I have to say that I really went for checking the PR for merging the next day just to see it already merged with the additional commits, without I having the change to request, for instance, a split into another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on pull request #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
Posted by GitBox <gi...@apache.org>.
leandron commented on PR #12189:
URL: https://github.com/apache/tvm/pull/12189#issuecomment-1197843899
Thanks for the fix @guberti, but can you clarify what is the error this is fixing and the motivation for the fix? I'm seeing some test issues when rebuilding `ci_qemu` and am interested in seeing whether this fixes the problems I'm seeing.
This is the one I'm seeing: https://ci.tlcpack.ai/job/tvm/job/ci-docker-staging/263/testReport/junit/ctypes.tests.micro.zephyr/test_zephyr/Test___test__QEMU___test_schedule_build_with_cmsis_dependency_mps2_an521_/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh merged pull request #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
Posted by GitBox <gi...@apache.org>.
mehrdadh merged PR #12189:
URL: https://github.com/apache/tvm/pull/12189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] guberti commented on pull request #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
Posted by GitBox <gi...@apache.org>.
guberti commented on PR #12189:
URL: https://github.com/apache/tvm/pull/12189#issuecomment-1198637161
I meant to have the PR re-reviewed, but I should have made sure all the original reviewers signed off again. That said, making a second PR would have been cleaner - you’re right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] guberti commented on pull request #12189: [microTVM] Fix timeout of -1 breaking Arduino transport
Posted by GitBox <gi...@apache.org>.
guberti commented on PR #12189:
URL: https://github.com/apache/tvm/pull/12189#issuecomment-1198617204
@leandron you're right, I should be more explicit about what this fixes. This PR only fixes to the workflow test for Arduino and the autotuning test for both platforms. My fix is unrelated to the error you’re seeing, unfortunately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org