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

[NJT] Allow construction of NJT within graph using offsets from inputs #124624

Draft
wants to merge 4 commits into
base: gh/soulitzer/296/base
Choose a base branch
from

Conversation

soulitzer
Copy link
Contributor

@soulitzer soulitzer commented Apr 22, 2024

Stack from ghstack (oldest at bottom):

Creating symbolic nested ints within the graph is difficult. Using unbacked symints should solve the most important(?) cases in the mean time.

See #118446

Known gaps:

  • creating NJT from intermediate offsets (offsets created within the graph, as opposed to being offsets passed in as inputs)
  • when the same offsets is also passed in as a input to the graph. We are not smart enough to realize that the offsets from that input is the same and therefore would fail when the sizes are compare ("s0 cannot be compared with u0")

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang

Copy link

pytorch-bot bot commented Apr 22, 2024

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit 044eaaf with merge base bad8d25 (image):

NEW FAILURE - The following job has failed:

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

soulitzer added a commit that referenced this pull request Apr 22, 2024
ghstack-source-id: ca8687cc9d878fe0cac88f12544622c7f6d3b879
Pull Request resolved: #124624
@soulitzer soulitzer marked this pull request as draft April 22, 2024 16:21
… from inputs"


Creating symbolic nested ints within the graph is difficult. Using unbacked symints should solve the most important(?) cases in the mean time.

See #118446

Known gaps:
- creating NJT from intermediate offsets (offsets created within the graph, as opposed to being offsets passed in as inputs)
- when the same offsets is also passed in as a input to the graph. We are not smart enough to realize that the offsets from that input is the same and therefore would fail when  the sizes are compare ("s0 cannot be compared with u0")



cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang

[ghstack-poisoned]
… from inputs"


Creating symbolic nested ints within the graph is difficult. Using unbacked symints should solve the most important(?) cases in the mean time.

See #118446

Known gaps:
- creating NJT from intermediate offsets (offsets created within the graph, as opposed to being offsets passed in as inputs)
- when the same offsets is also passed in as a input to the graph. We are not smart enough to realize that the offsets from that input is the same and therefore would fail when  the sizes are compare ("s0 cannot be compared with u0")



cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang

[ghstack-poisoned]
soulitzer added a commit that referenced this pull request Apr 22, 2024
ghstack-source-id: 19ce77c0fc0f385f2623f297167e8d5821aad968
Pull Request resolved: #124624
… from inputs"


Creating symbolic nested ints within the graph is difficult. Using unbacked symints should solve the most important(?) cases in the mean time.

See #118446

Known gaps:
- creating NJT from intermediate offsets (offsets created within the graph, as opposed to being offsets passed in as inputs)
- when the same offsets is also passed in as a input to the graph. We are not smart enough to realize that the offsets from that input is the same and therefore would fail when  the sizes are compare ("s0 cannot be compared with u0")



cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang

[ghstack-poisoned]
soulitzer added a commit that referenced this pull request Apr 22, 2024
ghstack-source-id: b438d136f6dffe865e4c366072ffabdaed5cfa2a
Pull Request resolved: #124624
Comment on lines +23 to +26
elif isinstance(tensor, FakeTensor):
shape_env = tensor.fake_mode.shape_env
tensor_symint = shape_env.create_unbacked_symint()
# Do we need to constrain as size?
Copy link
Contributor

Choose a reason for hiding this comment

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

why would this path ever be hit if we manually update the registry for fake tensors during tensor_unflatten?

@davidberard98
Copy link
Contributor

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Rebase failed due to Command git -C /home/runner/work/pytorch/pytorch rebase refs/remotes/origin/viable/strict gh/soulitzer/296/orig returned non-zero exit code 1

warning: skipped previously applied commit 20048a0e48c
warning: skipped previously applied commit b5400b8fd89
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
Rebasing (1/1)
Auto-merging test/dynamo/test_subclasses.py
Auto-merging test/test_fake_tensor.py
CONFLICT (content): Merge conflict in test/test_fake_tensor.py
Auto-merging test/test_nestedtensor.py
Auto-merging torch/nested/_internal/nested_tensor.py
error: could not apply 0ac80a38671... [NJT] Allow construction of NJT within graph using offsets from inputs
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Could not apply 0ac80a38671... [NJT] Allow construction of NJT within graph using offsets from inputs

Raised by https://github.com/pytorch/pytorch/actions/runs/9274613146

davidberard98 added a commit that referenced this pull request Jun 8, 2024
We run into an error when we try to construct a NT in the graph and return it. One of the errors is with symint handling in is_concrete_int: the full error this PR is here: https://gist.github.com/davidberard98/877a52f6ea57025cc122d64361e598da. Repro: run the test_nestedtensor.py test after reverting the patch in symbolic_shapes.py.

Note - a separate issue occurs if we don't call fn() before fn_c(), where the tensors aren't marked as dynamic; fix for this is in #124624

[ghstack-poisoned]
davidberard98 added a commit that referenced this pull request Jun 8, 2024
We run into an error when we try to construct a NT in the graph and return it. One of the errors is with symint handling in is_concrete_int: the full error this PR is here: https://gist.github.com/davidberard98/877a52f6ea57025cc122d64361e598da. Repro: run the test_nestedtensor.py test after reverting the patch in symbolic_shapes.py.

Note - a separate issue occurs if we don't call fn() before fn_c(), where the tensors aren't marked as dynamic; fix for this is in #124624

[ghstack-poisoned]
davidberard98 added a commit that referenced this pull request Jun 26, 2024
We run into an error when we try to construct a NT in the graph and return it. One of the errors is with symint handling in is_concrete_int: the full error this PR is here: https://gist.github.com/davidberard98/877a52f6ea57025cc122d64361e598da. Repro: run the test_nestedtensor.py test after reverting the patch in symbolic_shapes.py.

Note - a separate issue occurs if we don't call fn() before fn_c(), where the tensors aren't marked as dynamic; fix for this is in #124624

Differential Revision: [D58315861](https://our.internmc.facebook.com/intern/diff/D58315861)

[ghstack-poisoned]
davidberard98 added a commit that referenced this pull request Jun 26, 2024
We run into an error when we try to construct a NT in the graph and return it. One of the errors is with symint handling in is_concrete_int: the full error this PR is here: https://gist.github.com/davidberard98/877a52f6ea57025cc122d64361e598da. Repro: run the test_nestedtensor.py test after reverting the patch in symbolic_shapes.py.

Note - a separate issue occurs if we don't call fn() before fn_c(), where the tensors aren't marked as dynamic; fix for this is in #124624

Differential Revision: [D58315861](https://our.internmc.facebook.com/intern/diff/D58315861)

[ghstack-poisoned]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants