[gigony]

__array_interface__ for the object returned by CuImage.associated_image() was missing so cannot be converted numpy.

This bug was introduced during the implementation of Cache and support for __cuda_array_interface__: #48 .

associated_image() API is not frequently used by users so it's fine.

We will need to add a test case for this once the artificial test image with the associated image (such as Philips TIFF) is available.

[grlee77]

This looks okay to me, but I have only rarely looked at pybind11 code, so it would be nice to have a test case to be 100% sure. Can you confirm that you tested the fix locally with a TIFF file?

Is there not some small public domain TIFF we could add as test data?

[gigony]

Thanks @grlee77! I have tested it locally with a Philips Tiff file having associated image so I can confirm it is working properly with the fix. I couldn't find some very small size of such test data so I would like to add test case for that later with generating an artificial test data with associated image during test time by using PyTest's fixture in the separate PR.

…

On Wed, Jul 21, 2021 at 1:37 PM Gregory R. Lee ***@***.***> wrote: ***@***.**** commented on this pull request. This looks okay to me, but I have only rarely looked at pybind11 code, so it would be nice to have a test case to be 100% sure. Can you confirm that you tested the fix locally with a TIFF file? Is there not some small public domain TIFF we could add as test data? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#65 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOW2ST2ODP2B3TJUK3UWSTTY4VZPANCNFSM5ADV525A> .

[jakirkham]

Thanks Gigon! 😄

Had a question about how we handle acquiring the GIL here

[jakirkham]

@gpucibot merge

[jakirkham]

Thanks Gigon! 😄

[gigony]

Thanks Gigon! 😄

Had a question about how we handle acquiring the GIL here

Thanks @jakirkham !

The line 403 (https://github.com/rapidsai/cucim/pull/65/files#diff-48de51ebe61a16c8d3878cba6d31c628f024287fc2671440f9bae39ab6bcbd64R403 ) is for acquiring GIL :)

Before 20.06.00, CuImage object always had __array_interface__ attributes, and depending on the availability of the array data, it exposed correct __array_interface__ content or None/{} value. GIL could be released with the mechanism, to speed up in multi-thread use cases.

However, from 20.06.00, we introduced supporting __cuda_array_interface__. Due to how existing frameworks (CuPy, PyTorch, NumPy) worked, I should be able to remove one of those two attributes (__array_interface__ and __cuda_array_interface__) when creating CuImage object (by read_region() or associated_image()). However, there is no native C++ way to create or delete attributes dynamically (I saw you raised issues pybind/pybind11#21 but it is close now).

For this reason, it needed to acquire GIL (for using Python objects in C++).
It could affect performance when used in multi-threading environment, but I did a quick benchmark (with v0.19.0) and couldn't find a significant performance degradation.
However, we can revisit this issue when the performance issue happens.

Thanks!