build wheels on CI #619

grlee77 · 2023-10-26T17:43:21Z

This MR includes the commits from #617, so that one should be merged first.

The purpose of this MR is to build and test wheels on RAPIDS CI infrastructure. Previously, cuCIM was using a custom manual wheel building process based on running:

./run build_python_package

which used dockcross/cibuildwheel. We would then manually upload the resulting wheel to PyPI using twine.

The changes in this MR are to enable automated wheel builds for future RAPIDS releases in a similar way to other RAPIDS projects.

Misc issues/questions for discussion

The build_wheels.sh script installs the YASM and the openslide libraries via system package managers. This is currently hard-coded in that script rather than using the dependencies.yaml mechanism being used elsewhere. Is this reasonable or is there a better way?
Following cuML's example, I added a separate ci/release/apply_wheel_modifications.sh script that updates the pyproject.toml as needed (to fixup version string and the CUDA version strings in the cucim package name and CuPy dependency)
I updated the cuCIM test suite to make a handful of tests that relied on imagecodecs to be skipped if it is not installed. This is because I did not find a pre-compiled wheel for it on aarch64 and when it tried to compile from source instead, it failed due to various missing system packages needed for the compilation.
Unlike most RAPIDS projects, which seem to use scikit-build, this project performs a separate CMake build via run build_local commands and copies the C++ libraries into the Python source tree. The pyproject.toml then just does a standard pure-python build, but will package the previously compiled libraries along with the other files.
The wheel tests are currently taking approximately 35-50 minutes to run. It should be possible to reduce that time by an order of magnitude if the CuPy kernels are cached across runs. Does any RAPIDS projects cache the compiled CuPy kernels in this way? (cached kernels end up in ~/.cupy/kernel_cache/ by default, although an alternative location can be specified via CUPY_CACHE_DIR)

…kage manager

…copied)

…s requirements

bdice

Approving. I have a few comments but this is the last pass of review I think I need to give.

CONTRIBUTING.md

ci/wheel_smoke_test.py

dependencies.yaml

bdice · 2023-11-01T20:22:54Z

dependencies.yaml

      - output_types: [conda]
        packages:
+          - imagecodecs>=2021.6.8
+          - matplotlib-base
+          - openslide-python>=1.1.2


Like imagecodecs above, it seems like openslide-python is pinned the same way for conda and requirements. Do you want to list that above in the [conda, requirements, pyproject] section?

Note that in most cases you want the same dependencies in requirements and pyproject because they both install from PyPI, so I'm questioning why we have both [requirements] and [requirements, pyproject] sections. I expect we really only need three sections: [conda, requirements, pyproject] for dependencies pinned the same way in conda/pip, [conda] for conda-only dependencies, and [requirements, pyproject] for pip-only dependencies.

removed duplicate definitions. Install of imagecodecs and openslide-python is currently disabled at wheel test time, but we intend to fix that after this PR is merged (opened issue #626 to track)

bdice · 2023-11-01T20:26:54Z

dependencies.yaml

+        packages:
+          - imagecodecs>=2021.6.8
+          - openslide-python>=1.1.2
+      # temporarily remove imagecodecs / openslide-python from wheel tests


Maybe this code comment affects the review comments I left above. I don't see manylinux ARM builds of imagecodecs. I'll leave it to you to figure out the best course of action here.

Yeah, I need to open an issue to fix running tests when imagecodecs is installed (there is a single test case that fails on CI, but I couldn't reproduce using a wheel built on my local system). There was also an issue on arm64 where it tries to compile from source, but fails (only x86_64 wheels were available)

I went ahead and merged the requirements and pyproject sections and just comment it out in for now.

opened issue #626 to track

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

divyegala

VERSION_CPP is an interesting solution. It does seem to be more concise than what we are doing elsewhere

ci/release/update-version.sh

.github/workflows/build.yaml

ci/build_wheel.sh

Co-authored-by: jakirkham <jakirkham@gmail.com>

vyasr

praise: Generally looks good! All the key changes are in place, there's just a little bit of cleanup to be done I think.

note: Looks like some formatting changes snuck in probably due to the change in the pre-commit config, but they're small and easily identified so not a big deal.

vyasr · 2023-11-06T19:10:22Z

ci/build_python.sh

+# for CMake VERSION need to truncate any trailing 'a'
+version_cpp=${version%a*}


suggestion (non-blocking): The alternative would be maintaining a single file and then stripping out the alpha in CMake. Now that it's done this way I don't know if it's worth changing, but it would let you carry around one less file.

Agreed. I did it this way because I am not familiar with the proper CMake syntax to do it. Happy to update it if you have suggestions on how the CMake code would look.

This would probably do it (after the existing file(STRING...) command):

string(REGEX REPLACE "a.*$" "" VERSION ${VERSION})

Thanks, @vyasr, that seems to work! I pushed a commit to remove VERSION_CPP as well as simplify the CI scripts a bit (there was still writing of some unused VERSION file).

There should now be only a single top-level VERSION file (as well as symlink to it from python/cucim/src/cucim/VERSION)

vyasr · 2023-11-06T19:21:21Z

ci/build_wheel.sh

+package_dir="python/cucim"
+package_src_dir="${package_dir}/src/${package_name}"
+
+CMAKE_BUILD_TYPE="release"


thought: I was going to ask if this was really necessary since it's CMake's default, but now I see that this is for the build_local script. It's out of scope for this PR, but maybe that script should support a default since CMake does?

There is a default, but it is "debug" rather than release! Could potentially change it if there is no objection from @gigony

local subcommand="${1:-all}" local build_type="${2:-debug}"

I'll leave that up to you two decide. It does seem odd to me that the default would be a debug build though.

Raised as issue: #629

vyasr · 2023-11-06T19:21:53Z

ci/build_wheel.sh

+echo "The package name and/or version was modified in the package source. The git diff is:"
+git diff


note: JFYI we've removed this from every other repo at this point but you're welcome to keep it if you find it useful for debugging. It's been a long time since we got much use out of it.

I have now removed it

vyasr · 2023-11-06T19:22:40Z

ci/build_wheel.sh

+echo "The package name and/or version was modified in the package source. The git diff is:"
+git diff
+
+pip install --upgrade pip


suggestion (blocking): This shouldn't be necessary. Were you running into specific problems? If so, do we need to address them somewhere?

No, I was originally also installing a newer CMake etc here and was updating pip before doing that. I have now removed this line.

vyasr · 2023-11-06T19:25:01Z

ci/build_wheel.sh

+# Install pip build dependencies (not yet using pyproject.toml)
+rapids-dependency-file-generator \
+  --file_key "py_build" \
+  --output "requirements" \
+  --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" | tee build_requirements.txt
+pip install -r build_requirements.txt


suggestion (blocking): Is this still true? Now that #617 is merged you should be using pyproject.toml. That said, I don't know how the C++ build works when you do build_local. Are these dependencies somehow used there? That would be strange and require some gymnastics for library paths, but I suppose it's possible?

Yes, I think it still needs to be this way as from the pyproject.toml perspective this is a pure Python package. The pyproject build relies on an external build step (build_local script in this file) having already built and copied the corresponding C++ .so files into the python/cucim/src/cucim/clara folder.

I agree that this is a bit non-standard and we can potentially look into moving to a scikit-build based approach in the future. I don't have time to focus on that at the moment, though.

One benefit of the approach as it is now is that even Windows users can build the pure Python package using pip without having to have CMake and compile the C++ library (which supports linux only). They would be able to use all of cucim.skimage and part of cucim.core, but just would not have the I/O functions in cucim.clara available.

I get that there's a separate build_local run, what I'm confused about is what dependencies are being installed by pip that are used in that step? Is it just for CMake/ninja? If so, are these really "py_build" requirements?

Here's what I see on CI:

# To make changes, edit dependencies.yaml and run `rapids-dependency-file-generator`. cmake>=3.23.1,!=3.25.0 ninja setuptools>=24.2.0 wheel

Trying deleting these lines in PR: #628

vyasr · 2023-11-06T19:28:13Z

cpp/cmake/deps/openslide.cmake

+    elseif (EXISTS /usr/lib/aarch64-linux-gnu/libopenslide.so)
+        set(OPENSLIDE_LIB_PATH /usr/lib/aarch64-linux-gnu/libopenslide.so)


question: I can see why this change was needed in general, but it doesn't seem relevant to this PR. Was it just snuck in out of convenience?

It is needed here or the arm64 wheel builds fail to find libopenslide and fail during the run build_local call. The use of libopenslide is only needed for C++ lib benchmarks and test cases and it isn't directly used by the Python wheel, but it has to be found to run the C++ build stage (There is not currently a CMake flag to disable building the benchmark binary or tests).

… scripts

jakirkham · 2023-11-06T21:59:33Z

In discussion with Vyas offline, we've agreed to go ahead and merge. We can follow up on anything else in subsequent PRs/issues

jakirkham · 2023-11-06T22:00:07Z

Thanks Greg for the PR and everyone for the reviews! 🙏

jakirkham · 2023-11-06T22:00:12Z

/merge

…ests will run (#634) This MR may initially fail during wheel testing due to #626, as a few test cases relying on these packages were not being run at the time #619 was merged. Authors: - Gregory Lee (https://github.com/grlee77) - https://github.com/jakirkham Approvers: - Ray Douglass (https://github.com/raydouglass) - Gigon Bae (https://github.com/gigony) - https://github.com/jakirkham URL: #634

grlee77 added feature request New feature or request non-breaking Introduces a non-breaking change labels Oct 26, 2023

grlee77 added this to the v23.12.00 milestone Oct 26, 2023

grlee77 requested review from a team as code owners October 26, 2023 17:43

grlee77 force-pushed the grelee/build-wheel-on-ci branch from 190b779 to 28fd183 Compare October 27, 2023 01:24

grlee77 requested a review from a team as a code owner October 27, 2023 02:15

grlee77 force-pushed the grelee/build-wheel-on-ci branch 2 times, most recently from 1b7b409 to 6f7c6ef Compare October 27, 2023 03:04

grlee77 changed the title ~~[draft] build wheels on CI~~ build wheels on CI Oct 27, 2023

grlee77 self-assigned this Oct 27, 2023

grlee77 added 18 commits October 27, 2023 15:30

add wheel building scripts and workflow entries

a4e2160

update pr-builder entries

29f5e9d

make wheel build/test scripts executable

cdbb0b2

call pip directly

3ec1147

temporarily disable install step of cucim bindings build

fd29f8e

apply the new patch

f2c6323

remove patches and install YASM and openslide dev libs via system pac…

c2b7e82

…kage manager

add missing apt update call

6d453ee

update build_wheel.sh

1794824

restore previously commented line

bcb3e19

fix copy command (don't add -r and use *.so so cmake subfolder isn't …

6036955

…copied)

omit build of cuslide_benchmarks and cuslide_tests on arm64

94c0e93

omit building cucim_benchmarks, cucim_tests on arm64

e381d11

fix patch

d932e98

omit copying bin folder

edec7a0

update dependencies.yaml to remove non-pypi files from pyproject test…

9f371cf

…s requirements

remove imagecodecs and openslide-python test dependencies

b70fce1

add missing fi in if/else statement

4a1d09e

bdice approved these changes Nov 1, 2023

View reviewed changes

grlee77 and others added 3 commits November 1, 2023 19:48

apply black globally

fe2ebd9

cleanup test_python section of dependencies.yaml

6ebfccf

Update CONTRIBUTING.md

01dcff6

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

divyegala approved these changes Nov 2, 2023

View reviewed changes

raydouglass reviewed Nov 3, 2023

View reviewed changes

ci/release/update-version.sh Outdated Show resolved Hide resolved

.github/workflows/build.yaml Outdated Show resolved Hide resolved

ci/build_wheel.sh Outdated Show resolved Hide resolved

This was referenced Nov 3, 2023

Add yasm & openslide for wheel builds rapidsai/ci-imgs#90

Closed

Add yasm & openslide for cuCIM wheel builds rapidsai/ci-imgs#91

Merged

jakirkham reviewed Nov 3, 2023

View reviewed changes

ci/build_wheel.sh Outdated Show resolved Hide resolved

grlee77 and others added 3 commits November 3, 2023 17:09

just use NEXT_FULL_TAG directly for VERSION_CPP

20df41f

Update ci/build_wheel.sh

4ca9eff

Co-authored-by: jakirkham <jakirkham@gmail.com>

remove unnecessary 'needs' condition

f65b052

grlee77 force-pushed the grelee/build-wheel-on-ci branch from a041436 to f65b052 Compare November 3, 2023 22:41

raydouglass approved these changes Nov 6, 2023

View reviewed changes

Merge branch 'branch-23.12' into grelee/build-wheel-on-ci

02b8918

vyasr requested changes Nov 6, 2023

View reviewed changes

minor cleanup to build_wheel.sh

c8f0968

jakirkham requested a review from vyasr November 6, 2023 20:13

handle stripping of alpha from VERSION string in CMake to simplify CI…

c05219f

… scripts

jakirkham removed the request for review from vyasr November 6, 2023 21:58

jakirkham approved these changes Nov 6, 2023

View reviewed changes

rapids-bot bot merged commit 7d56b5e into rapidsai:branch-23.12 Nov 6, 2023
39 checks passed

jakirkham mentioned this pull request Nov 10, 2023

Updating cuCIM's build/CI infra to use scikit-build, rapids-cmake, whl builds, etc. #305

Closed

This was referenced Nov 17, 2023

install imagecodecs and openslide-python dependencies so additional tests will run #634

Merged

[BUG] fix isolated test failure in test_cache_hit_miss #626

Open

jakirkham mentioned this pull request Dec 9, 2023

Cleanup old build files #655

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build wheels on CI #619

build wheels on CI #619

grlee77 commented Oct 26, 2023 •

edited

Loading

bdice left a comment

bdice Nov 1, 2023

grlee77 Nov 6, 2023

bdice Nov 1, 2023

grlee77 Nov 1, 2023

grlee77 Nov 6, 2023

divyegala left a comment

vyasr left a comment

vyasr Nov 6, 2023

grlee77 Nov 6, 2023

vyasr Nov 6, 2023

grlee77 Nov 6, 2023

vyasr Nov 6, 2023

grlee77 Nov 6, 2023

vyasr Nov 6, 2023

jakirkham Nov 10, 2023

vyasr Nov 6, 2023

grlee77 Nov 6, 2023 •

edited

Loading

vyasr Nov 6, 2023

grlee77 Nov 6, 2023 •

edited

Loading

vyasr Nov 6, 2023

grlee77 Nov 6, 2023

vyasr Nov 6, 2023

jakirkham Nov 10, 2023

jakirkham Nov 10, 2023

vyasr Nov 6, 2023

grlee77 Nov 6, 2023

jakirkham commented Nov 6, 2023

jakirkham commented Nov 6, 2023

jakirkham commented Nov 6, 2023

		# for CMake VERSION need to truncate any trailing 'a'
		version_cpp=${version%a*}

		echo "The package name and/or version was modified in the package source. The git diff is:"
		git diff

		elseif (EXISTS /usr/lib/aarch64-linux-gnu/libopenslide.so)
		set(OPENSLIDE_LIB_PATH /usr/lib/aarch64-linux-gnu/libopenslide.so)

build wheels on CI #619

build wheels on CI #619

Conversation

grlee77 commented Oct 26, 2023 • edited Loading

Misc issues/questions for discussion

bdice 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

divyegala left a comment

Choose a reason for hiding this comment

vyasr 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

grlee77 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grlee77 Nov 6, 2023 • edited Loading

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

jakirkham commented Nov 6, 2023

jakirkham commented Nov 6, 2023

jakirkham commented Nov 6, 2023

grlee77 commented Oct 26, 2023 •

edited

Loading

grlee77 Nov 6, 2023 •

edited

Loading

grlee77 Nov 6, 2023 •

edited

Loading