gromero edited a comment on pull request #8677: URL: https://github.com/apache/tvm/pull/8677#issuecomment-894525227
> @gromero thanks for your feedback. I changed the PR description with more details. > Before the timeout was manually set inside the `read_line` function and it was 10sec. Now, I changed it to 60s. > I changed the commit message as well. I see now. You're changing values 5s (for writing) and 10s (for reading) for the transport both to 70s right? (You said 60s above but I see `timeout_sec = 70` - is there a catch in the code here or it was just a typo in your comment above?). So what confused me is that in your first comment in the first commit of the patchset where you add the new parameter `timeout_sec` you actually increase the timeout value for both read and write to 100 seconds and then, on the second commit, you decrease the timeout_sec value to 70 seconds, so when both commits are squashed (pity Github squashes it before merging... but that's another story) you're right, it will amount to a increase in the timeout to 70s (for both read and write). So, if I'm following it correctly, the suggestion I have is to split better next time the patchset, like not saying a commit adds a parameter and the code adds a parameters plus does other changes like modifying other values (like the timeout_sec for read and write). That eases a lot the review process, specially for more complex changes. Your second commit message version is better but I think you could s/change/Increase/ and s/a reasonable value/70 seconds/ to inform to the readers more precisely what that commit does when applied (maybe next time, so feel free to change at will or only when you get more reviews). Anyways, just as an example, I would split them as: 1. https://github.com/gromero/tvm/commit/8a6277d9479b04a392dc3711990f0504f62bc844 - Add 'timeout_sec' parameter 2. https://github.com/gromero/tvm/commit/532fe2b81c812e5621eadbb24391d3b50a7ebaf7 - Increase test timeout to 70 sec Finally, about the code itself, just out of curiosity, I'm wondering if 70 seconds isn't too much for a device to reply in this case. In your experiments have you tried any smaller value? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
