-
Notifications
You must be signed in to change notification settings - Fork 55
Remove some unnecessary uses of ContextResettingTestCase #507
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
Changes from all commits
e888d07
ee4b8d3
aae6461
90c8909
bb10c44
62c044e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,17 +6,18 @@ | |
| import numpy as np | ||
|
|
||
| from numba.cuda.cudadrv import driver, drvapi, devices | ||
| from numba.cuda.testing import unittest, ContextResettingTestCase | ||
| from numba.cuda.testing import unittest, CUDATestCase | ||
| from numba.cuda.testing import skip_on_cudasim | ||
|
|
||
|
|
||
| @skip_on_cudasim("CUDA Memory API unsupported in the simulator") | ||
| class TestCudaMemory(ContextResettingTestCase): | ||
| class TestCudaMemory(CUDATestCase): | ||
| def setUp(self): | ||
| super().setUp() | ||
| self.context = devices.get_context() | ||
|
|
||
| def tearDown(self): | ||
| self.context.reset() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @gmarkall I think this should clear things safely, this appears to free the resources within the context but is less scorched earth than a full
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean it will no longer invalidate assumptions made by cuda-core?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gmarkall are you asking if
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Step by step this method:
My reading of this is that the net effect is that all resources owned by this context object are released - but it only releases resources it itself allocated. This is distinct from the full gpu reset function called by
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for clarifying - this makes sense. |
||
| del self.context | ||
| super(TestCudaMemory, self).tearDown() | ||
|
|
||
|
|
@@ -107,7 +108,7 @@ def dtor(): | |
| self.assertEqual(dtor_invoked[0], 2) | ||
|
|
||
|
|
||
| class TestCudaMemoryFunctions(ContextResettingTestCase): | ||
| class TestCudaMemoryFunctions(CUDATestCase): | ||
| def setUp(self): | ||
| super().setUp() | ||
| self.context = devices.get_context() | ||
|
|
@@ -153,7 +154,7 @@ def test_d2d(self): | |
|
|
||
|
|
||
| @skip_on_cudasim("CUDA Memory API unsupported in the simulator") | ||
| class TestMVExtent(ContextResettingTestCase): | ||
| class TestMVExtent(CUDATestCase): | ||
| def test_c_contiguous_array(self): | ||
| ary = np.arange(100) | ||
| arysz = ary.dtype.itemsize * ary.size | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,13 @@ | |
| import numpy as np | ||
| from numba.cuda.cudadrv import driver | ||
| from numba import cuda | ||
| from numba.cuda.testing import unittest, ContextResettingTestCase | ||
| from numba.cuda.testing import unittest, CUDATestCase | ||
|
|
||
|
|
||
| class TestHostAlloc(ContextResettingTestCase): | ||
| class TestHostAlloc(CUDATestCase): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lot of these are fairly old tests from the upstream numba code base where it looks like a lot of tests may have simply reset the device every time in an effort to be extra safe and start with a clean slate every time. This may have made sense at the time but might not anymore. |
||
| def tearDown(self): | ||
| cuda.current_context().reset() | ||
|
|
||
| def test_host_alloc_driver(self): | ||
| n = 32 | ||
| mem = cuda.current_context().memhostalloc(n, mapped=True) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,6 @@ | |
| from numba.cuda.cudadrv.linkable_code import CUSource | ||
| from numba.cuda.testing import ( | ||
| CUDATestCase, | ||
| ContextResettingTestCase, | ||
| skip_on_cudasim, | ||
| ) | ||
|
|
||
|
|
@@ -42,7 +41,7 @@ def get_hashable_handle_value(handle): | |
|
|
||
|
|
||
| @skip_on_cudasim("Module loading not implemented in the simulator") | ||
| class TestModuleCallbacksBasic(ContextResettingTestCase): | ||
| class TestModuleCallbacksBasic(CUDATestCase): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Connected with @isVoid about these and they should be safe to remove.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the reasons I left this was because I hadn't dug into why it was used, and it wasn't obvious to me - can you share the reasoning please?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it is superfluous. |
||
| def test_basic(self): | ||
| counter = 0 | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,11 @@ | |
| import threading | ||
| from numba import cuda | ||
| from numba.cuda.cudadrv.driver import driver | ||
| from numba.cuda.testing import unittest, ContextResettingTestCase | ||
| from numba.cuda.testing import unittest, CUDATestCase | ||
| from queue import Queue | ||
|
|
||
|
|
||
| class TestResetDevice(ContextResettingTestCase): | ||
| class TestResetDevice(CUDATestCase): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment (and code) below suggests that the context on the main thread is unaffected (it creates new threads for the tests) so I think there should be no need to reset the context again after the test has run. |
||
| def test_reset_device(self): | ||
| def newthread(exception_queue): | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
|
|
||
| import numpy as np | ||
| from numba import cuda | ||
| from numba.cuda.testing import unittest, ContextResettingTestCase | ||
| from numba.cuda.testing import unittest, CUDATestCase | ||
|
|
||
|
|
||
| def newthread(exception_queue): | ||
|
|
@@ -21,12 +21,12 @@ def newthread(exception_queue): | |
| stream.synchronize() | ||
| del dA | ||
| del stream | ||
| cuda.close() | ||
| cuda.synchronize() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the intention of |
||
| except Exception as e: | ||
| exception_queue.put(e) | ||
|
|
||
|
|
||
| class TestSelectDevice(ContextResettingTestCase): | ||
| class TestSelectDevice(CUDATestCase): | ||
| def test_select_device(self): | ||
| exception_queue = Queue() | ||
| for i in range(10): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,14 +5,14 @@ | |
|
|
||
| from numba import vectorize, guvectorize | ||
| from numba import cuda | ||
| from numba.cuda.testing import unittest, ContextResettingTestCase, ForeignArray | ||
| from numba.cuda.testing import unittest, CUDATestCase, ForeignArray | ||
| from numba.cuda.testing import skip_on_cudasim, skip_if_external_memmgr | ||
| from numba.cuda.tests.support import linux_only, override_config | ||
| from unittest.mock import call, patch | ||
|
|
||
|
|
||
| @skip_on_cudasim("CUDA Array Interface is not supported in the simulator") | ||
| class TestCudaArrayInterface(ContextResettingTestCase): | ||
| class TestCudaArrayInterface(CUDATestCase): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think context resets are needed here - some tests check the list of pending deallocations and flush it, but that should not require a context reset. |
||
| def assertPointersEqual(self, a, b): | ||
| self.assertEqual( | ||
| a.device_ctypes_pointer.value, b.device_ctypes_pointer.value | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems to rely on there being no active context at all which is hard to guarantee without the hard
cuda.close.