Fix `backend.copyto` for mismatched dtypes to CuPy ndarray #8043

hvy · 2019-08-27T09:39:21Z

Fixes a parameter initialization bug. See #7965 (comment).

hvy · 2019-08-27T10:28:22Z

chainer/backend.py

@@ -65,7 +65,8 @@ def copyto(dst, src):
    elif isinstance(dst, cuda.ndarray):
        if isinstance(src, chainer.get_cpu_array_types()):
            src = numpy.asarray(src)
-            if dst.flags.c_contiguous or dst.flags.f_contiguous:
+            if (src.dtype == dst.dtype


set does not accept an array of different dtype, so we choose to fall through if dtypes don't match.

toslunar

copyto LGTM. You may split the PR for TestUninitializedParameterWithDevices.

toslunar · 2019-08-27T10:51:57Z

tests/chainer_tests/test_variable.py

        x.initialize(a.shape)
        assert isinstance(x.data, xp.ndarray)
        assert x._has_chainerx_array is (xp is chainerx)
-        xp.testing.assert_array_equal(x.data, xp.asarray(a))
+        xp.testing.assert_array_equal(
+            x.data.astype(expected_dtype, copy=False), xp.asarray(a))


I suppose

assert x.dtype == expected_dtype xp.testing.assert_array_equal(x.data, xp.asarray(a, expected_dtype))

toslunar · 2019-08-27T10:53:28Z

tests/chainer_tests/test_variable.py

        x.initialize(a.shape)
        assert isinstance(x.data, xp.ndarray)
        assert x._has_chainerx_array is (xp is chainerx)
-        xp.testing.assert_array_equal(x.data, xp.asarray(a))
+        xp.testing.assert_array_equal(
+            x.data.astype(expected_dtype, copy=False), xp.asarray(a))
        xp.testing.assert_array_equal(x.grad, np.float32('nan'))


Why do you keep np.float32? It could be np.nan or float('nan').

hvy · 2019-08-28T01:56:14Z

Thanks for the comments. I'll address them in a separate PR. Removed test_variable.py from this one. PTAL.

niboshi · 2019-08-28T06:06:33Z

Should copyto support dtype casting?
I think copyto is a low-level function which usual users don't use.
I feel it's better to fix the caller to handle dtypes rather than adding convenient features into copyto.

toslunar · 2019-08-28T06:23:58Z

Random ideas:

Automatic casting is error-prone, assuming that backend.copyto is used in model definitions more often than in training loops.
backend.copyto could always check dtypes.
strides (or just contiguity) might also matter.
numpy.copyto supports casting.

hvy · 2019-08-28T06:52:58Z

I'm okay with making it strict, not allowing different dtypes. This functions is by no means a hot spot so we can afford some checking. We could also take inspiration from numpy.copyto and allow the caller to control the casting behavior, but that's a bit overkill considering the current use cases of this function.

@toslunar do you have concrete cases where stride checking would be beneficial?

toslunar · 2019-08-29T05:13:00Z

@toslunar do you have concrete cases where stride checking would be beneficial?

Not really. Loading NHWC-layout parameters, maybe. A low-level copy-to sounds like memcpy.

toslunar · 2019-09-12T10:02:33Z

The PR is no longer a PR-blocker because #8046 is closed.

stale · 2020-01-26T06:25:54Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 30 days if no further activity occurs. Thank you for your contributions.

toslunar · 2020-02-18T07:47:37Z

We discussed the status of the PR internally.

The issue is classified as a bug because the behavior depends on the contiguity of the arrays.
Since the master branch is stable now, it's better to align the behavior to the permissive side, i.e. allowing casting always (rather than denying it).

toslunar · 2020-02-21T01:36:26Z

Jenkins, test this please.

chainer-ci · 2020-02-21T03:20:49Z

Jenkins CI test (for commit 0aa2ad7, target branch master) succeeded!

chainer-ci · 2020-02-24T09:54:12Z

@toslunar This pull-request is marked as st:test-and-merge, but there were no activities for the last 3 days. Could you check?

toslunar · 2020-02-25T02:43:38Z

Jenkins, test this please.

chainer-ci · 2020-02-25T03:59:19Z

Jenkins CI test (for commit 5bbb615, target branch master) succeeded!

toslunar · 2020-02-25T09:41:58Z

/test

toslunar · 2020-03-26T07:36:55Z

/test

emcastillo · 2020-03-26T08:17:23Z

Jenkins, test this please

chainer-ci · 2020-03-26T09:11:18Z

Jenkins CI test (for commit 5bbb615, target branch master) failed with status FAILURE.

emcastillo · 2020-03-27T00:39:39Z

Jenkins, test this please

chainer-ci · 2020-03-27T03:36:19Z

Jenkins CI test (for commit 5bbb615, target branch master) succeeded!

toslunar · 2020-03-30T07:18:17Z

FlexCI failures: timeout of chainer/tests/chainer_tests/functions_tests/rnn_tests/test_function_slstm.py

emcastillo · 2020-03-30T09:27:13Z

Re-running pfCI still fails on the same tests

[FAILED] /chainer/tests/chainer_tests/functions_tests/normalization_tests/test_l2_normalization.py (60 failed, 486 passed, 102 skipped, 648 deselected in 36.70 seconds)
--

E   cupy.cuda.driver.CUDADriverError: CUDA_ERROR_LAUNCH_OUT_OF_RESOURCES: too many resources requested for launch |  
2020-03-30 18:56:34.864857 STDOUT 2433] |   |  
2020-03-30 18:56:34.864858 STDOUT 2433] | cupy/cuda/driver.pyx:124: CUDADriverError |  
2020-03-30 18:56:34.864859 STDOUT 2433]

Both of them added some extra errors.
Will clone and verify locally 😢

emcastillo · 2020-04-02T09:04:21Z

Jenkins, test this please

chainer-ci · 2020-04-02T10:14:40Z

Jenkins CI test (for commit 5bbb615, target branch master) succeeded!

emcastillo · 2020-04-03T07:45:34Z

Jenkins, test this please

chainer-ci · 2020-04-03T08:46:06Z

Jenkins CI test (for commit 5bbb615, target branch master) succeeded!

Fix backend.copyto for mismatched dtypes to CuPy ndarray

667fa17

hvy added the cat:bug Bug report or fix. label Aug 27, 2019

hvy mentioned this pull request Aug 27, 2019

Support mixed16/float16 GroupNormalization #7965

Merged

toslunar self-assigned this Aug 27, 2019

hvy commented Aug 27, 2019

View reviewed changes

toslunar requested changes Aug 27, 2019

View reviewed changes

gwtnb added the to-be-backported Pull request that should be backported. label Aug 27, 2019

Revert test_variable.py

0aa2ad7

hvy mentioned this pull request Aug 28, 2019

Add test for Parameter initialization with device transfer and config dtype #8046

Closed

hvy added the pr-blocker Blocking other PRs. label Aug 28, 2019

hvy mentioned this pull request Sep 9, 2019

Correctly cast fill_value in constant initializer #8089

Merged

toslunar removed the pr-blocker Blocking other PRs. label Sep 12, 2019

toslunar self-requested a review October 28, 2019 05:26

toslunar added the st:needs-discussion State indicating that discussions are needed before proceeding. label Oct 28, 2019

stale bot added the stale Not updated for a longer period of time. label Jan 26, 2020

toslunar added this to the v7.3.0 milestone Feb 4, 2020

stale bot removed the stale Not updated for a longer period of time. label Feb 4, 2020

toslunar removed st:needs-discussion State indicating that discussions are needed before proceeding. to-be-backported Pull request that should be backported. labels Feb 21, 2020

toslunar added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Feb 21, 2020

Merge branch 'master' into fix-copyto-mismatched-dtype

5bbb615

toslunar removed the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Feb 25, 2020

kmaehashi assigned emcastillo and unassigned toslunar Mar 17, 2020

emcastillo added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Apr 3, 2020

emcastillo approved these changes Apr 3, 2020

View reviewed changes

emcastillo merged commit 0e23958 into chainer:master Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `backend.copyto` for mismatched dtypes to CuPy ndarray #8043

Fix `backend.copyto` for mismatched dtypes to CuPy ndarray #8043

hvy commented Aug 27, 2019

hvy Aug 27, 2019

toslunar left a comment

toslunar Aug 27, 2019

toslunar Aug 27, 2019

hvy commented Aug 28, 2019

niboshi commented Aug 28, 2019

toslunar commented Aug 28, 2019

hvy commented Aug 28, 2019

toslunar commented Aug 29, 2019

toslunar commented Sep 12, 2019

stale bot commented Jan 26, 2020

toslunar commented Feb 18, 2020

toslunar commented Feb 21, 2020

chainer-ci commented Feb 21, 2020

chainer-ci commented Feb 24, 2020

toslunar commented Feb 25, 2020

chainer-ci commented Feb 25, 2020

toslunar commented Feb 25, 2020

toslunar commented Mar 26, 2020

emcastillo commented Mar 26, 2020

chainer-ci commented Mar 26, 2020

emcastillo commented Mar 27, 2020

chainer-ci commented Mar 27, 2020

toslunar commented Mar 30, 2020

emcastillo commented Mar 30, 2020 •

edited

Loading

emcastillo commented Apr 2, 2020

chainer-ci commented Apr 2, 2020

emcastillo commented Apr 3, 2020

chainer-ci commented Apr 3, 2020

Fix backend.copyto for mismatched dtypes to CuPy ndarray #8043

Fix backend.copyto for mismatched dtypes to CuPy ndarray #8043

Conversation

hvy commented Aug 27, 2019

hvy Aug 27, 2019

Choose a reason for hiding this comment

toslunar left a comment

Choose a reason for hiding this comment

toslunar Aug 27, 2019

Choose a reason for hiding this comment

toslunar Aug 27, 2019

Choose a reason for hiding this comment

hvy commented Aug 28, 2019

niboshi commented Aug 28, 2019

toslunar commented Aug 28, 2019

hvy commented Aug 28, 2019

toslunar commented Aug 29, 2019

toslunar commented Sep 12, 2019

stale bot commented Jan 26, 2020

toslunar commented Feb 18, 2020

toslunar commented Feb 21, 2020

chainer-ci commented Feb 21, 2020

chainer-ci commented Feb 24, 2020

toslunar commented Feb 25, 2020

chainer-ci commented Feb 25, 2020

toslunar commented Feb 25, 2020

toslunar commented Mar 26, 2020

emcastillo commented Mar 26, 2020

chainer-ci commented Mar 26, 2020

emcastillo commented Mar 27, 2020

chainer-ci commented Mar 27, 2020

toslunar commented Mar 30, 2020

emcastillo commented Mar 30, 2020 • edited Loading

emcastillo commented Apr 2, 2020

chainer-ci commented Apr 2, 2020

emcastillo commented Apr 3, 2020

chainer-ci commented Apr 3, 2020

Fix `backend.copyto` for mismatched dtypes to CuPy ndarray #8043

Fix `backend.copyto` for mismatched dtypes to CuPy ndarray #8043

emcastillo commented Mar 30, 2020 •

edited

Loading