-
Notifications
You must be signed in to change notification settings - Fork 61
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
build wheels on CI #619
build wheels on CI #619
Changes from 66 commits
a4e2160
29f5e9d
cdbb0b2
3ec1147
fd29f8e
f2c6323
c2b7e82
6d453ee
1794824
bcb3e19
6036955
94c0e93
e381d11
d932e98
edec7a0
9f371cf
b70fce1
4a1d09e
4c4f4a0
977f672
5f87e2a
6153d1b
cf2ca0e
e872185
9c4d5ce
46254bc
076964a
f22f45c
1d088c7
7ce3653
6a2d6fc
3b058c7
a353e6a
2791228
dff57f8
aba929e
80225dc
6ad0883
8937ab1
9bb33ab
75cd8e8
6a4518d
82a29a6
4d6a369
a76f858
488dfc6
922c5db
44e9847
661c188
5d7bbd9
025a455
c9e8af3
76d858b
ca5059d
9cc4987
33edec4
3daf9ad
5d9645e
4277ae7
588cd24
fe2ebd9
6ebfccf
01dcff6
20df41f
4ca9eff
f65b052
02b8918
c8f0968
c05219f
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 |
---|---|---|
|
@@ -66,3 +66,22 @@ jobs: | |
branch: ${{ inputs.branch }} | ||
date: ${{ inputs.date }} | ||
sha: ${{ inputs.sha }} | ||
wheel-build: | ||
secrets: inherit | ||
uses: rapidsai/shared-workflows/.github/workflows/[email protected] | ||
with: | ||
build_type: ${{ inputs.build_type || 'branch' }} | ||
branch: ${{ inputs.branch }} | ||
sha: ${{ inputs.sha }} | ||
date: ${{ inputs.date }} | ||
script: ci/build_wheel.sh | ||
wheel-publish: | ||
needs: wheel-build | ||
secrets: inherit | ||
uses: rapidsai/shared-workflows/.github/workflows/[email protected] | ||
with: | ||
build_type: ${{ inputs.build_type || 'branch' }} | ||
branch: ${{ inputs.branch }} | ||
sha: ${{ inputs.sha }} | ||
date: ${{ inputs.date }} | ||
package-name: cucim |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ jobs: | |
- conda-python-build | ||
- conda-python-tests | ||
- docs-build | ||
- wheel-build | ||
- wheel-tests | ||
secrets: inherit | ||
uses: rapidsai/shared-workflows/.github/workflows/[email protected] | ||
checks: | ||
|
@@ -50,3 +52,17 @@ jobs: | |
arch: "amd64" | ||
container_image: "rapidsai/ci-conda:latest" | ||
run_script: "ci/build_docs.sh" | ||
wheel-build: | ||
needs: checks | ||
secrets: inherit | ||
uses: rapidsai/shared-workflows/.github/workflows/[email protected] | ||
with: | ||
build_type: pull-request | ||
script: ci/build_wheel.sh | ||
wheel-tests: | ||
needs: wheel-build | ||
secrets: inherit | ||
uses: rapidsai/shared-workflows/.github/workflows/[email protected] | ||
with: | ||
build_type: pull-request | ||
script: ci/test_wheel.sh |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,3 +22,12 @@ jobs: | |
branch: ${{ inputs.branch }} | ||
date: ${{ inputs.date }} | ||
sha: ${{ inputs.sha }} | ||
wheel-tests: | ||
secrets: inherit | ||
uses: rapidsai/shared-workflows/.github/workflows/[email protected] | ||
with: | ||
build_type: nightly | ||
branch: ${{ inputs.branch }} | ||
date: ${{ inputs.date }} | ||
sha: ${{ inputs.sha }} | ||
script: ci/test_wheel.sh |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
#!/bin/bash | ||
# Copyright (c) 2023, NVIDIA CORPORATION. | ||
|
||
set -euo pipefail | ||
|
||
package_name="cucim" | ||
package_dir="python/cucim" | ||
package_src_dir="${package_dir}/src/${package_name}" | ||
|
||
CMAKE_BUILD_TYPE="release" | ||
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. 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 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. 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}" 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'll leave that up to you two decide. It does seem odd to me that the default would be a debug build though. 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. Raised as issue: #629 |
||
|
||
source rapids-configure-sccache | ||
source rapids-date-string | ||
|
||
version=$(rapids-generate-version) | ||
commit=$(git rev-parse HEAD) | ||
|
||
# for CMake VERSION_CPP need to truncate any trailing 'a' | ||
version_cpp=${version%a*} | ||
|
||
RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen ${RAPIDS_CUDA_VERSION})" | ||
|
||
# Patch project metadata files to include the CUDA version suffix and version override. | ||
pyproject_file="${package_dir}/pyproject.toml" | ||
|
||
PACKAGE_CUDA_SUFFIX="-${RAPIDS_PY_CUDA_SUFFIX}" | ||
# update package name to have the cuda suffix | ||
sed -i "s/name = \"${package_name}\"/name = \"${package_name}${PACKAGE_CUDA_SUFFIX}\"/g" ${pyproject_file} | ||
echo "${version}" > VERSION | ||
echo "${version_cpp}" > VERSION_CPP | ||
echo "${version_cpp}" > "${package_dir}/VERSION" | ||
sed -i "/^__git_commit__/ s/= .*/= \"${commit}\"/g" "${package_src_dir}/_version.py" | ||
|
||
if [[ ${PACKAGE_CUDA_SUFFIX} == "-cu12" ]]; then | ||
# change pyproject.toml to use CUDA 12.x version of cupy | ||
sed -i "s/cupy-cuda11x/cupy-cuda12x/g" ${pyproject_file} | ||
fi | ||
|
||
echo "The package name and/or version was modified in the package source. The git diff is:" | ||
git diff | ||
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. 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. 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 have now removed it |
||
|
||
pip install --upgrade pip | ||
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. suggestion (blocking): This shouldn't be necessary. Were you running into specific problems? If so, do we need to address them somewhere? 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. No, I was originally also installing a newer CMake etc here and was updating pip before doing that. I have now removed this line. |
||
|
||
# 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 | ||
Comment on lines
+34
to
+39
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. 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 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. Yes, I think it still needs to be this way as from the 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 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 get that there's a separate 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. 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 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. Trying deleting these lines in PR: #628 |
||
|
||
|
||
# First build the C++ lib using CMake via the run script | ||
echo "libcucim version: `cat VERSION_CPP`" | ||
./run build_local all ${CMAKE_BUILD_TYPE} | ||
|
||
cd "${package_dir}" | ||
|
||
python -m pip wheel . -w dist -vvv --no-deps --disable-pip-version-check | ||
|
||
mkdir -p final_dist | ||
python -m auditwheel repair -w final_dist dist/* | ||
|
||
RAPIDS_PY_WHEEL_NAME="${package_name}_${RAPIDS_PY_CUDA_SUFFIX}" rapids-upload-wheels-to-s3 final_dist |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#!/bin/bash | ||
# Copyright (c) 2023, NVIDIA CORPORATION. | ||
|
||
set -eou pipefail | ||
|
||
RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen ${RAPIDS_CUDA_VERSION})" | ||
RAPIDS_PY_WHEEL_NAME="cucim_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 ./dist | ||
|
||
# echo to expand wildcard before adding `[extra]` requires for pip | ||
python -m pip install $(echo ./dist/cucim*.whl)[test] | ||
|
||
# Run smoke tests for aarch64 pull requests | ||
if [[ "$(arch)" == "aarch64" && ${RAPIDS_BUILD_TYPE} == "pull-request" ]]; then | ||
python ./ci/wheel_smoke_test.py | ||
else | ||
# TODO: revisit enabling imagecodecs package during testing | ||
python -m pytest ./python/cucim | ||
fi |
grlee77 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import cupy as cp | ||
|
||
import cucim | ||
import cucim.skimage | ||
|
||
|
||
if __name__ == "__main__": | ||
# verify that all top-level modules are available | ||
assert cucim.is_available("clara") | ||
assert cucim.is_available("core") | ||
assert cucim.is_available("skimage") | ||
|
||
# generate a synthetic image and apply a filter | ||
img = cucim.skimage.data.binary_blobs(length=512, n_dim=2) | ||
assert isinstance(img, cp.ndarray) | ||
assert img.dtype.kind == "b" | ||
assert img.shape == (512, 512) | ||
|
||
eroded = cucim.skimage.morphology.binary_erosion( | ||
img, cp.ones((3, 3), dtype=bool) | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,9 @@ if (NOT TARGET deps::openslide) | |
set(OPENSLIDE_LIB_PATH "$ENV{CONDA_PREFIX}/lib/libopenslide.so") | ||
elseif (EXISTS /usr/lib/x86_64-linux-gnu/libopenslide.so) | ||
set(OPENSLIDE_LIB_PATH /usr/lib/x86_64-linux-gnu/libopenslide.so) | ||
else () # CentOS 6 | ||
elseif (EXISTS /usr/lib/aarch64-linux-gnu/libopenslide.so) | ||
set(OPENSLIDE_LIB_PATH /usr/lib/aarch64-linux-gnu/libopenslide.so) | ||
Comment on lines
+25
to
+26
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. 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? 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. It is needed here or the arm64 wheel builds fail to find libopenslide and fail during the |
||
else () # CentOS (x86_64) | ||
set(OPENSLIDE_LIB_PATH /usr/lib64/libopenslide.so) | ||
endif () | ||
|
||
|
This file was deleted.
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.
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.
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.
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.
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 would probably do it (after the existing
file(STRING...)
command):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.
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
)