Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid sharing handles between threads #2053

Merged
merged 10 commits into from Apr 1, 2019

Conversation

Projects
None yet
6 participants
@kmaehashi
Copy link
Member

commented Feb 26, 2019

Fixes #1109 and #2045.

@kmaehashi

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@okuta Could you take a look if this fix is acceptable? I'd like to do the same thing for all handles.

@okuta

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

OK, It is acceptable.

@okuta okuta self-assigned this Feb 26, 2019

@okuta okuta added this to the v6.0.0b3 milestone Feb 26, 2019

Show resolved Hide resolved cupy/cuda/device.pyx Outdated
Show resolved Hide resolved cupy/cuda/device.pyx Outdated
Show resolved Hide resolved cupy/cuda/device.pyx Outdated
@pentschev

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@kmaehashi thanks for starting with this issue, don't hesitate to ask me if you need some extra hands to help with it.

@beam2d beam2d removed this from the v6.0.0b3 milestone Feb 28, 2019

@kmaehashi kmaehashi force-pushed the kmaehashi:fix-cusolver-threading branch from 372e23e to 20862a0 Mar 4, 2019

@kmaehashi kmaehashi changed the title [WIP] Avoid sharing handles between threads Avoid sharing handles between threads Mar 4, 2019

@kmaehashi

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Ready for review.

@pentschev

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

FWIW, I just tested this branch with my multithreaded use cases and it works great. Thanks a lot for this PR, @kmaehashi!

@kmaehashi

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@pentschev Thanks for testing PR! We will proceed to review.

@okuta okuta added this to the v6.0.0rc1 milestone Mar 17, 2019

@okuta

okuta approved these changes Mar 17, 2019

@okuta

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

jenkins, test this please.

@chainer-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 17, 2019

Jenkins CI test (for commit 520f0b2, target branch master) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@okuta

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

E   AssertionError: Parameterized test failed.
E   
E   Base test method: TestLschol.test_csrmatrix
E   Test parameters:
E     dtype: <type 'numpy.float32'>
E   
E   AssertionError: 
E   Fail: 10, Success: 0
E   
E   The first error message:
E   Traceback (most recent call last):
E     File "cupy/testing/condition.py", line 61, in <lambda>
E       lambda: f(ins, *args[1:], **kwargs),
E     File "/work/cupy/tests/cupyx_tests/linalg_tests/sparse_tests/test_solve.py", line 61, in test_csrmatrix
E       x = cupyx.linalg.sparse.lschol(A, b)
E     File "cupyx/linalg/sparse/solve.py", line 49, in lschol
E       handle = device.get_cusolver_sp_handle()
E     File "cupy/cuda/device.pyx", line 57, in cupy.cuda.device.get_cusolver_sp_handle
E       cpdef get_cusolver_sp_handle():
E     File "cupy/cuda/device.pyx", line 58, in cupy.cuda.device.get_cusolver_sp_handle
E       return _get_device().cusolver_sp_handle
E     File "cupy/cuda/device.pyx", line 213, in cupy.cuda.device.Device.cusolver_sp_handle.__get__
E       'cusolver_sp_handles', cusolver.spCreate, cusolver.spDestroy)
E   AttributeError: 'module' object has no attribute 'spDestroy'```
@jakirkham

This comment has been minimized.

Copy link

commented Mar 18, 2019

Looks like this is just a matter of adding a spDestroy function that wraps cusolverSpDestroy. Does that sound right?

@kmaehashi

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

Thanks, locally confirmed tests/cupyx_tests/linalg_tests/sparse_tests/test_solve.py pass.

@jakirkham Yes, I added it.

@okuta

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

travis ci error.

    cupy/cuda/cusolver.cpp:2370:80: error: ‘cusolverSpDestroy’ was not declared in this scope
             __pyx_v_status = cusolverSpDestroy(((cusolverSpHandle_t)__pyx_v_handle));```
@chainer-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2019

Jenkins CI test (for commit 11dd88e, target branch master) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@kmaehashi

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

Test failures are not related to this PR.

@jakirkham

This comment has been minimized.

Copy link

commented Mar 31, 2019

Is there anything else needed before integrating this PR? Anything that either of you need help with?

@okuta

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

LGTM!

@okuta okuta merged commit b6d7c51 into cupy:master Apr 1, 2019

3 of 4 checks passed

Jenkins Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 91.081%
Details
@okuta

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

I will backport this PR after #2121 merged.

@kmaehashi kmaehashi deleted the kmaehashi:fix-cusolver-threading branch Apr 1, 2019

@jakirkham

This comment has been minimized.

Copy link

commented Apr 1, 2019

Thank you! 😊

@pentschev

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Thank you all for the effort here, this fix is very much welcome for the work I've been doing! :)

okuta added a commit to okuta/cupy that referenced this pull request Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.