use sourceless builder for builtin getattr #113340
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113340
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8db8464 with merge base dadca7a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pmeier
added
the
keep-going
There was a problem hiding this comment.
I think this looks reasonable as-is. There are probably some undesirable corner cases like dynamically created classes. However, one of the export test cases show that substituting type(A) with A.__class__ works today and I can't see why they should be treated differently.
pmeier
force-pushed
the
dynamo-sourceless
branch
from
17bcfed
to
572d983
Compare
| @@ -1271,7 +1273,7 @@ def call_type(self, tx, obj: VariableTracker): | |||
| return VariableBuilder(tx, TypeSource(obj.source))(py_type) | |||
|
|
|||
| if py_type is not None: | |||
| return ConstantVariable.create(py_type) | |||
| return SourcelessBuilder()(tx, py_type) | |||
There was a problem hiding this comment.
cc @yanboliang Is this alright? The issue torchvision was hitting was taht py_type = torch.Tensor coming from a sourceless tensor, it was wrongly being wrapped into a ConstantVariable rather than a TorchVariable.
Is this alright, or would you want us to be a bit less general here and just do this for torch.Tensor so that most paths still hit the UserError?
There was a problem hiding this comment.
VariableTrackers produced by SourcelessBuilder are unguarded, what happens if there is a graph break after a type(obj) call if obj is generated internally (doesn't have a source)? I think it works well for constant and torch variables, we can't guarantee if they are other variables. But I don't expect a typecall returns too complicated data structure, so it should be fine IMO.
There was a problem hiding this comment.
Ok, so Ed argues in #113390 that this is alright (that PR subsumes this one)
albanD
added
the
triaged
In TorchVision we use the following (simplified) dispatch mechanism:
```python
import torch
def kernel1(tensor):
return tensor + 2
def dispatcher1(input):
kernel = get_kernel(dispatcher1, type(input))
return kernel(input)
def kernel2(tensor):
return tensor - 2
def dispatcher2(input):
kernel = get_kernel(dispatcher2, type(input))
return kernel(input)
# We actually use the function and type as keys, rather than their names.
# However, this currently not supported, but should be easy to add after
# pytorch#111196
REGISTRY = {
"dispatcher1": {"Tensor": kernel1},
"dispatcher2": {"Tensor": kernel2},
}
def get_kernel(dispatcher, input_type):
dispatcher_registry = REGISTRY[dispatcher.__name__]
for cls in input_type.__mro__:
kernel = dispatcher_registry[cls.__name__]
break
return kernel
```
This can be compiled without graph breaks:
```python
cfn = torch.compile(dispatcher1, fullgraph=True)
torch.testing.assert_close(int(cfn(torch.tensor(3))), 5)
cfn = torch.compile(dispatcher2, fullgraph=True)
torch.testing.assert_close(int(cfn(torch.tensor(3))), 1)
```
However, if we start chaining these calls, we hit some issues:
```python
class Pipeline(torch.nn.Module):
def forward(self, input):
input = dispatcher1(input)
input = dispatcher2(input)
return input
cfn = torch.compile(Pipeline(), fullgraph=True)
torch.testing.assert_close(int(cfn(torch.tensor(3))), 3)
```
```
Can't access members of type(obj) for a generated custom object. Please use __class__ instead
```
The error message is not really helpful here. The following happens: when compiling `dispatcher1`, `get_kernel` gets inlined. That means when hitting `dispatcher2`, the `type` call no longer happens on an input with a source. Thus, in the first iteration we hit the top branch, while in the second we hit the bottom:
https://github.com/pytorch/pytorch/blob/addb8e29cd842e1a290cb0b55662ee0423ab2498/torch/_dynamo/variables/builtin.py#L1264-L1268
And the error message I posted above originates from the type being treated as constant. This PR replaces this with a `SourcelessBuilder` instead.
With that fix in place, we hit another pointing to `input_type.__mro__`
```
AssertionError: Consider SourcelessBuilder for ephemeral objects, usually objects created locally.
```
Fix is similar: instead of using a `VariableBuilder` here, we use a `SourcelessBuilder` in case we have no `source`:
https://github.com/pytorch/pytorch/blob/addb8e29cd842e1a290cb0b55662ee0423ab2498/torch/_dynamo/variables/builtin.py#L1167-L1168
Pull Request resolved: pytorch#113340
Approved by: https://github.com/peterbell10, https://github.com/lezcano
huydhn
removed
the
keep-going
|
@pytorchbot revert -m 'Sorry for reverting your change, but the test is failing internally' -c ghfirst 2 test are failing
I'm not quite sure what the parameterized lambdas are, but they both fail with no test found error: |
|
@pytorchbot successfully started a revert job. Check the current status here. |
|
@pmeier your PR has been successfully reverted. |
This reverts commit d64bc8f. Reverted #113340 on behalf of https://github.com/huydhn due to Sorry for reverting your change, but the test is failing internally ([comment](#113340 (comment)))
|
@huydhn Are you sure this is coming from this PR and not #113390? #113390 fixed some behavior with how types are handled by dynamo and in the process adapted the test pytorch/test/dynamo/test_export.py Line 2854 in 9b0f2f8 (the same test name as in your traceback above). Basically there was a check that an error was raised that was removed, since #113390 fixed the underlying issue. My PR here also touches this test, but only cleans it up and does not change about the way dynamo deals with types. That being said, if #113390 is reverted as well, we need to wait for it to be relanded before this PR is merged as well. |
|
Or maybe for whatever reason, the internal CI doesn't like lambdas in the parametrization? Not sure how this test came into being, but it seems like a "copy" from the test I linked above? |
|
Here is a diff to avoid having a diff --git a/test/dynamo/test_export.py b/test/dynamo/test_export.py
index 2653f08b4f8..e3cb30cc207 100644
--- a/test/dynamo/test_export.py
+++ b/test/dynamo/test_export.py
@@ -2851,8 +2851,13 @@ def forward(self, x):
with self.assertRaisesRegex(RuntimeError, "Shape must be more than 4"):
gm(torch.randn(3, 4, 5))
- @common_utils.parametrize("type_fn", [type, lambda obj: obj.__class__])
- def test_access_class_method_from_user_class(self, type_fn):
+ @common_utils.parametrize("type_access", ["builtin", "class"])
+ def test_access_class_method_from_user_class(self, type_access):
+ if type_access == "builtin":
+ type_fn = type
+ elif type_access == "class":
+ type_fn = lambda obj: obj.__class__
+
class A:
@classmethod
def func(cls):Not sure why the lambda could be a problem in the parametrization, but maybe worth a try? |
|
Tested my patch in #113340 (comment) with @NicolasHug and it passed internal checks. The likely problem was that the lambda in the test name has a memory address in it and if test collection and execution are not happening in the same process, the test is not found. @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 |
|
@pmeier to be frank, I would like to argue that having lambda address as part of the test name is bad UX |
|
You can use |
Totally agree here. I made the same assumption as you regarding the test name. But now that I know that this is not the case, I pushed my patch from #113340 (comment) in 131e177 that gets rid of lambda parametrization, but rather just uses strings. Subtest is only really valuable if you have a for loop inside your test, which is an antipattern to begin with. |
|
@lezcano I was mistaken here. I thought you were talking about correct? If so, you are right and I could have used that. Will send a cleanup PR. |
#113340 was reverted initially due to a bad default parametrization name. The test looked like ```python common_utils.parametrize( "type_fn", [ type, lambda obj: obj.__class__, ], ) def test_access_class_method_from_user_class(self, type_fn): ``` This is a valid parametrization, but results in these default test names: ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<class 'type'> test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<function ExportTests_<lambda> at 0x7f3be5de0c10> ``` Ignoring the whitespace in the test names, which can lead to other issues down the line, the problem in #113340 was that the lambda parameter included a memory address. IIUC, internally, the tests are not collected and run in the same process. Meaning, the address of the lambda and in turn the test name is no longer valid on the runner. This is fixed earlier in the stack by giving the parametrization an explicit name with `subtest`, but this PR is about preventing issues in the default case. `pytest` solves this by simply using the name of the parameter plus its index as id in the test name: ```python import pytest class Foo: def __repr__(self): return str(id(self)) pytest.mark.parametrize( "bar", [ pytest.param(type), pytest.param(lambda obj: obj.__class__), pytest.param(Foo()), ], ) def test_foo(bar): pass ``` ``` ❯ pytest main.py --co -q main.py::test_foo[type] main.py::test_foo[<lambda>] main.py::test_foo[bar2] ``` `pytest` has better defaults for `type` and `lambda` than we do, but is has a safe default for custom objects. This PR aligns our default test name with `pytest`. Using the parametrization from above again, we now collect ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn0 test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn1 ``` which might not be as expressive at first glance, but at least prevents bugs. [ghstack-poisoned]
#113340 was reverted initially due to a bad default parametrization name. The test looked like ```python common_utils.parametrize( "type_fn", [ type, lambda obj: obj.__class__, ], ) def test_access_class_method_from_user_class(self, type_fn): ``` This is a valid parametrization, but results in these default test names: ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<class 'type'> test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<function ExportTests_<lambda> at 0x7f3be5de0c10> ``` Ignoring the whitespace in the test names, which can lead to other issues down the line, the problem in #113340 was that the lambda parameter included a memory address. IIUC, internally, the tests are not collected and run in the same process. Meaning, the address of the lambda and in turn the test name is no longer valid on the runner. This is fixed earlier in the stack by giving the parametrization an explicit name with `subtest`, but this PR is about preventing issues in the default case. `pytest` solves this by simply using the name of the parameter plus its index as id in the test name: ```python import pytest class Foo: def __repr__(self): return str(id(self)) pytest.mark.parametrize( "bar", [ pytest.param(type), pytest.param(lambda obj: obj.__class__), pytest.param(Foo()), ], ) def test_foo(bar): pass ``` ``` ❯ pytest main.py --co -q main.py::test_foo[type] main.py::test_foo[<lambda>] main.py::test_foo[bar2] ``` `pytest` has better defaults for `type` and `lambda` than we do, but is has a safe default for custom objects. This PR aligns our default test name with `pytest`. Using the parametrization from above again, we now collect ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn0 test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn1 ``` which might not be as expressive at first glance, but at least prevents bugs. [ghstack-poisoned]
Cleanup from #113340 (comment). ``` ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_attr test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_builtin ``` Pull Request resolved: #113855 Approved by: https://github.com/lezcano, https://github.com/huydhn
#113340 was reverted initially due to a bad default parametrization name. The test looked like ```python @common_utils.parametrize( "type_fn", [ type, lambda obj: obj.__class__, ], ) def test_access_class_method_from_user_class(self, type_fn): ``` This is a valid parametrization, but results in these default test names: ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<class 'type'> test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn_<function ExportTests_<lambda> at 0x7f3be5de0c10> ``` Ignoring the whitespace in the test names, which can lead to other issues down the line, the problem in #113340 was that the lambda parameter included a memory address. IIUC, internally, the tests are not collected and run in the same process. Meaning, the address of the lambda and in turn the test name is no longer valid on the runner. This is fixed earlier in the stack by giving the parametrization an explicit name with `subtest`, but this PR is about preventing issues in the default case. `pytest` solves this by simply using the name of the parameter plus its index as id in the test name: ```python import pytest class Foo: def __repr__(self): return str(id(self)) @pytest.mark.parametrize( "bar", [ pytest.param(type), pytest.param(lambda obj: obj.__class__), pytest.param(Foo()), ], ) def test_foo(bar): pass ``` ``` ❯ pytest main.py --co -q main.py::test_foo[type] main.py::test_foo[<lambda>] main.py::test_foo[bar2] ``` `pytest` has better defaults for `type` and `lambda` than we do, but is has a safe default for custom objects. This PR aligns our default test name with `pytest`. Using the parametrization from above again, we now collect ```bash ❯ pytest test/dynamo/test_export.py -k test_access_class_method_from_user_class --co -q test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn0 test/dynamo/test_export.py::ExportTests::test_access_class_method_from_user_class_type_fn1 ``` which might not be as expressive at first glance, but at least prevents bugs. Pull Request resolved: #113856 Approved by: https://github.com/malfet, https://github.com/huydhn ghstack dependencies: #113855
In TorchVision we use the following (simplified) dispatch mechanism:
This can be compiled without graph breaks:
However, if we start chaining these calls, we hit some issues:
The error message is not really helpful here. The following happens: when compiling
dispatcher1,get_kernelgets inlined. That means when hittingdispatcher2, thetypecall no longer happens on an input with a source. Thus, in the first iteration we hit the top branch, while in the second we hit the bottom:pytorch/torch/_dynamo/variables/builtin.py
Lines 1264 to 1268 in addb8e2
And the error message I posted above originates from the type being treated as constant. This PR replaces this with a
SourcelessBuilderinstead.With that fix in place, we hit another pointing to
input_type.__mro__Fix is similar: instead of using a
VariableBuilderhere, we use aSourcelessBuilderin case we have nosource:pytorch/torch/_dynamo/variables/builtin.py
Lines 1167 to 1168 in addb8e2
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng @avikchaudhuri @gmagogsfm @zhxchen17 @tugsbayasgalan @angelayi @suo @ydwu4