Solve UnboundLocalError on copy_from_host_async

[viantirreau]

Close #4898

[leofang]

Apparently we missed this because there's no test covering any copy_*_async() functions that use streams, at least they are not directly tested in test_memory.py.

@viantirreau is it possible for you to follow #4562 and add async tests? It should be sufficient to duplicate existing tests with streams. (Even better: parametrize the tests with and without streams.) Let me know if you need any help or me taking over the test part (since it was me who messed up 😅), thanks!

[leofang]

I just checked. Could you move this line to the if size > 0: block below? This would align with the other function copy_to_host_async in the same file.

[viantirreau]

No problem!
I will work on this tonight :)

[emcastillo]

lets delete it in +473, now is dupped :)

[viantirreau]

Ooops 😆

[viantirreau]

Just a heads-up. I'm not very acquainted with Python tests, especially regarding GPUs 😆
Nevertheless, I think I can pull this off by looking at your PR #4562 and the other tests.

Even better: parametrize the tests with and without streams

By parametrized you mean using the decorator? If that's the case, I was thinking about creating a class especially for the async methods, something in the lines of

@testing.parameterize(*testing.product({
    'use_streams': [True, False],
}))
@testing.gpu
class TestMemoryPointerAsync(unittest.TestCase):
    
    def setUp(self):
        self.stream = stream_module.Stream() if self.use_streams else None

    def test_copy_to_and_from_host_async_using_raw_ptr(self):
        a_gpu = memory.alloc(4)
        a_cpu = ctypes.c_int(100)
        a_cpu_ptr = ctypes.cast(ctypes.byref(a_cpu), ctypes.c_void_p)
        a_gpu.copy_from_async(a_cpu_ptr.value, 4, stream=self.stream)

        b_cpu = ctypes.c_int()
        b_cpu_ptr = ctypes.cast(ctypes.byref(b_cpu), ctypes.c_void_p)
        a_gpu.copy_to_host_async(b_cpu_ptr.value, 4, stream=self.stream)
        assert b_cpu.value == a_cpu.value

    def test_copy_from_device_async_using_raw_ptr(self):
        a_gpu = memory.alloc(4)
        a_cpu = ctypes.c_int(100)
        a_cpu_ptr = ctypes.cast(ctypes.byref(a_cpu), ctypes.c_void_p)
        a_gpu.copy_from_async(a_cpu_ptr.value, 4, stream=self.stream)

        b_gpu = memory.alloc(4)
        b_gpu.copy_from_async(a_gpu, 4, stream=self.stream)
        b_cpu = ctypes.c_int()
        b_cpu_ptr = ctypes.cast(ctypes.byref(b_cpu), ctypes.c_void_p)
        b_gpu.copy_to_host_async(b_cpu_ptr.value, 4, stream=self.stream)
        assert b_cpu.value == a_cpu.value

When defining self.stream, I'm hesitant about whether it's correct to use stream_module.Stream() or cupy.cuda.Stream(), as I saw both being used in stream-like variables in the cuda memory tests.

PS: I'm now thinking that I could just parametrize the existing class TestMemoryPointer, and add these 3 methods. Appreciate your view on this.

[viantirreau]

Ended up copying other two synchronous tests and I confirmed that all of them are passing in copy_*_async(), yay 🥳
Now I think that it's better to have this TestMemoryPointerAsync class alone, so as to not interfere with (and clutter) the non-stream versions of the tests. What do you think?

[leofang]

Thanks a lot, @viantirreau! I think it makes sense to isolate async tests to a separate class. Just left a few comments, mainly to ensure no race condition happens.

[leofang]

FYI, in the future to commit a bunch of suggestions at once, you can go to the "Files changed" tab, and select "add suggestion to batch". It'd appear as one single commit, reducing the need of multiple clicks 🙂

[leofang]

LGTM! Thanks again for reporting the bug and for contributing the fix.

[leofang]

Jenkins, test this please

[viantirreau]

Wooow! That's a great tip hahaha.

Thanks for your help, I enjoyed the challenge :)

[leofang]

Another tip: @viantirreau you can edit your PR description to include this clause "Close #4898". By the time your PR is merged, the issue will be automatically closed. A GitHub convenience feature.

[leofang]

Note: I confirm locally that the added tests in this PR would fail if the bug were not fixed.

[chainer-ci]

Jenkins CI test (for commit 2e4c2b3, target branch master) succeeded!

[leofang]

@emcastillo Automerge seems to be glitchy?