-
Notifications
You must be signed in to change notification settings - Fork 78
display verbose message when build failed. #5600
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
| try: | ||
| subprocess.run(cc_cmd, check=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) | ||
| except subprocess.CalledProcessError as e: | ||
| output = e.stdout.decode(*SUBPROCESS_DECODE_ARGS) | ||
| raise RuntimeError(output) |
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.
Hi @xuhancn. Thanks for your contribution!
Let's try simplifying the code. Does this work for you?
| try: | |
| subprocess.run(cc_cmd, check=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) | |
| except subprocess.CalledProcessError as e: | |
| output = e.stdout.decode(*SUBPROCESS_DECODE_ARGS) | |
| raise RuntimeError(output) | |
| subprocess.check_output(cc_cmd, stderr=subprocess.STDOUT, encoding=locale.getpreferredencoding()) |
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.
Hi @anmyachev I think my code is better, we'd better keep try-except block. It is working well in PyTorch for a long time.
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.
We have one more criterion: how much the code deviates from the upstream. I use this as a guide when trying to modify your code. The smaller it is, the easier it is to support for us and potentially upstream it.
Several things from docs: https://docs.python.org/3.10/library/subprocess.html#subprocess.check_output
- This is equivalent to:
run(..., check=True, stdout=PIPE).stdout - If the return code was non-zero it raises a CalledProcessError. The CalledProcessError object will have the return code in the returncode attribute and any output in the output attribute.
The main reason why you use try-except is to decode the output (at least I see it like this). However, if immediately pass encoding to the call command, then this is not necessary.
|
Closes #3760 |
anmyachev
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.
If I haven't convinced you, you can merge the changes as is.
Issue:
When triton build failed, it missed verbose error message display.
Fixing:
It referenced to
PyTorchcpp_builder code, to show verbose error message: https://github.com/pytorch/pytorch/blob/2bec68e73b64715354af076ad309335f943e36cd/torch/_inductor/cpp_builder.py#L613-L631Local validation:

After this fixing, it should shows the verbose build fail message:
New contributor declaration
I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end testsFILL THIS IN.Select one of the following.
littests.littests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)