Implement Cache and __cuda_array_interface__ for v21.06.00 #48

gigony · 2021-06-05T11:08:40Z

I'm sorry for this big change.
This includes CI build failure fixes as well as new features/fixes/documents that couldn't be merged due to CI build failures.

@quasiben on top of your change (#46), the only missing piece was, adding pytest (8334f1f) (which was not revealed until I added set -x to the build script (fae529d).

@jakirkham This change doesn't include a doc builder change in your PR (
https://github.com/rapidsai/cucim/pull/45/files#diff-5ed0e3039eb400d13dc55579a034925ea8d08e9f2054085b1c79fdc46a7c09a8L29) so the script may need to further updated in the next change.

Fix memory leak in deflate decoder (Fix memory leak in deflate decoder #37)
Update build script to handle failures (Fix-up install issues encountered on CI #45, Fix-up install issues encountered on CI part 2 #46)
Fix typo in meta.yaml (Fix-up install issues encountered on CI #45, Fix-up install issues encountered on CI part 2 #46)
Add pytest to gpu build script (Fix-up install issues encountered on CI #45, Fix-up install issues encountered on CI part 2 #46)
Implement cache mechanism ([FEA] Support Cache Mechanism in cuCIM #34)
Add __cuda_array_interface__
Support CUDA 10.x for cucim.skimage package (cuda 10.x support #44)
Expose preferred_memory_capacity() for cache
Fix failures due to GIL and nullptr access
Add notebooks for cache and cuda array interface ([FEA] Support Cache Mechanism in cuCIM #34)
Update change logs for v21.06.00

Remove scripts/docs related to bump2version because we don't need it anymore since the versioning scheme is changed to CalVer.

gigony · 2021-06-07T05:03:38Z

Hi @ajschmidt8, could this change be merged into 21.06.00 branch before the release (#47)?

quasiben · 2021-06-07T12:37:47Z

python/cucim/setup.py

@@ -49,14 +44,13 @@ def read(*names, **kwargs):
    url='https://github.com/rapidsai/cucim',
    packages=find_packages('src'),
    package_dir={'cucim': 'src/cucim'},
-    py_modules=[splitext(basename(path))[0] for path in glob('src/*.py')],
+    # # https://docs.python.org/3/distutils/setupscript.html


Any reason to leave these comments here instead of removing ?

Same question for the package_data= commented code below

Oh, no specific reason. Just for future reference. Will remove it. Thank you for pointing out that!

quasiben · 2021-06-07T12:39:46Z

I found it easier to look at the individual commits. I left a few small comments on the setup.py file. I think there is something probably better to do than add the pytest conda package but i'm ok pushing that off for another PR

jakirkham

Thanks Gigon! 😄

Had some questions below

jakirkham · 2021-06-07T16:00:54Z

python/cucim/src/localtest.py

+    #     count = 0
+    #     for h in range(start_location, height, tile_size):
+    #         for w in range(start_location, width, tile_size):
+    #             count += 1
+    #     start_loc_iter = ((w, h)
+    #                       for h in range(start_location, height, tile_size)
+    #                       for w in range(start_location, width, tile_size))
+    #     with Timer("  Thread elapsed time (OpenSlide)") as timer:
+    #         with concurrent.futures.ThreadPoolExecutor(
+    #             max_workers=num_workers
+    #         ) as executor:
+    #             executor.map(
+    #                 lambda start_loc: load_tile_openslide(
+    #                     slide, start_loc, tile_size),
+    #                 start_loc_iter,
+    #             )
+    #         openslide_time = timer.elapsed_time()
+    #         openslide_tot_time += openslide_time


Why is this commented?

my bad. changed the code during the local testing. Will revert it. Thanks!

All good. Thanks Gigon! 😄

jakirkham · 2021-06-07T16:01:05Z

python/cucim/src/localtest.py

+    # print("  Performance gain (OpenSlide/cuCIM): {}".format(
+    #     openslide_time / cucim_time))


Same question here

Will revert it.

python/cucim/setup.cfg

jakirkham · 2021-06-07T16:28:15Z

ci/gpu/build.sh

@@ -77,13 +79,13 @@ gpuci_conda_retry build -c ${LIBCUCIM_BLD_PATH} -c conda-forge -c rapidsai-night

 # Install cuCIM and its dependencies
 gpuci_logger "Installing cuCIM and its dependencies"
-gpuci_conda_retry install -y -c ${LIBCUCIM_BLD_PATH} -c ${CUCIM_BLD_PATH} \
-    "rapids-build-env=$MINOR_VERSION.*" \


Why did this get dropped?

I found that there is no rapids-build-env package whose version is $MINOR_VERSION.* as we switched to CalVer.

https://anaconda.org/rapidsai/rapids-build-env/files

@gigony, this will need to be re-added. We use the rapidsai-nightly conda channel for pre-release packaging. You can find the cal-ver packages here: https://anaconda.org/rapidsai-nightly/rapids-build-env/files

Somewhat related. I think we are missing an activate line (this is probably my fault). Was trying to fix that here ( 495aa73 ). Think we may need to add that as well

jakirkham · 2021-06-07T16:37:40Z

cpp/include/cucim/cuimage.h

 #include "cucim/core/framework.h"
+


nit: Could we drop this blank line?

Thanks! Will remove it.

jakirkham · 2021-06-07T17:06:04Z

cpp/src/memory/memory_manager.cu

@@ -74,4 +68,53 @@ void get_pointer_attributes(PointerAttributes& attr, const void* ptr)
    }
 }

+bool move_raster_from_host(void** target, size_t size, cucim::io::Device& dst_device)


Can device be const?

Suggested change

bool move_raster_from_host(void** target, size_t size, cucim::io::Device& dst_device)

bool move_raster_from_host(void** target, size_t size, const cucim::io::Device& dst_device)

Yes, it should be const. Will update it.

jakirkham · 2021-06-07T17:06:21Z

cpp/src/memory/memory_manager.cu

+    return true;
+}
+
+bool move_raster_from_device(void** target, size_t size, cucim::io::Device& dst_device)


Suggested change

bool move_raster_from_device(void** target, size_t size, cucim::io::Device& dst_device)

bool move_raster_from_device(void** target, size_t size, const cucim::io::Device& dst_device)

Yes, it should be const. Will update it.

jakirkham · 2021-06-07T17:09:01Z

python/cucim/setup.py

    version=read('VERSION').strip(),  # versioneer.get_version(),
-    # cmdclass=versioneer.get_cmdclass()
+    cmdclass=versioneer.get_cmdclass(),


If we are using versioneer for cmdclass, should we use for version as well?

In case of versioneer, version comes from git tags, but C++ code's build system currently requires version as VERSION file.
I can try to refactor C++ code builder to get the version from git tags later (https://ipenguin.ws/2012/11/cmake-automatically-use-git-tags-as.html) but the current VERSION file seems okay to me.

Currently, version number from versioneer is used only when CuPy with scikit-image is available (#44, 818dc2f#diff-80a85bd6c52ce89c0dc59a606ca1205a2673c20ff860c65a3964db51ffa64ec8R47)

jakirkham · 2021-06-07T17:09:36Z

python/cucim/src/cucim/__init__.py

+# Try to import cupy first.
+# If cucim.clara package is imported first, you may see the following error when running on CUDA 10.x (#44)
+#   python3: Relink `/usr/lib/x86_64-linux-gnu/libnccl.so.2.8.3' with `/lib/x86_64-linux-gnu/librt.so.1' for IFUNC symbol `clock_gettime'
+#   Segmentation fault
+try:
+    import cupy
+except ImportError:
+    pass


Interesting. Do we know why that is?

Don't know the exact root cause, but I guess,

This change is related to #44

If the system has only CUDA 10.x library, since cucim.clara part of the library depends on CUDA 11 and different version of runtime, it looks that it can conflict with symbols CUDA-based library that depends on CUDA 10.X

You can see version-related issues from other Github issue:
tensorflow/tensorflow#19375

jakirkham · 2021-06-07T17:10:18Z

python/cucim/src/localtest.py

-for num_workers in range(1, num_threads + 1):
+for num_workers in (num_threads,):  # range(1, num_threads + 1):


Why is this commented?

It is modified during the local test (since I didn't want to execute it for all available thread # configurations.)
Reverted it.

gigony · 2021-06-07T17:55:59Z

Thanks @jakirkham and @quasiben for the review! I would address your comments tonight.

ajschmidt8

Few changes requested here. Also, branch-21.06 is in code-freeze, so the base branch for this PR will need to be updated to branch-21.08.

ajschmidt8 · 2021-06-07T18:22:27Z

ci/gpu/build.sh

@@ -77,13 +79,13 @@ gpuci_conda_retry build -c ${LIBCUCIM_BLD_PATH} -c conda-forge -c rapidsai-night

 # Install cuCIM and its dependencies
 gpuci_logger "Installing cuCIM and its dependencies"
-gpuci_conda_retry install -y -c ${LIBCUCIM_BLD_PATH} -c ${CUCIM_BLD_PATH} \
-    "rapids-build-env=$MINOR_VERSION.*" \


@gigony, this will need to be re-added. We use the rapidsai-nightly conda channel for pre-release packaging. You can find the cal-ver packages here: https://anaconda.org/rapidsai-nightly/rapids-build-env/files

ajschmidt8 · 2021-06-07T18:26:47Z

CHANGELOG.md

@@ -1,4 +1,4 @@
-# cuCIM 21.06.00 (Date TBD)
+# cuCIM [21.06.00](https://github.com/rapidsai/cucim/wiki/release_notes_v21.06.00) (Date TBD)


For consistency with the changelogs of other RAPIDS libraries, this change should probably be removed. Also, we have a script that updates the changelog during each release, so it would overwrite this change.

Oh, I see. Will remove it from here.

And for the following change, will try to read rapids-build-env with rapidsai-nightly version.

gpuci_conda_retry install -y -c ${LIBCUCIM_BLD_PATH} -c ${CUCIM_BLD_PATH} \ "rapids-build-env=$MINOR_VERSION.*" \

quasiben · 2021-06-07T18:41:17Z

Few changes requested here. Also, branch-21.06 is in code-freeze, so the base branch for this PR will need to be updated to branch-21.08.

For clarity, we'd like for this to be released with 21.06 as other libraries would like to pick this up -- but not part of the larger RAPIDS integration meta-package. Is that something we could do ?

ajschmidt8 · 2021-06-07T18:45:11Z

Few changes requested here. Also, branch-21.06 is in code-freeze, so the base branch for this PR will need to be updated to branch-21.08.

For clarity, we'd like for this to be released with 21.06 as other libraries would like to pick this up -- but not part of the larger RAPIDS integration meta-package. Is that something we could do ?

@quasiben, gotcha. This PR has a lot of changes, so I'm not sure it'd be a good idea to merge all of this a few days before the release. Couple questions here though.

Which libraries require these changes?
Are the changes here considered "required" for 21.06 to work? Or are they just "nice-to-haves"?

jakirkham · 2021-06-07T18:48:34Z

Several of these changes were in previous PRs that were blocked due to CI issues. So Gigon has aggregated them together. IOW they are not as new as they might seem

The changes are really limited to cuCIM and are mainly in response to needs Clara has in their downstream libraries or outside user requests. So they shouldn't affect other RAPIDS libraries

quasiben · 2021-06-07T18:52:12Z

@gigony would be better positioned to answer these questions:

Which libraries require these changes?
a. MONAI is a higher lever product depending on
Are the changes here considered "required" for 21.06 to work? Or are they just "nice-to-haves"?
a. The majority of these changes are in the required for 21.06. I would hope these large code changes before releases decreases as the library matures.

Again, @gigony can you also comment here ?

gigony · 2021-06-07T19:37:37Z

1. MONAI and Clara Train SDK is using cuCIM. since they are using PyPI package and I would release new version of PyPI package(21.06.00) separately tomorrow, conda package doesn't affect to those projects. However, not merging this would cause a discrepancy between the two packages (conda vs PyPI) and that's what I am worrying about. 2. I am sorry for the last minute changes. I will try to push all the needed change before code freeze next time.

…

On Mon, Jun 7, 2021 at 11:52 AM Benjamin Zaitlen ***@***.***> wrote: @gigony <https://github.com/gigony> would be better positioned to answer these questions: 1. Which libraries require these changes? a. MONAI is a higher lever product depending on 2. Are the changes here considered "required" for 21.06 to work? Or are they just "nice-to-haves"? a. The majority of these changes are in the required for 21.06. I would hope these large code changes before releases decreases as the library matures. Again, @gigony <https://github.com/gigony> can you also comment here ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOW2SW4KQCVIOIWEAUI4PDTRUIOVANCNFSM46ENHSCA> .

raydouglass · 2021-06-07T21:19:49Z

I think this is fine to merge this into 21.06 after addressing some of the requested changes.

quasiben · 2021-06-07T21:55:36Z

Thanks @raydouglass

gigony · 2021-06-08T08:37:04Z

Thank you @quasiben @jakirkham @ajschmidt8 for the review!

I have addressed your comments except #48 (comment) to make sure that this change is buildable without errors.

Could we merge this PR first and then apply @jakirkham 's patches shown below, to use rapids conda environment?

`__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. Authors: - Gigon Bae (https://github.com/gigony) Approvers: - https://github.com/jakirkham URL: #65

gigony added 10 commits June 5, 2021 00:29

Fix memory leak in deflate decoder

5cc5779

Update build script to handle failures

fae529d

Fix typo in meta.yaml

8e869ef

Add pytest to gpu build script

8334f1f

Implement cache mechanism

566f130

Remove scripts/docs related to bump2version because we don't need it anymore since the versioning scheme is changed to CalVer.

Add __cuda_array_interface__

465c3ef

Support CUDA 10.x for cucim.skimage package (rapidsai#44)

818dc2f

Expose preferred_memory_capacity() for cache

e3ebcc9

Fix failures due to GIL and nullptr access

39c1c18

Add notebooks for cache and cuda array interface

15cb780

gigony added bug Something isn't working doc Improvements or additions to documentation feature request New feature or request improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jun 5, 2021

gigony requested review from quasiben and jakirkham June 5, 2021 11:08

gigony requested review from a team as code owners June 5, 2021 11:08

gigony changed the base branch from branch-21.08 to branch-21.06 June 5, 2021 11:10

gigony changed the title ~~Implement Cache and __cuda_array_interface__~~ Implement Cache and __cuda_array_interface__ for v21.06.00 Jun 5, 2021

gigony removed bug Something isn't working doc Improvements or additions to documentation improvement Improves an existing functionality labels Jun 5, 2021

gigony added 2 commits June 6, 2021 12:27

Update change logs for v21.06.00

05cfb75

Fix style errors

c310b6f

gigony force-pushed the cache_and_cuda_array branch from e977979 to c310b6f Compare June 6, 2021 19:28

gigony mentioned this pull request Jun 7, 2021

[RELEASE] cucim v21.06 #47

Merged

quasiben reviewed Jun 7, 2021

View reviewed changes

jakirkham reviewed Jun 7, 2021

View reviewed changes

ajschmidt8 requested changes Jun 7, 2021

View reviewed changes

gigony added 4 commits June 8, 2021 01:11

Address comments

095d768

Remove debug print code for cache

1499041

Extract methods for find_if

b0ea781

Fix wrong move of image metadata

49fc434

ajschmidt8 merged commit 45cf987 into rapidsai:branch-21.06 Jun 8, 2021

This was referenced Jun 8, 2021

Update CI scripts #51

Merged

[FEA] Support Cache Mechanism in cuCIM #34

Closed

gigony mentioned this pull request Jul 10, 2021

Fix missing array interface for associated_image() #65

Merged

		# print(" Performance gain (OpenSlide/cuCIM): {}".format(
		# openslide_time / cucim_time))

	bool move_raster_from_host(void** target, size_t size, cucim::io::Device& dst_device)
	bool move_raster_from_host(void** target, size_t size, const cucim::io::Device& dst_device)

	bool move_raster_from_device(void** target, size_t size, cucim::io::Device& dst_device)
	bool move_raster_from_device(void** target, size_t size, const cucim::io::Device& dst_device)

		for num_workers in range(1, num_threads + 1):
		for num_workers in (num_threads,): # range(1, num_threads + 1):

		@@ -1,4 +1,4 @@
		# cuCIM 21.06.00 (Date TBD)
		# cuCIM [21.06.00](https://github.com/rapidsai/cucim/wiki/release_notes_v21.06.00) (Date TBD)

Implement Cache and __cuda_array_interface__ for v21.06.00 #48

Implement Cache and __cuda_array_interface__ for v21.06.00 #48

Conversation

gigony commented Jun 5, 2021 • edited Loading

gigony commented Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quasiben commented Jun 7, 2021 • edited Loading

jakirkham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gigony commented Jun 7, 2021

ajschmidt8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quasiben commented Jun 7, 2021

ajschmidt8 commented Jun 7, 2021

jakirkham commented Jun 7, 2021 • edited Loading

quasiben commented Jun 7, 2021

gigony commented Jun 7, 2021 via email • edited Loading

raydouglass commented Jun 7, 2021 • edited Loading

quasiben commented Jun 7, 2021

gigony commented Jun 8, 2021

gigony commented Jun 5, 2021 •

edited

Loading

gigony commented Jun 7, 2021 •

edited

Loading

quasiben commented Jun 7, 2021 •

edited

Loading

jakirkham commented Jun 7, 2021 •

edited

Loading

gigony commented Jun 7, 2021 via email •

edited

Loading

raydouglass commented Jun 7, 2021 •

edited

Loading