add batch impl. for inplace `index_add` operation #112276

guilhermeleobas · 2023-10-27T18:51:43Z

Stack from ghstack (oldest at bottom):

-> add batch impl. for inplace index_add operation #112276

cc @zou3519 @Chillee @samdow @kshitij12345 @janeyx99

[ghstack-poisoned]

pytorch-bot · 2023-10-27T18:51:47Z

🔗 Helpful Links

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

📄 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 the bot commands wiki or our office hours

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

✅ You can merge normally! (2 Unrelated Failures)

As of commit e9bd676 with merge base c120e56 ():

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

ghstack-source-id: a845ffe9edc81ea23ffc7ac99fc1dd2527b6e7ec Pull Request resolved: #112276

guilhermeleobas · 2023-10-27T19:27:19Z

ref: #105539

[ghstack-poisoned]

ghstack-source-id: 5c48a2db610752992519c4f1bd97d571bf0dffb1 Pull Request resolved: #112276

kshitij12345

Looks good but have couple of questions. Thank you!

kshitij12345 · 2023-10-30T15:01:40Z

test/functorch/test_ops.py

@@ -956,6 +955,7 @@ def vjp_of_vjp(*args_and_cotangents):
             {torch.float32: tol(atol=5e-04, rtol=1e-04)}, device_type="cuda"),
    ))
    @skipOps('TestOperators', 'test_vmapvjp', vmapvjp_fail.union({
+        xfail('as_strided'),


We are just moving above xfail here? Is that correct?

Yes, that's correct. Before the xfail was in vmapvjp_fail, which is used in other places of this file. With the addition of index_add, there is a batch rule for as_strided, but it fails on some specific cases.

kshitij12345 · 2023-10-30T15:05:48Z

aten/src/ATen/functorch/BatchRulesScatterOps.cpp

@@ -977,11 +991,40 @@ std::tuple<Tensor,optional<int64_t>> index_add_batch_rule(
      other.select(*other_bdim, i) : other;
    const auto& index_slice = index_bdim.has_value() ?
      index.select(*index_bdim, i) : index;
-    results.push_back(at::index_add(self_slice, dim, index_slice, other_slice, alpha));
+
+    if (inplace) {


Is there a simple way to not initialize results for inplace case (as it is not used then).

Or maybe we should guard call to reserve with if (!inplace) { results.reserver(batch_size)}

Wdyt?

Done! I've guarded the initialization as you suggested.

cc zou3519 Chillee samdow kshitij12345 janeyx99 [ghstack-poisoned]

ghstack-source-id: 426b5d4e54220922af1d693c77eefa2d3a450336 Pull Request resolved: #112276

kshitij12345

LGTM, thanks @guilhermeleobas

guilhermeleobas · 2023-10-30T21:00:41Z

@pytorchbot merge

pytorchmergebot · 2023-10-30T21:02:33Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

PaliC · 2023-10-31T05:08:58Z

@pytorchbot revert -m "breaking linux binary builds" -c "nosignal"

You can find the breakage here: https://hud.pytorch.org/pytorch/pytorch/commit/e3c8c63deaf594699d827e84869a3ecd7e2ab494

pytorchmergebot · 2023-10-31T05:10:44Z

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

pytorchmergebot · 2023-10-31T05:10:52Z

@guilhermeleobas your PR has been successfully reverted.

This reverts commit e3c8c63. Reverted #112276 on behalf of https://github.com/PaliC due to breaking linux binary builds ([comment](#112276 (comment)))

guilhermeleobas · 2023-10-31T13:39:57Z

Hi @PaliC, failure doesn't seem to be related to the changes I did in this PR:

...
2023-10-31T04:53:40.4122964Z ++ echo 'Copied /usr/lib64/libgomp.so.1 to torch/lib/libgomp-a34b3233.so.1'
2023-10-31T04:53:40.4123779Z Copied /usr/lib64/libgomp.so.1 to torch/lib/libgomp-a34b3233.so.1
2023-10-31T04:53:40.4124389Z ++ for filepath in '"${DEPS_LIST[@]}"'
2023-10-31T04:53:40.4124860Z +++ basename /usr/local/cuda/lib64/libcusparseLt.so.0
2023-10-31T04:53:40.4133181Z ++ filename=libcusparseLt.so.0
2023-10-31T04:53:40.4133594Z ++ destpath=torch/lib/libcusparseLt.so.0
2023-10-31T04:53:40.4134414Z ++ [[ /usr/local/cuda/lib64/libcusparseLt.so.0 != \t\o\r\c\h\/\l\i\b\/\l\i\b\c\u\s\p\a\r\s\e\L\t\.\s\o\.\0 ]]
2023-10-31T04:53:40.4135402Z ++ cp /usr/local/cuda/lib64/libcusparseLt.so.0 torch/lib/libcusparseLt.so.0
2023-10-31T04:53:40.4143847Z cp: cannot stat ‘/usr/local/cuda/lib64/libcusparseLt.so.0’: No such file or directory

Can I just rebase and merge it again?

malfet · 2023-10-31T13:45:52Z

@pytorchbot merge -f "Revert was in error, sorry about that"

malfet · 2023-10-31T13:47:26Z

@pytorchbot revert -m "breaking linux binary builds" -c "nosignal"

You can find the breakage here: https://hud.pytorch.org/pytorch/pytorch/commit/e3c8c63deaf594699d827e84869a3ecd7e2ab494

I think failure is unrelated, as revert clearly did not help, cause of regression is pytorch/builder@7790132 that modified docker images for main, even though it should have targeted only 2.1

Also, nosignal is wrong classification here, as those builds were triggered at the time of merge, see https://github.com/pytorch/pytorch/actions/runs/6698655783/job/18201217605

pytorchmergebot · 2023-10-31T13:47:38Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345

…2276)" This reverts commit e3c8c63. Reverted pytorch#112276 on behalf of https://github.com/PaliC due to breaking linux binary builds ([comment](pytorch#112276 (comment)))

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345, https://github.com/malfet

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345

…2276)" This reverts commit e3c8c63. Reverted pytorch#112276 on behalf of https://github.com/PaliC due to breaking linux binary builds ([comment](pytorch#112276 (comment)))

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345, https://github.com/malfet

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345

…2276)" This reverts commit e3c8c63. Reverted pytorch#112276 on behalf of https://github.com/PaliC due to breaking linux binary builds ([comment](pytorch#112276 (comment)))

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345, https://github.com/malfet

add batch impl. for inplace index_add operation

d4f5480

[ghstack-poisoned]

guilhermeleobas added a commit that referenced this pull request Oct 27, 2023

add batch impl. for inplace index_add operation

d41b26b

ghstack-source-id: a845ffe9edc81ea23ffc7ac99fc1dd2527b6e7ec Pull Request resolved: #112276

pytorchbot added the open source label Oct 27, 2023

Update on "add batch impl. for inplace index_add operation"

12bb813

[ghstack-poisoned]

guilhermeleobas added a commit that referenced this pull request Oct 27, 2023

add batch impl. for inplace index_add operation

dae3199

ghstack-source-id: 5c48a2db610752992519c4f1bd97d571bf0dffb1 Pull Request resolved: #112276

guilhermeleobas self-assigned this Oct 28, 2023

guilhermeleobas marked this pull request as ready for review October 28, 2023 18:06

guilhermeleobas requested review from zou3519, Chillee and kshitij12345 as code owners October 28, 2023 18:06

guilhermeleobas added module: functorch Pertaining to torch.func or pytorch/functorch release notes: functorch release notes category; Pertaining to torch.func or pytorch/functorch labels Oct 30, 2023

kshitij12345 reviewed Oct 30, 2023

View reviewed changes

Update on "add batch impl. for inplace index_add operation"

e9bd676

cc zou3519 Chillee samdow kshitij12345 janeyx99 [ghstack-poisoned]

guilhermeleobas added a commit that referenced this pull request Oct 30, 2023

add batch impl. for inplace index_add operation

a561865

ghstack-source-id: 426b5d4e54220922af1d693c77eefa2d3a450336 Pull Request resolved: #112276

zou3519 approved these changes Oct 30, 2023

View reviewed changes

kshitij12345 approved these changes Oct 30, 2023

View reviewed changes

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

pytorchmergebot added the merging label Oct 30, 2023

pytorchmergebot added Merged and removed merging labels Oct 31, 2023

pytorchmergebot closed this in e3c8c63 Oct 31, 2023

pytorchmergebot added the Reverted label Oct 31, 2023

guilhermeleobas reopened this Oct 31, 2023

malfet approved these changes Oct 31, 2023

View reviewed changes

pytorchmergebot added the merging label Oct 31, 2023

pytorchmergebot removed the merging label Oct 31, 2023

pytorchmergebot closed this in 86196bf Oct 31, 2023

kit1980 removed the Reverted label Nov 1, 2023

guilhermeleobas mentioned this pull request Nov 2, 2023

UserWarning: There is a performance drop because we have not yet implemented the batching rule for aten::index_add_. Please file us an issue on GitHub so that we can prioritize its implementation. #105539

Closed

facebook-github-bot deleted the gh/guilhermeleobas/9/head branch November 3, 2023 14:27

xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Nov 7, 2023

add batch impl. for inplace index_add operation (pytorch#112276)

ac26cfd

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345

Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023

add batch impl. for inplace index_add operation (pytorch#112276)

394ffa8

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345

andreigh pushed a commit to andreigh/pytorch that referenced this pull request Nov 19, 2023

add batch impl. for inplace index_add operation (pytorch#112276)

c327bc4

Pull Request resolved: pytorch#112276 Approved by: https://github.com/zou3519, https://github.com/kshitij12345

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add batch impl. for inplace `index_add` operation #112276

add batch impl. for inplace `index_add` operation #112276

guilhermeleobas commented Oct 27, 2023 •

edited by pytorch-bot bot

pytorch-bot bot commented Oct 27, 2023 •

edited

guilhermeleobas commented Oct 27, 2023

kshitij12345 left a comment

kshitij12345 Oct 30, 2023

guilhermeleobas Oct 30, 2023

kshitij12345 Oct 30, 2023

guilhermeleobas Oct 30, 2023

kshitij12345 left a comment

guilhermeleobas commented Oct 30, 2023

pytorchmergebot commented Oct 30, 2023

PaliC commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

guilhermeleobas commented Oct 31, 2023

malfet commented Oct 31, 2023

malfet commented Oct 31, 2023 •

edited

pytorchmergebot commented Oct 31, 2023

add batch impl. for inplace index_add operation #112276

add batch impl. for inplace index_add operation #112276

Conversation

guilhermeleobas commented Oct 27, 2023 • edited by pytorch-bot bot

pytorch-bot bot commented Oct 27, 2023 • edited

🔗 Helpful Links

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

✅ You can merge normally! (2 Unrelated Failures)

guilhermeleobas commented Oct 27, 2023

kshitij12345 left a comment

Choose a reason for hiding this comment

kshitij12345 Oct 30, 2023

Choose a reason for hiding this comment

guilhermeleobas Oct 30, 2023

Choose a reason for hiding this comment

kshitij12345 Oct 30, 2023

Choose a reason for hiding this comment

guilhermeleobas Oct 30, 2023

Choose a reason for hiding this comment

kshitij12345 left a comment

Choose a reason for hiding this comment

guilhermeleobas commented Oct 30, 2023

pytorchmergebot commented Oct 30, 2023

Merge started

PaliC commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

guilhermeleobas commented Oct 31, 2023

malfet commented Oct 31, 2023

malfet commented Oct 31, 2023 • edited

pytorchmergebot commented Oct 31, 2023

Merge started

add batch impl. for inplace `index_add` operation #112276

add batch impl. for inplace `index_add` operation #112276

guilhermeleobas commented Oct 27, 2023 •

edited by pytorch-bot bot

pytorch-bot bot commented Oct 27, 2023 •

edited

malfet commented Oct 31, 2023 •

edited