[Dynamo] Fix - str handler for UserDefinedObjectVariable

[datagero]

Fixes #130301

Adjusted the call_str method to handle str conversion for UserDefinedObjectVariable.
Attempt in a clean branch for unrelated test errors.

cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10 @voznesenskym @penguinwu @EikanWang @Guobing-Chen @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/130506

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 318af0f with merge base d039b14 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[datagero]

@oulgen @anijain2305 second attempt on this PR (previous #130320), it does seem that the changes affect the tests for some setups, which I cannot replicate locally.

PYTORCH_TEST_WITH_DYNAMO=1 python test/test_nn.py -k TestNN.test_ParameterList_meta

In other setups (e.g., screenshot passes for dynamo, 3, 3, linux.2xlarge) I was not able to find this specific test, not sure why.

Due to the proposed changes, there be trouble retireving 'value' and/or '_size' attributes from ParameterList UserDefinedObjectVariable. For '_size' not entirely sure why it would have difficulty since the attribute is defined in the class init function. Current commit is hoping to get some additional logs to understand better.

I'm trying to run some tests on the CI but cannot trigger the tests - is there a way I can achieve this?

[datagero]

Hi @oulgen @anijain2305 - I think I need an approval to proceed with the CI tests?

[oulgen]

I clicked on the run tests button, each time you publish a new version, you need someone to run tests for you

[datagero]

@oulgen finally found the error, from test/test_nn.py], what I understand a str() function on the test ParameterList may not be not collecting/starting the _size parameter when we need it, by changing this to repr() the solution works.

[datagero]

hi @oulgen are you able to help triggering the tests to confirm changes? 🙏

[datagero]

@oulgen In test_saving_variable_to_disk defining tensors inside the saved_tensors_hooks context caused unintended side effects during assertions, specifically when calling _compare_regular_values_close. This was fixed by moving the tensor definitions (a and y) outside the saved_tensors_hooks context.

Test pass locally now:

[oulgen]

you shouldnt need to do this? @bdhirsh

[datagero]

Thanks for the review. Indeed I do not fully understand the effects.
@bdhirsh This is what I was able to infer:
In test_saving_variable_to_disk defining tensors inside the saved_tensors_hooks context caused unintended side effects during assertions, specifically when calling _compare_regular_values_close. This was fixed by moving the tensor definitions (a and y) outside the saved_tensors_hooks context.

[datagero]

Hi @bdhirsh would appreciate any guide/comment on this one!

[bdhirsh]

pulled your PR locally and this test fails for me with the change. taking a look

[ashwanirathee]

Is there more work needed on this or close to done @datagero?

[datagero]

@ashwani-rathee waiting on some feedback on a test/test_autograd.py modification; hopefully from @bdhirsh, unless there's someone else that can provide feedback? @oulgen

[anijain2305]

Maybe check if the arg.value has an overridden __str__ method. If yes, then either graph break right now, or inline the __str__ function.

[datagero]

So for the test/test_autograd.py discussed above, the call to str below is causing the issue for me. @bdhirsh were you able to have a look?

            def pack(x):
                name = os.path.join(tmp_dir, str(uuid.uuid4()))
                torch.save(x, name)
                return name

type(obj).__str__ is overriden to <function UUID.__str__ at 0x1026a8040>

if on the above we avoid str() and call uuid.uuid4().hex or uuid.uuid4().__str__(), then the test pass for me. Otherwise (even if changing torch/_dynamo/variables/builtin.py 1025 to be in-line value=arg.value.__str__()), it will throw assertion error:

AssertionError: Tensor-likes are not close!

Mismatched elements: 5 / 5 (100.0%)
Greatest absolute difference: 2.0 at index (0,) (up to 1e-05 allowed)
Greatest relative difference: inf at index (0,) (up to 1.3e-06 allowed)

[anijain2305]

You can do something like

elif isinstance(arg, (variables.UserDefinedObjectVariable)) and type(arg.value).__str__ is object.__str__:

This will incrementally add support for user defined objects relying on the object str method. And for rest, it will graph break.

If you want to support custom __str__ method then you might need to do more work.

1. Inline the __str__ method. Search for tx.inline_user_function_return to get an idea on how we inline.
2. You will have to account for __repr__ functions when __str__ is absent. See the next comment.

[anijain2305]

I think the reason its failing for you because str(x) does much more than just calling x.__str__()

[datagero]

Thanks @anijain2305 will have a look today, seems good pointers! The requirement of the ticket is to deal with custom __str__ methods so I'lll look at tx.inline_user_function_return and your notes.

From the ticket:

import torch

class C:
    x = 1

    def __str__(self):
        return "ok"

def foo(x):
    a = C()
    return x, str(a)

print(torch.compile(foo, fullgraph=True)(torch.ones(4)))

[datagero]

Sorcery! That helped a lot

[datagero]

• Original Ticket: Handles custom objects with custom __str__ functions.
  -TestAutograd.test_saving_variable_to_disk: Requires inline str handling for custom objects due to possible graph breaks. Use tx.inline_user_function_return
  -TestNN.test_ParameterList_meta: Catches issues with ParameterList, which throws an AttributeError with call_str. We return empty (i.e., same behaviour as before this implementation) as this is possible graph break.

For reviewer (@anijain2305 , @oulgen , @bdhirsh ):

• I'd appreciate if we can trigger the CI tests to check fixes
• Happy to adjust for any feedback on style / or the comment descriptions for "graph break" need revision for technical accuracy

[anijain2305]

No need of this line. L1064 already does it.

[anijain2305]

I think we should do an if condition instead of relying on Exception. We should probably just call VariableBuilder (cc @mlazos, I think we need to add a function in builder.py for identifying C functions and graph break on them). But I think this is not required in this PR.

[datagero]

Thanks - I did try/except in case UserFunctionVariable gets expanded to other function types. As per your note, I did not adjusted this on last push.

[anijain2305]

Thanks for the changes.

[oulgen]

can you revert these changes?

[datagero]

@oulgen done

[anijain2305]

@pytorchbot merge

[pytorchmergebot]

Merge started

Your 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

Advanced Debugging

Check the merge workflow status
here

[pytorchmergebot]

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / macos-py3-arm64 / test (default, 3, 3, macos-m1-stable)

Details for Dev Infra team

Raised by workflow job

[anijain2305]

@pytorchbot merge

[pytorchmergebot]

Merge started

Your 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

Advanced Debugging

Check the merge workflow status
here