Add support for directly passing symint to empty by ezyang · Pull Request #79494 · pytorch/pytorch

ezyang · 2022-06-14T02:25:19Z

Stack from ghstack (oldest at bottom):

Signed-off-by: Edward Z. Yang ezyang@fb.com

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

Signed-off-by: Edward Z. Yang <ezyangfb.com> ghstack-source-id: 773c244 Pull Request resolved: #79494

facebook-github-bot · 2022-06-14T02:25:30Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/79494
📄 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 our office hours

✅ No Failures (0 Pending)

As of commit 64374ac (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]

Signed-off-by: Edward Z. Yang <ezyangfb.com> ghstack-source-id: cc943e1 Pull Request resolved: #79494

albanD · 2022-06-14T13:50:09Z

  // TODO: technically we need to check that the classes match
  if (!a) {
    a = common->wrap(data_);
+    toSymInt(a); //


debug code?

toSymInt has a side-effect of persisting SymIntNodes into a SymIntTable, so a comment would probably help why we need to do this given that SymInt::toSymInt(a->add(b)); should have strong references to both a and b?

This code is completely broken anyway; every time you convert SymbolicIntNode to SymInt the current code allocates a fresh table entry which is wrong

albanD · 2022-06-14T13:51:26Z

+) {
+  auto device = device_or_default(device_opt);
+  TORCH_INTERNAL_ASSERT_DEBUG_ONLY(device.type() == DeviceType::Meta);
+  // NB: because there is no SparseMeta (yet), non-strided layout is


nit missing "not"

albanD · 2022-06-14T13:55:54Z

+  std::vector<SymInt> strides;
+  strides.resize(dim);
+
+  // TODO: Move this into TensorImpl


Isn't that already? You have the empty_tensor_restride(MemoryFormat::Contiguous) that does this.

nit: it does feel like we need a helper here.

empty_tensor_restride doesn't operate on symbolic integers. I'm a little torn about whether or not to directly SymInt'ify it or maintain two separate versions; there's a lot of arithmetic operations going on and it would be better for them to be all unchecked

albanD · 2022-06-14T13:56:40Z

    SparseCsrCPU, SparseCsrCUDA: empty_sparse_compressed
    QuantizedCPU, QuantizedCUDA: empty_unknown_quantized

+- func: empty.SymInt(SymInt[] size, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None, MemoryFormat? memory_format=None) -> Tensor


No overload conflict for this one?

No, because of #79281

Ho right this is done under the hood.

Krovatkin

Krovatkin · 2022-06-14T15:52:14Z

+  std::vector<SymInt> strides;
+  strides.resize(dim);
+
+  // TODO: Move this into TensorImpl


nit: it does feel like we need a helper here.

Krovatkin · 2022-06-14T15:56:28Z

  // TODO: technically we need to check that the classes match
  if (!a) {
    a = common->wrap(data_);
+    toSymInt(a); //


toSymInt has a side-effect of persisting SymIntNodes into a SymIntTable, so a comment would probably help why we need to do this given that SymInt::toSymInt(a->add(b)); should have strong references to both a and b?

albanD

Will need some cleanup, but sounds ok.

ezyang · 2022-06-14T20:32:58Z

@pytorchbot merge

pytorchmergebot · 2022-06-14T20:34:13Z

@pytorchbot successfully started a merge job. Check the current status here

github-actions · 2022-06-14T20:34:57Z

Hey @ezyang.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

ezyang · 2022-06-15T13:44:17Z

@pytorchbot revert

pytorch-bot · 2022-06-15T13:44:18Z

❌ 🤖 pytorchbot command failed:

@pytorchbot revert: error: the following arguments are required: -m/--message

usage: @pytorchbot revert -m MESSAGE
                          [-c {nosignal,ignoredsignal,landrace,weird,ghfirst}]

Try @pytorchbot help for more info.

ezyang · 2022-06-15T13:44:57Z

@pytorchbot revert -m "conflicts with earlier diff that needs revert"

pytorchmergebot · 2022-06-15T13:46:16Z

@pytorchbot successfully started a revert job. Check the current status here

pytorchmergebot · 2022-06-15T13:46:16Z

Reverting PR 79494 failed due to Comment @pytorchbot revert -m "conflicts with earlier diff that needs revert" does not seem to be a valid revert command
Raised by https://github.com/pytorch/pytorch/actions/runs/2502654093

ezyang · 2022-06-15T13:48:43Z

@pytorchbot revert -m "conflicts with earlier diff that needs revert" -c weird

pytorchmergebot · 2022-06-15T13:49:53Z

@pytorchbot successfully started a revert job. Check the current status here

This reverts commit 05664a9. Reverted #79494 on behalf of https://github.com/ezyang due to conflicts with earlier diff that needs revert

Signed-off-by: Edward Z. Yang <ezyangfb.com> Pull Request resolved: pytorch#79494 Approved by: https://github.com/albanD

This reverts commit 3d96708. Reverted pytorch#79494 on behalf of https://github.com/ezyang due to conflicts with earlier diff that needs revert

Add support for directly passing symint to empty

285afe0

Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

ezyang requested a review from bdhirsh as a code owner June 14, 2022 02:25

This was referenced Jun 14, 2022

Symbolic storage size #79492

Closed

Add support for multiply on direct SymInt #79493

Closed

ezyang added a commit that referenced this pull request Jun 14, 2022

Add support for directly passing symint to empty

afed6e7

Signed-off-by: Edward Z. Yang <ezyangfb.com> ghstack-source-id: 773c244 Pull Request resolved: #79494

facebook-github-bot added the cla signed label Jun 14, 2022

Update on "Add support for directly passing symint to empty"

73791e8

Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]

ezyang requested review from Chillee, Krovatkin and eellison June 14, 2022 13:24

Update on "Add support for directly passing symint to empty"

64374ac

Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]

ezyang added a commit that referenced this pull request Jun 14, 2022

Add support for directly passing symint to empty

9c21d11

Signed-off-by: Edward Z. Yang <ezyangfb.com> ghstack-source-id: cc943e1 Pull Request resolved: #79494

albanD reviewed Jun 14, 2022

View reviewed changes

Krovatkin approved these changes Jun 14, 2022

View reviewed changes

albanD approved these changes Jun 14, 2022

View reviewed changes

pytorchmergebot added the Merged label Jun 14, 2022

pytorchmergebot closed this in 05664a9 Jun 14, 2022

pytorchmergebot added the Reverted label Jun 15, 2022

pytorchmergebot added a commit that referenced this pull request Jun 15, 2022

Revert "Add support for directly passing symint to empty"

aa9d25e

This reverts commit 05664a9. Reverted #79494 on behalf of https://github.com/ezyang due to conflicts with earlier diff that needs revert

janeyx99 mentioned this pull request Jun 15, 2022

Make our help message consistent with revert behavior pytorch/test-infra#416

Closed

facebook-github-bot deleted the gh/ezyang/1221/head branch June 18, 2022 14:17

zengk95 mentioned this pull request Jun 22, 2022

[Meta] CI Revert Tracker #66178

Closed

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 25, 2026

Add support for directly passing symint to empty

3d96708

Signed-off-by: Edward Z. Yang <ezyangfb.com> Pull Request resolved: pytorch#79494 Approved by: https://github.com/albanD

Conversation

ezyang commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krovatkin Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krovatkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krovatkin Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Jun 14, 2022

Uh oh!

pytorchmergebot commented Jun 14, 2022

Uh oh!

github-actions Bot commented Jun 14, 2022

Uh oh!

ezyang commented Jun 15, 2022

Uh oh!

pytorch-bot Bot commented Jun 15, 2022

Uh oh!

ezyang commented Jun 15, 2022

Uh oh!

pytorchmergebot commented Jun 15, 2022

Uh oh!

pytorchmergebot commented Jun 15, 2022

Uh oh!

ezyang commented Jun 15, 2022

Uh oh!

pytorchmergebot commented Jun 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ezyang commented Jun 14, 2022 •

edited

Loading

facebook-github-bot commented Jun 14, 2022 •

edited

Loading

Krovatkin Jun 14, 2022 •

edited

Loading

Krovatkin Jun 14, 2022 •

edited

Loading