[inductor] Fix an unused variable in cpu_vec_isa.py #138473
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/138473
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 65ca3ef with merge base 288aa87 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
What is status variable here? Is this actually a case of missing error handling? |
|
@pytorchbot rebase |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
|
Successfully rebased |
| x86_isa_help_builder.get_target_file_path() | ||
| ) | ||
| if not os.path.isfile(output_path): | ||
| status, target_file = x86_isa_help_builder.build() |
There was a problem hiding this comment.
It feels like the status value here should be checked...
There was a problem hiding this comment.
Slightly unexpected results!
That line calls CppBuilder.build, which calls run_compile_command which calls _run_compile_command, which calls subprocess.check_output.
But check_output returns the contents of stdout and not a status code! (If the subprocess fails, check_output raises an exception which the code handles.)
The contents of stdout might contain any old junk and run_compile_command is only called in that one spot, so I got rid of that return value entirely.
| def build(self) -> Tuple[bytes, str]: | ||
| def build(self) -> None: | ||
| """ | ||
| It is must need a temperary directory to store object files in Windows. |
There was a problem hiding this comment.
@Skylion007: This is a little off-topic, but I see two issues in CppBuilder.build().
The first is that the docstring is not really useful.
The second is that the temporary directory doesn't get actually get deleted if run_compile_cmd raises an exception.
If this is intended behavior, it should be documented; otherwise, the _remove_dir should be in a finally: block.
I'd be happy to fix both of these while I was here.
| def _run_compile_cmd(cmd_line: str, cwd: str) -> None: | ||
| cmd = shlex.split(cmd_line) | ||
| try: | ||
| status = subprocess.check_output(args=cmd, cwd=cwd, stderr=subprocess.STDOUT) |
There was a problem hiding this comment.
I intend to keep current code. check_output need work with many OSs and many compilers. They may return different behaviors, even some error output messages. If we can't make sure we can make it better, we'd better keep current code.
There was a problem hiding this comment.
status is not a status variable: it is a byte string which merges stdout and stderr. At the very least, it should be correctly named.
But this result simply isn't used at all, anywhere in the code - indeed, it's not clear how it could be used since we have no information about what the output might be, or even if that output is stable between runs of the compiler or different settings.
Keeping unused, trivial code around just in case it might be used in the future is not a good idea.
You will note that this variable has already consumed time from three different engineers, because its name gives the false impression that we had forgotten to check a status variable.
There was a problem hiding this comment.
|
cc @EikanWang I think this would need a slightly larger cleanup if we want to change the variables to actually output a proper status flag etc. Would someone on your side be able to help with that by any chance? Or is deleting all the wrong variables as done here is ok for now and we can add back proper code later when it is needed? |
* Extracted from pytorch/pytorch#133492 ghstack-source-id: 4103a77 Pull Request resolved: pytorch/pytorch#138473
Sorry for the late response. @albanD , @rec , let's remove all the wrong variables just as this PR has done. |
albanD
left a comment
albanD
left a comment
There was a problem hiding this comment.
Sounds like it's ready to land, happy to help reviewing the follow up improvements if needed!
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
| def _run_compile_cmd(cmd_line: str, cwd: str) -> None: | ||
| cmd = shlex.split(cmd_line) | ||
| try: | ||
| status = subprocess.check_output(args=cmd, cwd=cwd, stderr=subprocess.STDOUT) |
Stack from ghstack (oldest at bottom):
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov