Skip to content

Update foreach APIs to use scalar lists#48223

Closed
izdeby wants to merge 29 commits intogh/izdeby/67/basefrom
gh/izdeby/67/head
Closed

Update foreach APIs to use scalar lists#48223
izdeby wants to merge 29 commits intogh/izdeby/67/basefrom
gh/izdeby/67/head

Conversation

@izdeby
Copy link
Copy Markdown
Contributor

@izdeby izdeby commented Nov 19, 2020 •
edited
Loading

Stack from ghstack:

Differential Revision: D25074763

Motivation
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef

Testing
Update the tests assuming that any scalar type can be passed now, not just double.

This was referenced Nov 19, 2020
Closed
Closed
Closed
@izdeby izdeby changed the title Update foreach APIs to use scalar lists [WIP] Update foreach APIs to use scalar lists Nov 19, 2020
@dr-ci
Copy link
Copy Markdown

dr-ci bot commented Nov 19, 2020 •
edited by facebook-github-bot
Loading

💊 CI failures summary and remediations

As of commit 2a95c20 (more details on the Dr. CI page):

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

See CircleCI build pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test2 (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun) ❄️

Feb 02 21:05:00 RuntimeError: CUDA error: an illegal memory access was encountered 
Feb 02 21:05:00   File "test_optim.py", line 1994, in test_update_bn_dnn
Feb 02 21:05:00     self._test_update_bn(dnn.cuda(), dl_x, dl_xy, True)
Feb 02 21:05:00   File "/opt/conda/lib/python3.6/site-packages/torch/nn/modules/module.py", line 491, in cuda
Feb 02 21:05:00     return self._apply(lambda t: t.cuda(device))
Feb 02 21:05:00   File "/opt/conda/lib/python3.6/site-packages/torch/nn/modules/module.py", line 387, in _apply
Feb 02 21:05:00     module._apply(fn)
Feb 02 21:05:00   File "/opt/conda/lib/python3.6/site-packages/torch/nn/modules/module.py", line 409, in _apply
Feb 02 21:05:00     param_applied = fn(param)
Feb 02 21:05:00   File "/opt/conda/lib/python3.6/site-packages/torch/nn/modules/module.py", line 491, in <lambda>
Feb 02 21:05:00     return self._apply(lambda t: t.cuda(device))
Feb 02 21:05:00 RuntimeError: CUDA error: an illegal memory access was encountered
Feb 02 21:05:00 
Feb 02 21:05:00 ----------------------------------------------------------------------
Feb 02 21:05:00 Ran 104 tests in 38.556s
Feb 02 21:05:00 
Feb 02 21:05:00 FAILED (errors=13)
Feb 02 21:05:00 
Feb 02 21:05:00 Generating XML reports...
Feb 02 21:05:00 Generated XML report: test-reports/dist-gloo/TEST-TestLRScheduler-20210202210421.xml
Feb 02 21:05:00 Generated XML report: test-reports/dist-gloo/TEST-TestOptim-20210202210421.xml
Feb 02 21:05:00 Generated XML report: test-reports/dist-gloo/TEST-TestSWAUtils-20210202210421.xml
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

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

Iurii Zdebskyi added 3 commits December 2, 2020 13:59
Update on "[WIP] Update foreach APIs to use scalar lists"
c84e8a3 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

[ghstack-poisoned]
Update on "[WIP] Update foreach APIs to use scalar lists"
deb56ab 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

[ghstack-poisoned]
Update on "[WIP] Update foreach APIs to use scalar lists"
c4ddf7e 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
@izdeby izdeby changed the title [WIP] Update foreach APIs to use scalar lists Update foreach APIs to use scalar lists Dec 4, 2020
@izdeby izdeby requested review from cpuhrsch, gchanan, mcarilli, ngimel and zou3519 and removed request for mcarilli December 4, 2020 17:47
Update on "Update foreach APIs to use scalar lists"
068559b 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
@izdeby izdeby mentioned this pull request Dec 8, 2020
Closed
Iurii Zdebskyi added 2 commits December 8, 2020 14:51
Update on "Update foreach APIs to use scalar lists"
3048c01 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
Update on "Update foreach APIs to use scalar lists"
5affd8b 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
zou3519
zou3519 reviewed Dec 10, 2020
View reviewed changes
zou3519
zou3519 reviewed Dec 10, 2020
View reviewed changes
zou3519
zou3519 reviewed Dec 10, 2020
View reviewed changes
zou3519
zou3519 reviewed Dec 10, 2020
View reviewed changes
zou3519
zou3519 reviewed Dec 10, 2020
View reviewed changes
test/test_foreach.py
self.assertEqual(res, expected)

foreach_bin_op_(tensors, scalars)
self.assertEqual(res, tensors)
Copy link
Copy Markdown
Contributor

@zou3519 zou3519 Dec 10, 2020

Choose a reason for hiding this comment

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

Some general feedback is: the tests are really hard to read because they branch so much. I'm not sure how to make them easier to read, though. Perhaps we should consider splitting them up or think about why the behavior diverges so much?

Copy link
Copy Markdown
Contributor Author

@izdeby izdeby Dec 10, 2020

Choose a reason for hiding this comment

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

Yes, i fully agree. thats why in the next PR in this stack i separate all the tests. Much easier to read and maintain.

Update on "Update foreach APIs to use scalar lists"
29c073b 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
@izdeby izdeby changed the title Update foreach APIs to use scalar lists [WIP] Update foreach APIs to use scalar lists Dec 10, 2020
Update on "[WIP] Update foreach APIs to use scalar lists"
5fce4c1 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
izdeby and others added 3 commits January 12, 2021 09:02
@izdeby
Update on "Update foreach APIs to use scalar lists"
810af38 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
Update on "Update foreach APIs to use scalar lists"
e812a55 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
Update on "Update foreach APIs to use scalar lists"
5882f8e 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
This was referenced Jan 25, 2021
Closed
Closed
Closed
Update on "Update foreach APIs to use scalar lists"
188e6b5 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
@izdeby izdeby mentioned this pull request Jan 26, 2021
Closed
Iurii Zdebskyi added 2 commits January 26, 2021 14:03
Update on "Update foreach APIs to use scalar lists"
53d164d 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
Update on "Update foreach APIs to use scalar lists"
650d668 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
@izdeby izdeby mentioned this pull request Jan 27, 2021
Closed
Iurii Zdebskyi added 4 commits January 27, 2021 07:18
Update on "Update foreach APIs to use scalar lists"
5a826bc 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
Update on "Update foreach APIs to use scalar lists"
7f0bbaf 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
Update on "Update foreach APIs to use scalar lists"
ee56268 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
Update on "Update foreach APIs to use scalar lists"
2a95c20 
Differential Revision: [D25074763](https://our.internmc.facebook.com/intern/diff/D25074763)

**Motivation**
Update existing _foreach APIs to use ScalarList instead of at::ArrayRef<double>

**Testing**
Update the tests assuming that any scalar type can be passed now, not just double.


[ghstack-poisoned]
@izdeby izdeby mentioned this pull request Feb 2, 2021
Closed
@stas00 stas00 mentioned this pull request Feb 2, 2021
Open
@blefaudeux blefaudeux mentioned this pull request Feb 2, 2021
Merged
4 tasks
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Feb 2, 2021

@izdeby merged this pull request in cce84b5.

@vincentqb
Copy link
Copy Markdown
Contributor

vincentqb commented Feb 4, 2021 •
edited
Loading

I see errors on master after this pull request landed, see history. Can we confirm the errors are not related to this PR?

Example:

Traceback (most recent call last):
  File "test_optim.py", line 386, in test_adam
    lambda weight, bias: optimizer([weight, bias], lr=1e-3)
  File "test_optim.py", line 243, in _test_basic_cases
    scheduler_constructors
  File "test_optim.py", line 126, in _test_basic_cases_template
    optimizer.step(fn)
  File "/opt/conda/lib/python3.6/site-packages/torch/optim/optimizer.py", line 89, in wrapper
    return func(*args, **kwargs)
  File "/opt/conda/lib/python3.6/site-packages/torch/autograd/grad_mode.py", line 27, in decorate_context
    return func(*args, **kwargs)
  File "/opt/conda/lib/python3.6/site-packages/torch/optim/_multi_tensor/adam.py", line 66, in step
    loss = closure()
  File "test_optim.py", line 115, in fn
    loss.backward()
  File "/opt/conda/lib/python3.6/site-packages/torch/tensor.py", line 245, in backward
    torch.autograd.backward(self, gradient, retain_graph, create_graph, inputs=inputs)
  File "/opt/conda/lib/python3.6/site-packages/torch/autograd/__init__.py", line 147, in backward
    allow_unreachable=True, accumulate_grad=True)  # allow_unreachable flag
RuntimeError: CUDA error: an illegal memory access was encountered

@vincentqb vincentqb mentioned this pull request Feb 4, 2021
Closed
@ngimel
Copy link
Copy Markdown
Collaborator

ngimel commented Feb 4, 2021

I've reverted this PR, there's IMA failure in this PR's CI, and master is currently broken.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Feb 4, 2021

This pull request has been reverted by 443a431.

@ngimel ngimel mentioned this pull request Feb 4, 2021
Closed
facebook-github-bot pushed a commit that referenced this pull request Feb 4, 2021
@facebook-github-bot
unbreak bc test (#51702)
1518aee 
Summary:
Caused by #48223 revert

Pull Request resolved: #51702

Reviewed By: mruberry

Differential Revision: D26245905

Pulled By: ngimel

fbshipit-source-id: 9fd7860ecb5c22b2e568db3347d51e648d6c5d6b
@facebook-github-bot facebook-github-bot deleted the gh/izdeby/67/head branch February 6, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zou3519 zou3519 zou3519 approved these changes

@mcarilli mcarilli Awaiting requested review from mcarilli

@gchanan gchanan Awaiting requested review from gchanan

@ngimel ngimel Awaiting requested review from ngimel

@cpuhrsch cpuhrsch Awaiting requested review from cpuhrsch

Assignees

No one assigned

Labels

cla signed Merged Reverted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@izdeby @zou3519 @ngimel @facebook-github-bot @vincentqb