Make empty_symint support dynamism by vanbasten23 · Pull Request #4524 · pytorch/xla

vanbasten23 · 2023-01-27T05:53:58Z

No description provided.

JackCaoG · 2023-01-27T18:07:53Z

Thanks, I will take a look

JackCaoG

can you dump HLO when it crashed? Then it should obvious why it failed.

vanbasten23 · 2023-01-31T01:25:20Z

can you dump HLO when it crashed? Then it should obvious why it failed.

When I do debug_flag save_hlo python pytorch/xla/test/test_dynamic_shapes.py TestDynamicShapes.test_empty_symint or debug_flag save_ir python pytorch/xla/test/test_dynamic_shapes.py TestDynamicShapes.test_empty_symint, neither the hlo dump or ir dump showed up. (fyi I have alias save_hlo="XLA_SAVE_TENSORS_FMT='hlo' XLA_SAVE_TENSORS_FILE='/tmp/save1.hlo'" in my bashrc). I have my command output but I don't find much hint on why the hlo dump was not generated. Any idea on how to debug this issue?
To unblock myself, I ended up doing print(torch_xla._XLAC._get_xla_tensors_text([t2])) and print(torch_xla._XLAC._get_xla_tensors_hlo([t2])). I was able to get the dump this way. Here is what I found: if I comment out this line https://github.com/pytorch/xla/pull/4524/files#diff-036cb21341ff8e390cc250e74fe9e3f0f15f259ea4bec4abcce49d95febf1553R154 print('t1=', t1), the test would actually succeed (here is the ir and hlo dump https://paste.googleplex.com/6398046270128128). On the other hand, if I execute the above line, the test would crash with error with ir/hlo dump.

Summary: This pull request tries to fix the following two tests: 1. test_allgather: allgather uses copy_ internally to update the input and copy_ being decomposed into copy which we need to lower. 2. test_send: send uses t.data to update t’s data which is not supported in functionalization. See functorch.functionalize doesn't allow t.data update pytorch#91093. After switching to copy_, it looks like the problem is fixed. Test Plan: CI.

Summary: test_logcumsumexp fails because the shape check on logcumsumexp.out is not executed after functionalized decomposition. As disccused in pytorch/pytorch#92912, this will be hard to fix. Since we never lower logcumsumexp, our users are highly likely not going to use it. Test Plan: CI.

This reverts commit 4ed2ef6.

…4526) Summary: According to pytorch/pytorch#91789, this test is added for the cpu device. Let's properly skip it. Test Plan: CI

Summary: The dynamic shape tests are broken because of upstream changes that decompose dynamic ops into different ways. Adds new lowering to fix that. Corresponding upstream changes for SymBool: pytorch/pytorch#92149. Test Plan: CI

alanwaketan

Generally LGTM. Let's wait for the CI results and the answers to the crash.

fix linter fix build fix build and linter

JackCaoG · 2023-01-31T23:46:45Z

Is test passing with the new symint change?

vanbasten23 · 2023-02-01T00:52:01Z

Is test passing with the new symint change?

The PR is intended to fix the test TestDynamicShapeModels.test_forward_pass_dynamic_input_correctness. Now the original error is gone and it failed with error XLASymNodeImpl::sub has not been implemented. (https://paste.googleplex.com/6638495719227392)

JackCaoG

Mostly lgtm. My general comments is whether we can just merge this pr to the master and let functionization branch to rebase. This way we don't need to block this change that is also required by head for months.

vanbasten23 · 2023-02-01T17:53:44Z

Mostly lgtm. My general comments is whether we can just merge this pr to the master and let functionization branch to rebase. This way we don't need to block this change that is also required by head for months.

Agreed. What's the best way to do that at this point? I was planning to cherry pick this commit to the master branch after this pr is merged.

JackCaoG · 2023-02-01T18:41:15Z

Mostly lgtm. My general comments is whether we can just merge this pr to the master and let functionization branch to rebase. This way we don't need to block this change that is also required by head for months.

Agreed. What's the best way to do that at this point? I was planning to cherry pick this commit to the master branch after this pr is merged.

You can just create a pr with most of the independent bits of this pr and merge it to master. Ask @wonjoolee95 to rebase then hopefully all test will pass.

vanbasten23 · 2023-02-01T20:44:05Z

You can just create a pr with most of the independent bits of this pr and merge it to master. Ask @wonjoolee95 to rebase then hopefully all test will pass.

done #4550. I'll mark the current pr draft again

vanbasten23 · 2023-02-06T18:19:22Z

Closing this. There's another identical PR that has been merged into master branch.

vanbasten23 changed the base branch from master to functionalization January 27, 2023 05:54

JackCaoG reviewed Jan 28, 2023

View reviewed changes

Comment thread torch_xla/csrc/aten_xla_type.cpp Resolved

JackCaoG reviewed Jan 28, 2023

View reviewed changes

Comment thread torch_xla/csrc/tensor_methods.cpp Resolved

JackCaoG reviewed Jan 28, 2023

View reviewed changes

vanbasten23 force-pushed the fixEmptySymint branch from f2b71da to 66ef0d6 Compare January 31, 2023 05:45

vanbasten23 commented Jan 31, 2023

View reviewed changes

Comment thread torch_xla/csrc/xla_graph_executor.cpp Resolved

vanbasten23 requested review from alanwaketan and miladm January 31, 2023 17:39

bdhirsh and others added 20 commits January 31, 2023 18:41

POC of functionalization integration

ba2b741

Add debugging print lines

b01ba30

Run linter

0a5607a

Run linter again

11b8799

Update torch pin

25ac28b

Add more debugging lines

c476227

Lower lift and lift_copy ops

e8f3d10

Update XLATensor namespace calls to tensor_methods

5794c08

Update pytorch_test_base.py

25fc75a

Disable failing tests for now

46638e2

Update lift_fresh ops

41ff5e1

PoC for detach_copy

aa2f754

Refine the comment

f0ef414

POC of functionalization integration

4656d14

Add debugging print lines

ac262aa

Run linter

b9b7296

Run linter again

4f235fe

Update torch pin

94f5b23

Add more debugging lines

7934d8a

Update XLATensor namespace calls to tensor_methods

058ba12

alanwaketan added 8 commits January 31, 2023 18:41

Skip test_exponential

03d6309

Turn on keep going

83494e2

Skip dynamic shape tests again

642de4b

Revert "Turn on keep going"

2b8472e

This reverts commit 4ed2ef6.

[Functionalization] Mark test_narrow_copy_non_contiguous won't fix (#…

ff40bc8

…4526) Summary: According to pytorch/pytorch#91789, this test is added for the cpu device. Let's properly skip it. Test Plan: CI

wonjoo-wj force-pushed the functionalization branch from 6cce4b6 to 15c695c Compare January 31, 2023 18:41

vanbasten23 force-pushed the fixEmptySymint branch from 464532c to 6570bde Compare January 31, 2023 18:51

alanwaketan reviewed Jan 31, 2023

View reviewed changes

Comment thread test/test_dynamic_shapes.py Resolved

Comment thread torch_xla/csrc/xla_graph_executor.cpp Resolved

Comment thread torch_xla/csrc/xla_op_builder.cpp Resolved

Make empty_symint support dynamism.

d226a62

fix linter fix build fix build and linter

vanbasten23 force-pushed the fixEmptySymint branch from 46bcd8d to d226a62 Compare January 31, 2023 22:31

JackCaoG self-requested a review January 31, 2023 23:47

JackCaoG reviewed Feb 1, 2023

View reviewed changes

fix linter

1a2d6b6

vanbasten23 marked this pull request as ready for review February 1, 2023 17:56

vanbasten23 marked this pull request as draft February 1, 2023 20:44

vanbasten23 mentioned this pull request Feb 1, 2023

Make empty_symint support dynamism. #4550

Merged

wonjoo-wj force-pushed the functionalization branch 2 times, most recently from 1c75e4c to ae08eb5 Compare February 2, 2023 18:21

vanbasten23 force-pushed the functionalization branch 2 times, most recently from 974c3a9 to caccc62 Compare February 3, 2023 19:26

vanbasten23 closed this Feb 6, 2023

Navigation Menu

Repository navigation

Make empty_symint support dynamism - #4524

Make empty_symint support dynamism#4524
vanbasten23 wants to merge 104 commits into
functionalizationfrom
fixEmptySymint

vanbasten23 commented Jan 27, 2023

JackCaoG commented Jan 27, 2023

Uh oh!

Uh oh!

JackCaoG left a comment

vanbasten23 commented Jan 31, 2023

Uh oh!

alanwaketan left a comment

Uh oh!

Uh oh!

Uh oh!

JackCaoG commented Jan 31, 2023

vanbasten23 commented Feb 1, 2023

JackCaoG left a comment

vanbasten23 commented Feb 1, 2023

JackCaoG commented Feb 1, 2023

vanbasten23 commented Feb 1, 2023

vanbasten23 commented Feb 6, 2023

Uh oh!

Labels

5 participants

Conversation

vanbasten23 commented Jan 27, 2023

JackCaoG commented Jan 27, 2023

Uh oh!

Uh oh!

JackCaoG left a comment

Choose a reason for hiding this comment

vanbasten23 commented Jan 31, 2023

Uh oh!

alanwaketan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JackCaoG commented Jan 31, 2023

vanbasten23 commented Feb 1, 2023

JackCaoG left a comment

Choose a reason for hiding this comment

vanbasten23 commented Feb 1, 2023

JackCaoG commented Feb 1, 2023

vanbasten23 commented Feb 1, 2023

vanbasten23 commented Feb 6, 2023

Uh oh!

Labels

5 participants