Split wrap_symint out of wrap_unspecialized_primitive #125483
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/125483
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 449acdd with merge base 79af814 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
While there are some similarities, they are also quite different (one handles Numpy numbers while the other handles ints. I am also going to add a wrap_symfloat soon which will do even more different behavior. So split these out for clarity. Signed-off-by: Edward Z. Yang <[email protected]> ghstack-source-id: fe0128b2113f3de87d9aeb40356d95437e77f1d3 Pull Request resolved: #125483
github-actions
bot
requested review from
albanD,
antoniojkim,
bdhirsh,
miladm and
SherlockNoMad
While there are some similarities, they are also quite different (one handles Numpy numbers while the other handles ints. I am also going to add a wrap_symfloat soon which will do even more different behavior. So split these out for clarity. Signed-off-by: Edward Z. Yang <[email protected]> ghstack-source-id: 12612e4e2da189b8167c606c3a2ed80595ec4103 Pull Request resolved: #125483
| ) | ||
|
|
||
| # NB: We do not do float. For motivation, see | ||
| # https://docs.google.com/document/d/1INSCdYu1PxXcr43HrD82OudeEuS-qxQe1yZmLg2wy6A/edit |
There was a problem hiding this comment.
It's actually not stale, but I'm going to be ripping all this out soon anyway
| # TODO: Do I actually need guard for constant source? | ||
| self.install_guards(GuardBuilder.CONSTANT_MATCH) |
There was a problem hiding this comment.
Yes. But note that we already have a version of this path in wrap_literal with a much more complex condition, so it would be good to reconcile them. Unless you plan to use this function in other contexts, of course.
| elif ( | ||
| type(value) is int | ||
| and not is_constant_source(self.get_source()) | ||
| and not isinstance(self.get_source(), RandomValueSource) |
There was a problem hiding this comment.
Why were RandomValues not allowed here before and why do we allow them now?
There was a problem hiding this comment.
The prior behavior, which this PR makes clear (because I did not change this at all), is that the RandomValue case can only occur under the wrap_unspecialized_primitive path, which eventually produces the UnspecializedPythonVariable subclass (that is treated like a Tensor). So wrap_symint can never be called with a RandomValueSource.
This is a kind of illogical separation, it's more logical for an integer random variable to be treated as a SymNodeVariable, but the point of this PR is not to fix the problem yet, it's just to make it more clear what the current behavior is.
| self.tx.output.unspec_variable_map[self.name] = unspec_var | ||
| if not is_constant_source(self.get_source()): | ||
| if self.tx.export and not isinstance(self.get_source(), LocalSource): | ||
| raise AssertionError( | ||
| f"Dynamo attempts to add additional input during export: value={wrapped_value}, source={self.get_source()}" |
There was a problem hiding this comment.
I'm not really sure what I want to do with this code. I suppose it can be factored to a method. It's pretty short though, I might end up rewriting it all
|
If people hate this "defactor" then I will probably just absorb it into the actual rewrite that is coming later in the stack. This is split out because it is change neutral (no behavior change) and I think will make the next rewrite easier to review. |
There was a problem hiding this comment.
Nah, it's fine.
You might also consider submitting the rest of changes as one PR with clean commit history to be able to have all the changes separated and easy to review, but also a clear end state that does not instantiate all the intermediary suboptimal states.
We use very little of the code in wrap_fx_proxy_cls, so dupe it out. Signed-off-by: Edward Z. Yang <[email protected]> Pull Request resolved: #125494 Approved by: https://github.com/lezcano ghstack dependencies: #125395, #125419, #125483
Signed-off-by: Edward Z. Yang <[email protected]> Pull Request resolved: #125496 Approved by: https://github.com/lezcano ghstack dependencies: #125395, #125419, #125483, #125494
Stack from ghstack (oldest at bottom):
While there are some similarities, they are also quite different (one
handles Numpy numbers while the other handles ints. I am also going to
add a wrap_symfloat soon which will do even more different behavior.
So split these out for clarity.
Signed-off-by: Edward Z. Yang [email protected]
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang