Skip to content
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

Update ICHECK error message with link to documentation page. #7869

Merged
merged 10 commits into from
Apr 22, 2021

Conversation

mdw-octoml
Copy link
Contributor

This PR updates the error message emitted by ICHECK failures to point to a documentation page, which we can update over time with more information on common failure cases and hints on how to resolve them. Welcome comments on the exact wording and the content of the documentation page.

@tqchen tqchen added the status: need update need update based on feedbacks label Apr 17, 2021
@tqchen
Copy link
Member

tqchen commented Apr 17, 2021

@mdw-octoml please fix the lint error, there is already an error handling doc here, would be great to merge the content with that one https://github.com/apache/tvm/blob/main/docs/contribute/error_handling.rst

@mdw-octoml
Copy link
Contributor Author

Fixed the lint error.

The existing error handling doc is for TVM developers, not TVM users. (It is also fairly confusing and includes a lot of details that someone simply using TVM would not need to understand.) Rather than try to have a single doc for both audiences, I think it is best to keep the user documentation separate, so it is less confusing. I added a link from the new doc to the developer doc for those who want to understand the details of how errors are generated.

Let me know what you think!

@tqchen
Copy link
Member

tqchen commented Apr 19, 2021

Makes sense. Wonders if we can have a better tile. Since it is under how to section, perhaps Handle Errors. The flaky error is known in #7870

@mdw-octoml
Copy link
Contributor Author

mdw-octoml commented Apr 19, 2021 via email

@tqchen
Copy link
Member

tqchen commented Apr 19, 2021

That sounds good

Copy link
Contributor

@jwfromm jwfromm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks Matt!

@jwfromm
Copy link
Contributor

jwfromm commented Apr 19, 2021

@tkonolige, it looks like this PR hit an error in the VM RPC test. Maybe we need to bump up the delay in the test from 2 seconds to a little longer to guarantee stability?

@tkonolige
Copy link
Contributor

Yeah, try bumping it to something larger.

@mdw-octoml
Copy link
Contributor Author

mdw-octoml commented Apr 20, 2021 via email

@mdw-octoml mdw-octoml force-pushed the mdw/update-error-message branch from 6eb9509 to 7a3effd Compare April 21, 2021 23:11
@mdw-octoml
Copy link
Contributor Author

I think I have resolved the merge issue, this should be good to land once CI passes.

@mdw-octoml
Copy link
Contributor Author

@tqchen @jroesch Looks like this is good to land

@tqchen tqchen merged commit a33eff5 into apache:main Apr 22, 2021
@tqchen
Copy link
Member

tqchen commented Apr 22, 2021

Thanks @mdw-octoml for contributing. Thanks @michalpiszczek @jwfromm for reviewing

@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels Apr 22, 2021
umangyadav pushed a commit to umangyadav/tvm that referenced this pull request May 5, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants