[fx] Add strict argument validation to Interpreter.boxed_run#166784
[fx] Add strict argument validation to Interpreter.boxed_run#166784meghendra6 wants to merge 4 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/166784
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit f157e8b with merge base 7b64ad9 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
torch/fx/interpreter.py
Outdated
| placeholder_nodes = [n for n in self.graph.nodes if n.op == "placeholder"] | ||
| expected_args = len(placeholder_nodes) | ||
| actual_args = len(args_list) | ||
| if actual_args != expected_args: | ||
| detail = ( | ||
| "extra arguments" | ||
| if actual_args > expected_args | ||
| else "missing arguments" | ||
| ) | ||
| raise RuntimeError( | ||
| f"Interpreter.boxed_run expected {expected_args} arguments " | ||
| f"for placeholders but received {actual_args} ({detail})." | ||
| ) | ||
|
|
||
| env = {n: arg for n, arg in zip(placeholder_nodes, args_list)} |
There was a problem hiding this comment.
This seems like a lot of changes, could we just add an assertion on line 228?
args_iter = iter(args_list)
env = {}
for n in self.graph.nodes:
if n.op == "placeholder":
env[n] = next(args_iter)
+ assert len(args_list) == len(env), "<error message>"There was a problem hiding this comment.
Thanks for the suggestion! I went ahead and simplified the code following your idea. The commit (4353144
) keeps the original loop and adds a simple check for the arg count. Appreciate the quick feedback!
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
Yeah this seems overly complicated
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
I think I would prefer simplifying this more by collecting the list of nodes to assign first, and then doing the length check, and then assigning. This eliminates the StopIteration logic.
Good point, that makes sense. I’ve updated the code in 81c6beb |
|
@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 |
Merge failedReason: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
|
@ezyang I've checked the CI failures, and they appear to be unrelated to the changes in this PR. The main issue seems to be in This looks like an infrastructure or environment issue, not caused by my code. Could someone advise on how to proceed? Happy to rebase or try a re-run. |
|
dynamo_wrapped contain the most important tests for this PR, could we rebase on viable/strict to let CI try again? |
|
@pytorchbot merge -r |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
Fixes issue where extra arguments were silently ignored, leading to graphs running with incorrect inputs. Now raises clear RuntimeError for both extra and missing arguments. Also improves error messages for better debugging experience.
|
Successfully rebased |
pytorchmergebot
force-pushed
the
fx-interpreter-boxed-run-strict-args
branch
from
2ff84f6 to
f157e8b
Compare
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 |
Merge failedReason: 3 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
|
It looks like all checks are green, but the PR still shows that the merge failed. I'm a bit confused about what's causing the issue. Any chance you could see what's going on? |
|
@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 |
6 participants
Summary
This PR fixes an issue where
torch.fx.Interpreter.boxed_runwould silently ignore extra input arguments instead of validating the argument count.Previously,
boxed_runwould only consume as many inputs as there were placeholder nodes and then clear the entireargs_list, hiding potential bugs. This change introduces a strict check to ensurelen(args_list)matches the number of placeholder nodes, raising aRuntimeErroron a mismatch.Fixes #166583.
Changes
len(args_list)against the number of placeholder nodes at the beginning ofboxed_run.RuntimeErrorwith a clear message ("extra arguments" or "missing arguments") if the counts do not match.args_list.clear()to only execute after successful validation and environment setup. If an error is raised,args_listis preserved for debugging.Testing
test_interpreter_boxed_run_argument_validationtotest/test_fx.py.args_listis cleared).RuntimeError,args_listis preserved).RuntimeError,args_listis preserved).User-facing impact / BC notes
This is a bug fix. Code that was incorrectly passing the wrong number of arguments to
boxed_runwill now fail fast with aRuntimeErrorinstead of executing silently with unintended inputs. Correctly written code is unaffected.cc @ezyang @EikanWang @jgong5 @wenzhe-nrv @chauhang @penguinwu @xmfan