Skip to content
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

Re-land: dynamo expand test with view-replay. #6958

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

ysiraichi
Copy link
Collaborator

This PR is a follow-up to #6950, re-introducing the dynamo expand test while setting the view_replay_for_aliased_outputs dynamo configuration that uses the outputs of the functionalization step for reconstructing output views.

cc @miladm @JackCaoG

@@ -671,6 +676,22 @@ def foo(x):
self.assertEqual(expected.dtype, actual.dtype)
self.assertEqual(expected.device, actual.device)

def test_return_expand(self):
_set_view_replay_for_aliased_outputs()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to leave this options to always true througout the test? If the intention is to always set this to True for PyTorch/XLA, you should set it in https://github.com/pytorch/xla/blob/master/torch_xla/__init__.py .

@@ -0,0 +1 @@
680a89e2ced8f45b87b7fd0509c07cf6b768be14
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a pin? I thought Brian's change already merged to main.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was reverted.

@ysiraichi
Copy link
Collaborator Author

@JackCaoG @vanbasten23 I'm puzzled by this CI failure. Doesn't TPU CI build take the .torch_pin into account? It looks like it's not

@JackCaoG
Copy link
Collaborator

JackCaoG commented Apr 25, 2024

yea TPU CI does not take into pin into account yet. I think it is oK to merge once you remove the pin. @will-cromar FYI

@ysiraichi ysiraichi force-pushed the ysiraichi/add-expand-test-retry branch from 72426d7 to 2c79b7c Compare April 30, 2024 03:07
@ysiraichi ysiraichi merged commit 42db709 into master Apr 30, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants