Merge `CUPY_CUB_BLOCK_REDUCTION_DISABLED` and `CUB_DISABLED` by asi1024 · Pull Request #3461 · cupy/cupy

asi1024 · 2020-06-19T08:59:26Z

Part of #3445.

Merges CUPY_CUB_BLOCK_REDUCTION_DISABLED and CUB_DISABLED into CUPY_CUB_ENABLED.
Supports interfaces to switch to use each backend, or define their priorities.
- Environment variable: CUPY_BACKENDS.
- cupy.core.set_reduction_backends and cupy.core.routine_backends (These functions are for debug use. We will leave them undocumented.)

…_DISABLED

asi1024 · 2020-06-24T04:50:42Z

PTAL.

leofang

Thanks again, @asi1024, for working on this! I've done a quick pass and left some comments. The major change needed here is to handle scan properly. I think we can use CUB for both faster indexing (which I intended to do but overlooked for some reason!) and cumsum/cumprod, but the current implementation introduces a regression for the latter.

leofang · 2020-06-24T15:30:07Z

cupy/cuda/__init__.py

 cub_enabled = False
-if int(os.getenv('CUB_DISABLED', 0)) == 0:
+if int(os.getenv('CUPY_CUB_ENABLED', 0)) == 1:
    try:
        from cupy.cuda import cub  # NOQA
        cub_enabled = True


I think this entire block (including the two lines below) should be changed to support CUPY_BACKENDS? At least cupy.cuda.cub_enabled should be removed as per #3445 (comment), but you'll need to fix every place that it appears in, such as the tests added in #2598 and #3428.

Fixed in ccd71b0.

cupy/core/_backend.pyx

leofang · 2020-06-24T16:03:36Z

cupy/core/_routines_math.pyx

+    for backend in _backend._routine_backends:
+        if backend == _backend.BACKEND_CUB:
+            # result will be None if the scan is not compatible with CUB
+            if op == scan_op.SCAN_SUM:
+                cub_op = cub.CUPY_CUB_CUMSUM
+            else:
+                cub_op = cub.CUPY_CUB_CUMPROD
+            res = cub.cub_scan(out, cub_op)
+            if res is not None:
+                return out


We probably need this here (for fast indexing) as well as in scan_core() (where this came from), because cupy/math/sumprod.py calls the latter directly. Moreover, this early appearance does not take care:

copy input: scan is an in-place operation (whether using CUB or not), so the input a must be copied to out.

ravel input: CUB scan needs a 1D contiguous input.

axis: CUB only handles non-batched scans (axis is None).

So at least for cupy.cumsum() and cupy.cumprod() this won't work.

Fix in 6d12fdc. Thanks!

leofang · 2020-06-24T16:04:02Z

cupy/core/_routines_math.pyx

-        if cupy.cuda.cub_enabled:
-            # result will be None if the scan is not compatible with CUB
-            if op == scan_op.SCAN_SUM:
-                cub_op = cub.CUPY_CUB_CUMSUM
-            else:
-                cub_op = cub.CUPY_CUB_CUMPROD
-            res = cub.cub_scan(result, cub_op)
-        if not cupy.cuda.cub_enabled or res is None:
-            scan(result, op, dtype, result)
+        scan(result, op, dtype, result)


See my comment above for CUB scan.

cupyx/scipy/sparse/csr.py

cupy/core/_backend.pyx

Co-authored-by: Leo Fang <leofang@bnl.gov>

asi1024 · 2020-06-25T08:04:23Z

PTAL.

leofang

LGTM except for a few small nitpicks. I think you can apply all suggestions at once by switching to the "Files changed" tab and adding them to the batch. This way we won't generate excessive commits.

leofang · 2020-06-25T13:54:33Z

tests/cupy_tests/core_tests/test_ndarray_reduction.py

+@unittest.skipUnless(
+    cupy.core._backend.get_routine_backends() == ['cub'],
+    'The CUB routine is not enabled')
+class TestFcontiguousReduction(unittest.TestCase):


Not sure why the class name was changed? Note both C- & F- contiguity get tested.

Suggested change

class TestFcontiguousReduction(unittest.TestCase):

class TestCUBreduction(unittest.TestCase):

Sorry, I thought the skip condition is not needed and renamed before, and I forgot to undo after this tests don't pass without skip condition.

leofang · 2020-06-25T13:58:39Z

tests/cupy_tests/core_tests/test_ndarray_reduction.py

-@unittest.skipIf(cupy.cuda.cub_enabled is False, 'The CUB module is not built')
-class TestCUBreduction(unittest.TestCase):
+@unittest.skipUnless(
+    cupy.core._backend.get_routine_backends() == ['cub'],


I think as long as CUB is set enabled (or cupy.cuda.cub is built), it should be fine that other backends exist?

Suggested change

cupy.core._backend.get_routine_backends() == ['cub'],

'cub' in cupy.core._backend.get_routine_backends(),

No, another backend may be chosen. However, cupy.core._backend.get_routine_backends()[:1] == ['cub'] may be better because the get_routine_backends indicates the priority of backends.

Ah I see. That sounds good. Either [0] or [:1] is fine.

Hi @asi1024 I thought about this again, and I think the rationale here was that if cupy.cuda.cub is built it should get tested (regardless how backends were set). If one of the CIs would be set up with CUPY_BACKENDS="cub," then this is OK; otherwise, once #2584 is up, we'll need to ensure these tests are run (by removing skipUnless() and using setUp() and tearDown() to temporarily overwrite the backends).

Let me know if you'd configure the CI or I should address the skips in #2584, thanks!

I agree to set backends in setUp and tearDown. Fixed in 43b2da6.

leofang · 2020-06-25T13:58:55Z

tests/cupy_tests/math_tests/test_sumprod.py

 @testing.gpu
-@unittest.skipIf(cupy.cuda.cub_enabled is False, 'The CUB module is not built')
+@unittest.skipUnless(
+    cupy.core._backend.get_routine_backends() == ['cub'],


Suggested change

cupy.core._backend.get_routine_backends() == ['cub'],

'cub' in cupy.core._backend.get_routine_backends(),

leofang · 2020-06-25T13:59:02Z

tests/cupy_tests/sorting_tests/test_search.py

 @testing.gpu
-@unittest.skipIf(cupy.cuda.cub_enabled is False, 'The CUB module is not built')
+@unittest.skipUnless(
+    cupy.core._backend.get_routine_backends() == ['cub'],


Suggested change

cupy.core._backend.get_routine_backends() == ['cub'],

'cub' in cupy.core._backend.get_routine_backends(),

leofang · 2020-06-25T13:59:08Z

tests/cupyx_tests/scipy_tests/sparse_tests/test_csr.py

 @testing.gpu
-@unittest.skipIf(cupy.cuda.cub_enabled is False, 'The CUB module is not built')
+@unittest.skipUnless(
+    cupy.core._backend.get_routine_backends() == ['cub'],


Suggested change

cupy.core._backend.get_routine_backends() == ['cub'],

'cub' in cupy.core._backend.get_routine_backends(),

leofang · 2020-06-25T15:10:00Z

cupy/cuda/__init__.py

-cub_enabled = False
-if int(os.getenv('CUB_DISABLED', 0)) == 0:
-    try:
-        from cupy.cuda import cub  # NOQA
-        cub_enabled = True
-    except ImportError:
-        pass


👍 I thought this change would disable from cupy.cuda import cub but it seems still working. Interesting...

leofang · 2020-06-25T15:13:23Z

cupy/core/_routines_math.pyx

+                res = cub.cub_scan(out, cub_op)
+                if res is not None:
+                    break
+        else:


GJ on using for-else 👍

leofang · 2020-06-25T15:15:29Z

cupyx/scipy/sparse/csr.py

-if cupy.cuda.cub_enabled:
+
+try:
    from cupy.cuda.cub import device_csrmv


Actually this violates the dev guideline...Let's fix it properly:

Suggested change

from cupy.cuda.cub import device_csrmv

from cupy.cuda import cub

leofang · 2020-06-25T15:15:45Z

cupyx/scipy/sparse/csr.py

+try:
    from cupy.cuda.cub import device_csrmv
+except ImportError:
+    pass


Suggested change

pass

cub = None

leofang · 2020-06-25T15:16:06Z

cupyx/scipy/sparse/csr.py

+                    for backend in _backend.get_routine_backends():
+                        if (backend == _backend.BACKEND_CUB
+                                and other.flags.c_contiguous):
+                            return device_csrmv(


Suggested change

return device_csrmv(

return cub.device_csrmv(

asi1024 · 2020-07-07T11:13:23Z

Jenkins, test this please.

pfn-ci-bot · 2020-07-07T11:13:27Z

Successfully created a job for commit a4ab70e:

Dashboard for commit a4ab70e

asi1024 · 2020-07-07T11:21:03Z

Jenkins, test this please.

pfn-ci-bot · 2020-07-07T11:21:07Z

Successfully created a job for commit a4ab70e:

Dashboard for commit a4ab70e

chainer-ci · 2020-07-07T11:22:04Z

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

asi1024 · 2020-07-07T11:22:51Z

I should trigger after chainer/chainer-test#586 is merged. I canceled the Jenkins CI.

asi1024 · 2020-07-08T12:09:19Z

Jenkins, test this please.

pfn-ci-bot · 2020-07-08T12:09:24Z

Successfully created a job for commit a4ab70e:

Dashboard for commit a4ab70e

chainer-ci · 2020-07-08T12:31:35Z

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

leofang

Oops! This line in test_csr.py:

func = 'cupyx.scipy.sparse.csr.device_csrmv'

needs to be changed to

func = 'cupyx.scipy.sparse.csr.cub.device_csrmv'

asi1024 · 2020-07-08T13:41:18Z

Jenkins, test this please.

pfn-ci-bot · 2020-07-08T13:41:22Z

Successfully created a job for commit 41f4e45:

Dashboard for commit 41f4e45

chainer-ci · 2020-07-08T13:55:08Z

Jenkins CI test (for commit 41f4e45, target branch master) succeeded!

leofang · 2020-07-08T17:24:30Z

@asi1024 Do you know why some CUB tests (I think) were skipped in cupy-py35? For example, search test_sumprod.py here:
https://jenkins.preferred.jp/job/chainer/job/cupy_pr/1493/TEST=cupy-py35,label=mn1-p100/console
Other CIs do not have this issue.

asi1024 · 2020-07-08T18:00:22Z

@leofang cupy-py35 uses NumPy 1.15.4, so 4 tests with @testing.with_requires('numpy>=1.16') in test_sumprod.py seems to have been skipped.

asi1024 · 2020-07-09T07:53:10Z

Jenkins, test this please.

pfn-ci-bot · 2020-07-09T07:53:13Z

Successfully created a job for commit 8a8feea:

Dashboard for commit 8a8feea

chainer-ci · 2020-07-09T08:09:15Z

Jenkins CI test (for commit 8a8feea, target branch master) succeeded!

kmaehashi · 2020-07-09T15:06:48Z

LGTM!

asi1024 added 3 commits June 19, 2020 08:52

Support set_{reduction/routine}_backends

dce99e9

Fix to use set_reduction_backends instead of CUPY_CUB_BLOCK_REDUCTION…

eb0325d

…_DISABLED

Rename CUB_DISABLED to CUPY_CUB_ENABLED

fb3455a

leofang mentioned this pull request Jun 20, 2020

Add tests for _cub_reduction #3462

Merged

3 tasks

kmaehashi self-assigned this Jun 22, 2020

kmaehashi added this to the v8.0.0b4 milestone Jun 22, 2020

asi1024 added 3 commits June 24, 2020 04:39

Set default backends from environment variables

11cce72

Fix to use _routine_backends to determine backend

c38a70b

Fix to use _routine_backends from scipy.sparse

4d00408

asi1024 force-pushed the core-backends branch from 8d6a414 to 4d00408 Compare June 24, 2020 04:43

asi1024 marked this pull request as ready for review June 24, 2020 04:46

asi1024 modified the milestones: v8.0.0b4, v8.0.0b5 Jun 24, 2020

leofang requested changes Jun 24, 2020

View reviewed changes

leofang mentioned this pull request Jun 24, 2020

Build the cupy.cuda.cub module by default #2584

Merged

2 tasks

kmaehashi reviewed Jun 25, 2020

View reviewed changes

cupy/core/_backend.pyx Outdated Show resolved Hide resolved

asi1024 and others added 3 commits June 25, 2020 05:48

Use list for set_*_backends inputs

fbf46c5

Define _get_backend as a cdef function

5dd3769

Co-authored-by: Leo Fang <leofang@bnl.gov>

Rename _get_routine_backends to get_routine_backends

0d7323f

Co-authored-by: Leo Fang <leofang@bnl.gov>

asi1024 force-pushed the core-backends branch from d30b2c4 to 0d7323f Compare June 25, 2020 05:51

Remove cub_enabled

ccd71b0

emcastillo modified the milestone: v8.0.0b5 Jun 25, 2020

Fix cub scan

6d12fdc

leofang requested changes Jun 25, 2020

View reviewed changes

asi1024 added 3 commits June 26, 2020 06:40

Remove unused internal variable

503e3d3

Fix following coding guideline

9566aee

Revert test name

2f61909

Fix to set _routine_accelerators

a4ab70e

asi1024 mentioned this pull request Jul 7, 2020

Rename CUPY_BACKENDS to CUPY_ACCELERATORS chainer/chainer-test#586

Merged

leofang requested changes Jul 8, 2020

View reviewed changes

Fix mock path

41f4e45

leofang approved these changes Jul 8, 2020

View reviewed changes

Merge remote-tracking branch 'cupy/master' into core-backends

8a8feea

kmaehashi added the cat:enhancement Improvements to existing features label Jul 9, 2020

kmaehashi merged commit 96fc0e8 into cupy:master Jul 9, 2020

asi1024 deleted the core-backends branch July 9, 2020 16:26

This was referenced Jul 9, 2020

Support CUB histogram #3473

Merged

Change docs to reflect CUPY_ACCELERATORS #3586

Closed

asi1024 added the no-compat Disrupts backward compatibility label Jul 12, 2020

	class TestFcontiguousReduction(unittest.TestCase):
	class TestCUBreduction(unittest.TestCase):

	cupy.core._backend.get_routine_backends() == ['cub'],
	'cub' in cupy.core._backend.get_routine_backends(),

	from cupy.cuda.cub import device_csrmv
	from cupy.cuda import cub

Uh oh!

Conversation

asi1024 commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asi1024 commented Jun 24, 2020

Uh oh!

leofang left a comment

Choose a reason for hiding this comment

Uh oh!

leofang Jun 24, 2020 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asi1024 Jun 25, 2020 • 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!

Uh oh!

Uh oh!

asi1024 commented Jun 25, 2020

Uh oh!

leofang left a comment

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!

leofang Jun 25, 2020 • 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!

asi1024 Jun 26, 2020 • 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!

leofang Jun 27, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

asi1024 commented Jul 7, 2020

Uh oh!

pfn-ci-bot commented Jul 7, 2020

Uh oh!

asi1024 commented Jul 7, 2020

Uh oh!

pfn-ci-bot commented Jul 7, 2020

Uh oh!

chainer-ci commented Jul 7, 2020

asi1024 commented Jun 19, 2020 •

edited

Loading

leofang Jun 24, 2020 •

edited

Loading

asi1024 Jun 25, 2020 •

edited

Loading

leofang Jun 25, 2020 •

edited

Loading

asi1024 Jun 26, 2020 •

edited

Loading

leofang Jun 27, 2020 •

edited

Loading

asi1024 commented Jul 8, 2020 •

edited

Loading