Symintify getitem and add the required helper functions by albanD · Pull Request #86207 · pytorch/pytorch

albanD · 2022-10-04T16:04:00Z

Note that this might not cover every use of the function (we know it doesn't)
But this is enough to get few models passing.

pytorch-bot · 2022-10-04T16:04:04Z

🔗 Helpful Links

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

📄 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

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

❌ 1 Failures

As of commit 4afd107:

The following jobs have failed:

macos-12-py3-x86-64 / build

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

albanD · 2022-10-04T16:04:24Z

aten/src/ATen/TensorIndexing.h

        real_dim,
        " with size ",
-        size);
+        size.guard_int(__FILE__, __LINE__));


@ezyang The guard here should be fine as the error message is never evaluated right?

Yes, is fine. But cc @Chillee was talking about making it so we always print the concrete values when printing symints, which would have made direct size print ok too.

albanD · 2022-10-05T14:58:02Z

test/test_autograd.py

+        out = nn.Conv2d(1, 1, 3)(a)
+        self.assertEqual(out.grad_fn._saved_bias_sym_sizes_opt, (1,))     # c10::optional<SymIntArrayRef> -> SymInt[]?
+        out = nn.Conv2d(1, 1, 3, bias=False)(a)
+        # TODO: This is BAD! we converted a c10::nullopt into a (0,)


I don't know how to fix that one off the top of my head and this is already a big PR. Will fix in a follow up.

👀 is this not caught by opinfos? (only nn.Conv2d)

Oh or maybe it already is and we have them xfail'd

OpInfo don't look at the python bindings for stuff saved in the backward.
These are debug-only APIs so this is not a big deal.

bdhirsh · 2022-10-05T15:01:03Z

aten/src/ATen/native/TensorShape.cpp

-  auto size = self.size(dim);
-  if (index < -size || index >= size) {
+  auto size = self.sym_sizes()[dim];
+  if (size < -index_ || size <= index_) {


Did this condition change?

No it did not. But the SymInt has to be on the left side and cannot have -SymInt. So just moved stuff around :)

should just add enough overloads to SymInt.h so that we don't have to do this...

What is the overload that should be implemented for int64_t < SymInt ?

aten/src/ATen/native/TensorShape.cpp

test/test_autograd.py

aten/src/ATen/TensorIndexing.h

bdhirsh · 2022-10-05T15:10:47Z

torch/_meta_registrations.py

+
+@register_meta(aten.index_put.default, register_dispatcher=False)
+def meta_index_put(self, indices, values, accumulate=False):
+    return self.new_empty(self.size())


torch.empty_like() to follow the same convention we have for most other python meta kernels?

Probably doesn't matter too much.

ezyang · 2022-10-05T17:48:48Z

@pytorchbot merge

pytorchmergebot · 2022-10-05T17:50:13Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered without a flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

pytorchmergebot · 2022-10-05T20:32:23Z

The merge job was canceled. If you believe this is a mistake,then you can re trigger it through pytorch-bot.

albanD · 2022-10-05T20:33:28Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-05T20:34:58Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

pytorchmergebot · 2022-10-05T21:16:34Z

The merge job was canceled. If you believe this is a mistake,then you can re trigger it through pytorch-bot.

albanD · 2022-10-05T21:17:06Z

@pytorchbot merge -f "circle ci is in outage so skipping circle CI functorch tests that are already in the main test"

pytorchmergebot · 2022-10-05T21:18:56Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the force (-f) flag. This means your change will be merged immediately, bypassing any CI checks (ETA: 1-5 minutes). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

bdhirsh

nice :)

albanD · 2022-10-05T23:11:58Z

Actually rebased on top of master and fixed land race issues

albanD · 2022-10-06T00:04:59Z

@pytorchbot merge -g

I'm very optimistic !

pytorchmergebot · 2022-10-06T00:06:22Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

pytorchmergebot · 2022-10-06T02:22:22Z

Merge failed

Reason: 2 additional jobs have failed, first few of them are: trunk ,trunk / macos-12-py3-x86-64 / build

Details for Dev Infra team

Raised by workflow job

ezyang · 2022-10-06T04:44:29Z

@pytorchbot merge -f "infra failure"

pytorchmergebot · 2022-10-06T04:46:15Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the force (-f) flag. This means your change will be merged immediately, bypassing any CI checks (ETA: 1-5 minutes). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

github-actions · 2022-10-06T04:47:05Z

Hey @albanD.
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.

seemethere · 2022-10-07T06:20:52Z

@pytorchbot revert -c ghfirst -m " Fails internal tests, see: https://www.internalfb.com/intern/sandcastle/job/22517998926071860/insights"

seemethere · 2022-10-07T06:21:40Z

Hello! This is failing internal tests on an xplat target like so:

[CONTEXT]     "nctype": lambda name: NamedCType(name, BaseCType(SymIntT)),
[CONTEXT] NameError: name 'SymIntT' is not defined
[CONTEXT]   in native_functions.yaml line /data/sandcastle/boxes/eden-trunk-hg-fb4a-fbsource/buck-out/gen/ee8c4ff0/xplat/caffe2/aten_src_path__/aten_src_path/aten/src/ATen/native/native_functions.yaml:2002:
[CONTEXT]     embedding(Tensor weight, Tensor indices, int padding_idx=-1, bool scale_grad_by_freq=False, bool sparse=False) -> Tensor

From: internalfb.com/intern/sandcastle/job/22517998926071860/insights

pytorchmergebot · 2022-10-07T06:23:18Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2022-10-07T06:23:22Z

Reverting PR 86207 failed

Reason: Command git -C /home/runner/work/pytorch/pytorch revert --no-edit fd5085c445c3f1a4c90e55154cf26fe30f52a0ab returned non-zero exit code 1

Auto-merging aten/src/ATen/native/TensorShape.cpp
Auto-merging functorch/test/test_aotdispatch.py
Auto-merging test/test_autograd.py
Auto-merging test/test_proxy_tensor.py
Auto-merging tools/autograd/derivatives.yaml
Auto-merging tools/autograd/load_derivatives.py
Auto-merging torch/_meta_registrations.py
CONFLICT (content): Merge conflict in torch/_meta_registrations.py
error: could not revert fd5085c445... Symintify getitem and add the required helper functions (#86207)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git revert --continue".
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".

Details for Dev Infra team

Raised by workflow job

symintify split_with_sizes, dropout, fused_fake_obs_quant. meta for padding_2d ops add meta_bernoulli_ meta kernel for at::gather get pytorch_struct to pass: meta for scatter_add, fix backward symintify split ops Pull Request resolved: #86334 Approved by: https://github.com/ezyang

…ch (#86334)" This reverts commit 08e3999. Reverted #86334 on behalf of https://github.com/seemethere due to Trying to revert #86207, this PR causes merge conflicts with the initial revert so will have to revert this as well

seemethere · 2022-10-07T16:05:02Z

@pytorchbot revert -c ghfirst -m " Fails internal tests, see: https://www.internalfb.com/intern/sandcastle/job/22517998926071860/insights"

pytorchmergebot · 2022-10-07T16:10:25Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2022-10-07T16:10:33Z

@albanD your PR has been successfully reverted.

…)" This reverts commit fd5085c. Reverted #86207 on behalf of https://github.com/seemethere due to Fails internal tests, see: https://www.internalfb.com/intern/sandcastle/job/22517998926071860/insights

…86207) Note that this might not cover every use of the function (we know it doesn't) But this is enough to get few models passing. [ghstack-poisoned]

…required helper functions (#86207)" Note that this might not cover every use of the function (we know it doesn't) But this is enough to get few models passing. [ghstack-poisoned]

…functions (#86207)" Note that this might not cover every use of the function (we know it doesn't) But this is enough to get few models passing. [ghstack-poisoned]

…86207) (#86487) Note that this might not cover every use of the function (we know it doesn't) But this is enough to get few models passing. Pull Request resolved: #86487 Approved by: https://github.com/ezyang

…86207) (#86487) (#86487) Summary: Note that this might not cover every use of the function (we know it doesn't) But this is enough to get few models passing. Pull Request resolved: #86487 Approved by: https://github.com/ezyang Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/55663b7f8174db5d71d611403078ebcec4075b1a Reviewed By: DanilBaibak Differential Revision: D40232593 Pulled By: DanilBaibak fbshipit-source-id: 6927cce33b26b86793670eaab0e027e90c54f613

albanD requested a review from ezyang October 4, 2022 16:04

albanD requested a review from soulitzer as a code owner October 4, 2022 16:04

facebook-github-bot added the cla signed label Oct 4, 2022

github-actions bot requested review from Chillee, Krovatkin, anjali411, antoniojkim, miladm and wconstab October 4, 2022 16:04

albanD commented Oct 4, 2022

View reviewed changes

albanD commented Oct 5, 2022

View reviewed changes

bdhirsh reviewed Oct 5, 2022

View reviewed changes

aten/src/ATen/native/TensorShape.cpp Show resolved Hide resolved

bdhirsh reviewed Oct 5, 2022

View reviewed changes

test/test_autograd.py Show resolved Hide resolved

ezyang reviewed Oct 5, 2022

View reviewed changes

aten/src/ATen/TensorIndexing.h Outdated Show resolved Hide resolved

ezyang approved these changes Oct 5, 2022

View reviewed changes

bdhirsh reviewed Oct 5, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 5, 2022

pytorchmergebot added the Merged label Oct 5, 2022

pytorchmergebot closed this in 17addb3 Oct 5, 2022

bdhirsh approved these changes Oct 5, 2022

View reviewed changes

Fix land race issue

7ca8235

albanD force-pushed the getitem_symint branch from 217d209 to 7ca8235 Compare October 5, 2022 23:11

remove xfail

4afd107

pytorchmergebot closed this in fd5085c Oct 6, 2022

seemethere mentioned this pull request Oct 7, 2022

Merge more symbolic meta kernels and symint changes from branch #86334

Closed

github-actions bot deleted the getitem_symint branch April 6, 2024 01:50

Conversation

albanD commented Oct 4, 2022

Uh oh!

pytorch-bot bot commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 1 Failures

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!

bdhirsh Oct 5, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Oct 5, 2022

Uh oh!

pytorchmergebot commented Oct 5, 2022

Uh oh!

pytorchmergebot commented Oct 5, 2022

Uh oh!

albanD commented Oct 5, 2022

Uh oh!

pytorchmergebot commented Oct 5, 2022

Uh oh!

pytorchmergebot commented Oct 5, 2022

Uh oh!

albanD commented Oct 5, 2022

Uh oh!

pytorchmergebot commented Oct 5, 2022

Uh oh!

bdhirsh left a comment

Choose a reason for hiding this comment

Uh oh!

albanD commented Oct 5, 2022

Uh oh!

albanD commented Oct 6, 2022

Uh oh!

pytorchmergebot commented Oct 6, 2022

Uh oh!

pytorchmergebot commented Oct 6, 2022

Merge failed

Uh oh!

ezyang commented Oct 6, 2022

Uh oh!

pytorchmergebot commented Oct 6, 2022

Uh oh!

github-actions bot commented Oct 6, 2022

Uh oh!

seemethere commented Oct 7, 2022

Uh oh!

seemethere commented Oct 7, 2022

Uh oh!

pytorchmergebot commented Oct 7, 2022

Uh oh!

pytorchmergebot commented Oct 7, 2022

Reverting PR 86207 failed

Uh oh!

seemethere commented Oct 7, 2022

Uh oh!

pytorchmergebot commented Oct 7, 2022

pytorch-bot bot commented Oct 4, 2022 •

edited

Loading

bdhirsh Oct 5, 2022 •

edited

Loading