Skip to content
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

Remove NumPy <2 pin, remove proj pin #1441

Merged
merged 31 commits into from
Sep 24, 2024

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Aug 19, 2024

This PR removes the NumPy<2 pin which is expected to work for
RAPIDS projects once CuPy 13.3.0 is released (CuPy 13.2.0 had
some issues preventing the use with NumPy 2).

Also contributes to rapidsai/build-planning#56 by removing the pin on proj (which allows this project to work with newer fmt / spdlog, in sync with conda-forge).

@seberg seberg added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Aug 19, 2024
@github-actions github-actions bot added conda Related to conda and conda configuration Python Related to Python code labels Aug 19, 2024
@bdice
Copy link
Contributor

bdice commented Aug 21, 2024

I merged in the upstream to see if CI is failing in the same way we see here: rapidsai/docker#709 (comment)

@jakirkham jakirkham closed this Aug 24, 2024
@jakirkham jakirkham reopened this Aug 24, 2024
@jakirkham
Copy link
Member

Closing and reopening to restart CI now that cuDF is done: rapidsai/cudf#16300

@jakirkham jakirkham marked this pull request as ready for review August 24, 2024 05:53
@jakirkham jakirkham requested a review from a team as a code owner August 24, 2024 05:53
@jakirkham
Copy link
Member

jakirkham commented Aug 24, 2024

Seeing some unexpected and unrelated failures that appear to be due to a cuDF change

Running CI in a no change PR to see if this reproduces there: #1444

Edit: Seeing the same error over there ( #1444 (comment) ). Following up separately

@jakirkham
Copy link
Member

On the Conda side, it looks like we are still seeing NumPy 1 in CI.

numpy                     1.26.4          py311h64a7726_0    conda-forge

Think we are missing another dependency. Will need to take a closer look

@jakirkham
Copy link
Member

Took all of the dependencies on CI (except RAPIDS ones) and tried to create a Conda environment with them locally on Linux ARM (though there should be no difference between x86_64 in this regard). It created the environment using NumPy 2

Didn't see anything obviously missing in this repo

The other RAPIDS dependencies are all migrated

Not sure what we are missing. May need to dig deeper later

@jameslamb
Copy link
Member

Edit: Seeing the same error over there ( #1444 (comment) ). Following up separately

Strongly suspect this is a result of the changes from rapidsai/cudf#15483.

I'll work on adapting cuspatial to account for those, but going to wait just a bit until rapidsai/cudf#16640 is merged.

@jakirkham
Copy link
Member

Depends on PR: #1447

rapids-bot bot pushed a commit that referenced this pull request Aug 29, 2024
Contributes to rapidsai/build-planning#33.

Proposes the following for `cuspatial` wheels:

* add build and runtime dependencies on `libcudf` wheels
* stop vendoring copies of `libcudf.so`, `libnvcomp.so`, `libnvcomp_bitcomp.so`, and `libnvcomp_gdeflate.so`
  - *(load `libcudf.so` dynamically at runtime instead)*

And other related changes for development/CI:

* combine all `pip install` calls into 1 in wheel-testing scripts
  - *like rapidsai/cudf#16575
  - *to improve the chance that packaging issues are discovered in CI*
* `dependencies.yaml` changes:
   - more use of YAML anchors = less duplication
   - use dedicated `depends_on_librmm` and `depends_on_libcudf` groups
* explicitly pass a package type to `gha-tools` wheel uploading/downloading scripts

## Notes for Reviewers

### Benefits of these changes

Unblocks CI in this repo (ref: #1444 (comment), #1441 (comment)).

Reduces wheel sizes for `cuspatial` wheels by about 125MB 😁 

| wheel          | size (before)  | size (this PR) |
|:-----------:|-------------:|---------------:|
| `cuspatial` |   146.0M        |   21M               |
| `cuproj `     |       0.9M       |   0.9M              |
|**TOTAL**   |  **146.9M** | **21.9M**        |

*NOTES: size = compressed, "before" = 2024-08-21 nightlies (c60bd4d), CUDA = 12, Python = 3.11*

<details><summary>how I calculated those (click me)</summary>

```shell
# note: 2024-08-21 because that was the most recent date with
#           successfully-built cuspatial nightlies
#
docker run \
    --rm \
    -v $(pwd):/opt/work:ro \
    -w /opt/work \
    --network host \
    --env RAPIDS_NIGHTLY_DATE=2024-08-21 \
    --env RAPIDS_NIGHTLY_SHA=c60bd4d \
    --env RAPIDS_PR_NUMBER=1447 \
    --env RAPIDS_PY_CUDA_SUFFIX=cu12 \
    --env RAPIDS_REPOSITORY=rapidsai/cuspatial \
    --env WHEEL_DIR_BEFORE=/tmp/wheels-before \
    --env WHEEL_DIR_AFTER=/tmp/wheels-after \
    -it rapidsai/ci-wheel:cuda12.5.1-rockylinux8-py3.11 \
    bash

mkdir -p "${WHEEL_DIR_BEFORE}"
mkdir -p "${WHEEL_DIR_AFTER}"

py_projects=(
    cuspatial
    cuproj
)

for project in "${py_projects[@]}"; do
    # before
    RAPIDS_BUILD_TYPE=nightly \
    RAPIDS_PY_WHEEL_NAME="${project}_${RAPIDS_PY_CUDA_SUFFIX}" \
    RAPIDS_REF_NAME="branch-24.10" \
    RAPIDS_SHA=${RAPIDS_NIGHTLY_SHA} \
        rapids-download-wheels-from-s3 python "${WHEEL_DIR_BEFORE}"

    # after
    RAPIDS_BUILD_TYPE=pull-request \
    RAPIDS_PY_WHEEL_NAME="${project}_${RAPIDS_PY_CUDA_SUFFIX}" \
    RAPIDS_REF_NAME="pull-request/${RAPIDS_PR_NUMBER}" \
        rapids-download-wheels-from-s3 python "${WHEEL_DIR_AFTER}"
done

du -sh ${WHEEL_DIR_BEFORE}/*
du -sh ${WHEEL_DIR_BEFORE}
du -sh ${WHEEL_DIR_AFTER}/*
du -sh ${WHEEL_DIR_AFTER}
```

</details>

Reduces the amount of additional work required to start shipping `libcuspatial` wheels.

### Background

This is part of ongoing work towards packaging `libcuspatial` as a wheel.

relevant prior work:

* packaging `libcudf` wheels: rapidsai/cudf#15483
* consolidating `pip install` calls in CI scripts for `cudf`: rapidsai/cudf#16575
* `cudf` dropping its Arrow library dependency: rapidsai/cudf#16640

### How I tested this

Confirmed in local builds and CI logs that `cudf` is being *found*, not *built*, in `cuspatial` builds.

```text
-- CPM: Using local package [email protected]
```

([build link](https://github.com/rapidsai/cuspatial/actions/runs/10602971716/job/29386288614?pr=1447#step:9:23472))

Built `cuspatial` wheels locally and ran all the unit tests, without issue.

#

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Matthew Roeschke (https://github.com/mroeschke)

URL: #1447
@jameslamb
Copy link
Member

#1447 is merged and I've pulled in those changes here

@jakirkham
Copy link
Member

jakirkham commented Aug 30, 2024

Ok think I understand what is happening with NumPy 1 being installed on CI now, OSMnx 1.9.4 (currently latest) pins to NumPy 1

IIUC OSMnx 2.0 will relax this constraint

I don't think there is a lot we can do about that

Looking at the code here, it appears the cuSpatial library doesn't actually use OSMnx. Only the notebooks do

Perhaps there is a way to move that dependency out of the main CI paths so we can test with NumPy 2 here (ignoring notebooks which will have to stay on NumPy 1)

Think if we can do that, we can move forward

Edit: Nvm this is already only installed in the notebook job. So this is not an issue

@jakirkham
Copy link
Member

Should add another piece is rasterio needs a sufficiently recent package build to pickup up NumPy 2 support: conda-forge/rasterio-feedstock#299

This can only happen if proj 9.4 is installed. However we were constraining proj to 9.3. James proposed relaxing that in PR ( #1449 ), which is now merged

While rasterio is not a direct dependency, it does get pulled in through other dependencies we have in the spatial packages. So James fix should also help relax pinnings for NumPy 2 to more easily be picked up

@jameslamb
Copy link
Member

🎉 🎉 🎉

...
fmt                       11.0.2               h70be974_0    conda-forge
...
numpy                     2.0.2           py311he9aa9f1_0    conda-forge
...
proj                      9.5.0                h07e4b22_0    conda-forge
...
spdlog                    1.14.1               h9d9cc24_1    conda-forge
...
tiledb                    2.26.1               hf11d815_0    conda-forge
...

(build-python-tests link)

@jameslamb jameslamb added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Sep 19, 2024
@jameslamb jameslamb changed the title Remove NumPy <2 pin Remove NumPy <2 pin, remove proj pin Sep 19, 2024
@jameslamb jameslamb added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Sep 19, 2024
conda/recipes/cuspatial/meta.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the ci label Sep 23, 2024
@jameslamb jameslamb removed 3 - Ready for Review Ready for review by team 5 - DO NOT MERGE Hold off on merging; see PR for details labels Sep 23, 2024
@jameslamb
Copy link
Member

Ran CI again after removing all testing/debugging changes... still getting numpy 2.x!

I think this can be merged.

...
cudf                      24.10.00a373    cuda12_py312_240923_g8b12cf4e66_373    rapidsai-nightly
cuproj                    24.10.00a75     cuda12_py312_240923_g218cc315_75    file:///tmp/python_channel
cupy                      13.3.0          py312h7d319b9_0    conda-forge
cupy-core                 13.3.0          py312h28031eb_0    conda-forge
cuspatial                 24.10.00a75     cuda12_py312_240923_g218cc315_75    file:///tmp/python_channel
...
fmt                       11.0.2               h434a139_0    conda-forge
...
numba                     0.60.0          py312h83e6fd3_0    conda-forge
numpy                     2.0.2           py312h58c1407_0    conda-forge
...
spdlog                    1.14.1               hed91bc2_1    conda-forge
...

(build link)

@jameslamb
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit be73c8e into rapidsai:branch-24.10 Sep 24, 2024
75 checks passed
@jakirkham
Copy link
Member

Huzzah! 🥳

Thanks for all of the hard work here. Especially pushing through fmt / spdlog migrations James! 🙏

rapids-bot bot pushed a commit to rapidsai/integration that referenced this pull request Sep 24, 2024
Contributes to rapidsai/build-planning#56

With rapidsai/cuspatial#1441, it should be possible to revert some of the workarounds introduced in #719.

## Notes for Reviewers

### How to test this

if this is working, we should see the following in the conda solves:

* `fmt >=11.0.2`
* `spdlog >=1.14.1`

We won't see `numpy >=2` yet, because `cugraph` doesn't support it yet (rapidsai/cugraph#4615).

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #722
@seberg seberg deleted the my_new_branch branch September 25, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conda Related to conda and conda configuration improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to Python code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants