[microTVM] Fix timeout of -1 breaking Arduino transport

[guberti]

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.

cc @alanmacd @gromero @mehrdadh

[leandron]

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_/

[guberti]

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

[gromero]

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

[guberti]

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.

[gromero]

@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 chance to request, for instance, a split into another PR.

[guberti]

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