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
Make DimConstraints create actionable message #100103
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/100103
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 9e0d86a: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
55688d2
to
2765afe
Compare
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.
I'll wait for the tests to finish before approving.
test/test_dynamic_shapes.py
Outdated
action_code = dim_constraints.prettify_results(inspect.signature(dummy_f)) | ||
static_code, dynamic_code = re.findall(r"```(.*?)```", action_code, re.DOTALL) | ||
expected_static = ''' | ||
def static_constraints(x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x11, x12): |
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.
This function doesn't do anything. One idea would be to join the constraints with and
(still on separate lines). Then perhaps the user can call it to check specialized inputs.
While you're at it, call this specializations
and call the other specify_constraints
, since the API calls the result constraints
.
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.
yeah, sounds good to me. For specializations call, it seems to possibly contain assignment operator "=": https://github.com/pytorch/pytorch/blob/main/torch/fx/experimental/symbolic_shapes.py#L1638. Should we just turn it into "==" and in the _static_results?
test/test_proxy_tensor.py
Outdated
gm.shape_env.produce_guards(fx_placeholder_vals(gm), names, _simplified=True, constraint_inputs=None) | ||
) | ||
guards = gm.shape_env.produce_guards_and_constraints(fx_placeholder_vals(gm), | ||
names, _simplified=True, constraint_inputs=None)[0] |
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.
funny formatting
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.
Yeah, I can change it to other format. lintrunner -a somehow didn't work.
@@ -1994,11 +2008,13 @@ def create_symbol( | |||
self.var_to_sources[r].append(source) | |||
return r | |||
|
|||
# Generates a list of guards strings which, when evaluated in a context that | |||
# Generates a tuple of 1. a list of guards strings which, when evaluated in a context that |
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.
Maybe break 1. and 2. into separate lines, always find it hard to read otherwise.
torch/_dynamo/guards.py
Outdated
guards = output_graph.shape_env.produce_guards( | ||
( | ||
guards, | ||
shape_constraints, |
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.
These are only input shape constraints, not include inline shape constraints due to unbacked symint right?
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.
Yep i think we should name it dim_constraints to be consistent with original naming.
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.
Sure
if self._static_results: | ||
sorted_static_results = sorted(self._static_results) |
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.
Is it sorted by tensor first then by dim?
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.
It's just sorted lexicographically to have deterministic results.
torch/_dynamo/eval_frame.py
Outdated
@@ -800,6 +805,7 @@ def result_capturing_wrapper(*graph_inputs): | |||
hooks=Hooks( | |||
guard_export_fn=guard_export_print, | |||
guard_fail_fn=None, | |||
constraint_export_fn=constraint_export, |
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.
I feel like we don't need a hook here. In here (https://github.com/pytorch/pytorch/pull/100103/files#diff-c663df72d7a648204ff256e604fe3c9b393e7e2d58204e3bc818d90a99712911L768), you already have access to fake tensors where you can just access the shape_env directly.
torch/_dynamo/guards.py
Outdated
guards = output_graph.shape_env.produce_guards( | ||
( | ||
guards, | ||
shape_constraints, |
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.
Yep i think we should name it dim_constraints to be consistent with original naming.
torch/_dynamo/hooks.py
Outdated
@@ -7,3 +7,4 @@ | |||
class Hooks: | |||
guard_export_fn: Optional[Callable[[Set["Guard"]], None]] = None | |||
guard_fail_fn: Optional[Callable[[Tuple["GuardFail"]], None]] = None | |||
constraint_export_fn: Optional[Callable[["DimConstraints"], None]] = None |
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.
doesn't seem like we need it if we just access the fake tensor and get shape env from there.
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.
So we just add dim_constraints as part of ShapeEnv, right? That's sounds better to me.
test/test_dynamic_shapes.py
Outdated
x4.size()[0] == 8 | ||
x5.size()[1] = 22 | ||
x7.size()[3] == 96 | ||
x8.size()[1] == 22 |
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.
I am bit confused why it needs to be wrapped in function. We don't expect user to execute this function right? Maybe it is better if we just print it as it is
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.
The naming of function signature may be different from user program:
args = [t1, t2, t3]
def f(x0, x1, x2):
# computations on x0, x1, x2
pass
torchdynamo.export(f, *args)
In this case, directly returned constraints may look like this:
constraints = [
2 <= dynamic_dim(x1, 1),
dynamic_dim(x2, 1) == dynamic_dim(x1, 1),
dynamic_dim(x1, 1) == dynamic_dim(x0, 1),
]
It's not directly copy-pastable: users need to find the mapping between t1, t2, t3 and x0, x1, x2. If we export a function with the same signature, we could directly call specify_constriants like below:
def specify_constraints(x0, x1, x2):
return [
2 <= dynamic_dim(x1, 1),
dynamic_dim(x2, 1) == dynamic_dim(x1, 1),
dynamic_dim(x1, 1) == dynamic_dim(x0, 1),
]
constraints = specify_constraitns(*args)
We could use the same calling convention as original f. Does it make sense?
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.
I guess I am curious why the static constraints like x7.size()[3] == 96
are wrapped inside function that doesn't return anything. For dynamic constraints, it totally makes sense :)
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.
Ohh, sorry I thought you were referring to static ones. For the static constraints, we'll call it specializations and make it return a boolean in this revision. It seems to be prettier than the L[""] and more consistent with specify_constraints.
2765afe
to
1cf6e21
Compare
torch/_dynamo/eval_frame.py
Outdated
@@ -827,6 +827,24 @@ def result_capturing_wrapper(*graph_inputs): | |||
assert out_guards is not None, "Failed to produce guards during tracing" | |||
assert fake_mode is not None | |||
|
|||
if tracing_mode == "symbolic": |
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.
I think it will always be a fake tensor received to the export backend? So probably no need for check here
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.
ok, let's just use the already instantiated fake_mode then.
torch/_dynamo/eval_frame.py
Outdated
if "example_value" in node.meta: | ||
return node.meta[ | ||
"example_value" | ||
].fake_mode.shape_env.dim_constraints |
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.
Why not just do it here? (
pytorch/torch/_dynamo/eval_frame.py
Line 789 in ae0eb23
fake_mode = _guards.detect_fake_mode(inner_example_inputs) |
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.
Thanks for pointing out! That makes the code even simpler.
1cf6e21
to
9e0d86a
Compare
def specializations(x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x11, x12): | ||
return (x0.size()[0] == 8 and | ||
x1.size()[2] == 96 and | ||
x11.size()[1] == 1 and | ||
x12.size()[2] == 3 and | ||
x2.size()[0] == 8 and | ||
x3.size()[0] == 8 and | ||
x4.size()[0] == 8 and | ||
x5.size()[1] == 22 and | ||
x7.size()[3] == 96 and | ||
x8.size()[1] == 22) |
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.
This is to inform users that those dims are specialized. Users don't need to paste and use it anywhere explicitly right?
@tugsbayasgalan These are also turned into runtime check correct? If it's not stored in metadata, how do you find this info
@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 |
This pr makes summary of dimension constraints actionable. Before the pr, it will print:
Users need to initialize the L environment manually and copy the constraints over. After the pr, we have:
, where dynamic_constraints has the same input signature as users code. This allow users to copy-paste and run the code to generate the constraints before exporting as shown below:
Implementation-wise, this pr also
The purpose is to surface the DimConstraints to dynamo.export, where we could reliably get the original function's signature.
The alternative to the above is to get the function signature before creating SHAPE_ENV guard (https://github.com/pytorch/pytorch/blob/main/torch/_dynamo/output_graph.py#L227) and pass it to DimConstraints, but I couldn't recover the signature before creating SHAPE_ENV because the frame's f_globals/locals don't contain the original function.
cc @soumith @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @desertfire