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.

Sign up for GitHub

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

Jump to bottom

rearrange dependencies.yaml, fix development instructions #1451

Merged
merged 3 commits into from
Sep 9, 2024
Merged

rearrange dependencies.yaml, fix development instructions #1451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7bb212f
rearrange dependencies.yaml, fix development instructions
jameslamb Aug 30, 2024
2760685
merge branch-24.10
jameslamb Sep 5, 2024
caeb6ad
Merge branch 'branch-24.10' into misc/test-dependencies
jameslamb Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions conda/environments/all_cuda-118_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ dependencies:
- ipython
- ipywidgets
- libcudf==24.10.*,>=0.0.0a0
- libcuspatial-tests==24.10.*,>=0.0.0a0
- libcuspatial==24.10.*,>=0.0.0a0
- librmm==24.10.*,>=0.0.0a0
- myst-parser
- nbsphinx
Expand Down
2 changes: 0 additions & 2 deletions conda/environments/all_cuda-125_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ dependencies:
- ipython
- ipywidgets
- libcudf==24.10.*,>=0.0.0a0
- libcuspatial-tests==24.10.*,>=0.0.0a0
- libcuspatial==24.10.*,>=0.0.0a0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the build instructions in the development docs:

cuspatial/docs/source/developer_guide/build.md

Lines 15 to 32 in 1544e7b

## Install dependencies
1. `export CUSPATIAL_HOME=$(pwd)/cuspatial`
2. clone the cuSpatial repo
```shell
conda env create -n cuspatial --file conda/environments/all_cuda-118_arch-x86_64.yaml
```
## Build cuSpatial
### From the cuSpatial Dev Container:
Execute `build-cuspatial-cpp` to build `libcuspatial`. The following options may be added.
- `-DBUILD_TESTS=ON`: build `libcuspatial` unit tests.
- `-DBUILD_BENCHMARKS=ON`: build `libcuspatial` benchmarks.
- `-DCMAKE_BUILD_TYPE=Debug`: Create a Debug build of `libcuspatial` (default is Release).
In addition, `build-cuspatial-python` to build cuspatial cython components.

  1. they recommend starting by creating a conda environment from one of the files in conda/environments
  2. they recommend building libcuspatial and its tests from source

Those steps are currently broken.

The libcuspatial-tests conda package is not distributed anywhere, so that env creation will fail like this:

Solving environment: failed

ResolvePackageNotFound: 
  - libcuspatial-tests[version='24.10.*,>=0.0.0a0']

Here, I'm recommending dropping those packages from these files. Similar to how for example you won't find e.g. libcuml in these files in the cuml repo (code link) or libcuml in the cudf repo (code link).

- librmm==24.10.*,>=0.0.0a0
- myst-parser
- nbsphinx
Expand Down
52 changes: 39 additions & 13 deletions dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ files:
- depends_on_librmm
- rapids_build_skbuild
- run_python_cuspatial
- test_libcuspatial
- test_notebooks
- test_python_cuspatial
- test_python_cuproj
Expand All @@ -30,24 +29,27 @@ files:
output: none
includes:
- cuda_version
- depends_on_libcuspatial
- test_libcuspatial
test_python:
output: none
includes:
- cuda_version
- depends_on_cuproj
- depends_on_cuspatial
- py_version
- test_python_cuspatial
- test_python_cuproj
- test_cuspatial
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that here, I've taken a list that previously contained cuproj, cuspatial, and libcuspatial with only cuproj and cuspatial.

That's intentional.... this test_python list is used for testing the Python conda packages:

cuspatial/ci/test_python.sh

Lines 13 to 17 in 1544e7b

rapids-dependency-file-generator \
--output conda \
--file-key test_python \
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" \
--prepend-channel "${CPP_CHANNEL}" --prepend-channel "${PYTHON_CHANNEL}" | tee env.yaml

Omitting libcuspatial here gives us a chance to learn about packaging errors of the form "cuspatial is missing a dependency on `libcuspatial" in CI.

test_notebooks:
output: none
includes:
- cuda_version
- depends_on_cuml
- depends_on_cuproj
- depends_on_cuspatial
- test_notebooks
- notebooks
- py_version
- test_cuspatial
checks:
output: none
includes:
Expand All @@ -57,9 +59,11 @@ files:
output: none
includes:
- cuda_version
- depends_on_libcuspatial
- depends_on_cuspatial
- depends_on_cuproj
- docs
- py_version
- test_cuspatial
py_build_cuspatial:
output: [pyproject]
pyproject_dir: python/cuspatial
Expand Down Expand Up @@ -133,7 +137,6 @@ files:
includes:
- test_python_cuproj
- depends_on_cuspatial
- test_cuspatial
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section populates the [test] extra for cuproj wheels.

The test_cuspatial list contained only conda dependencies, so it didn't affect wheels in any way. That's why you see this being removed without replacement.


channels:
- rapidsai
Expand Down Expand Up @@ -391,6 +394,12 @@ dependencies:
packages:
- pyproj>=3.6.0,<3.7a0

depends_on_libcuspatial:
common:
- output_types: conda
packages:
- &libcuspatial_unsuffixed libcuspatial==24.10.*,>=0.0.0a0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1450 will add requirements / pyproject sections to this. They don't need to exist until libcuspatial wheels exist.


depends_on_rmm:
common:
- output_types: conda
Expand Down Expand Up @@ -519,6 +528,31 @@ dependencies:
- cuspatial-cu11==24.10.*,>=0.0.0a0
- {matrix: null, packages: [*cuspatial_unsuffixed]}

depends_on_cuproj:
common:
- output_types: conda
packages:
- &cuproj_unsuffixed cuproj==24.10.*,>=0.0.0a0
- output_types: requirements
packages:
# pip recognizes the index as a global option for the requirements.txt file
- --extra-index-url=https://pypi.nvidia.com
- --extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple
specific:
- output_types: [requirements, pyproject]
matrices:
- matrix:
cuda: "12.*"
cuda_suffixed: "true"
packages:
- cuproj-cu12==24.10.*,>=0.0.0a0
- matrix:
cuda: "11.*"
cuda_suffixed: "true"
packages:
- cuproj-cu11==24.10.*,>=0.0.0a0
- {matrix: null, packages: [*cuproj_unsuffixed]}

depends_on_cupy:
common:
- output_types: conda
Expand All @@ -540,15 +574,7 @@ dependencies:
common:
- output_types: conda
packages:
- libcuspatial==24.10.*,>=0.0.0a0
- libcuspatial-tests==24.10.*,>=0.0.0a0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure if I like this structure where we have a dependency list that defines libcuspatial-tests. In libcudf tests we list the libcudf-tests package explicitly rather than using a dependency list. https://github.com/rapidsai/cudf/blob/7018a33be752da9363db5431560d8d12bf378920/ci/test_cpp_common.sh#L34

The *-tests conda packages are kind of special in that they are only CI artifacts and thus never available from any “proper” conda channels, only local channels of CI built artifacts. For that reason I am hesitant to use RDFG to define them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm. The alternate proposal I had in mind (removing the test_libcuspatial dependency list and installing libcuspatial-tests manually) would not make sense for this repo, since we’ve moved to the “single step solve” for C++ tests in cuspatial.

cuspatial/ci/test_cpp.sh

Line 17 in 0e9d36c

--prepend-channel "${CPP_CHANNEL}" | tee env.yaml

Maybe we do have the right approach and no changes are needed…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep exactly, the use of that --prepend-channel pattern here is why I left this as-is.

test_cuspatial:
common:
- output_types: conda
packages:
- libcuspatial==24.10.*,>=0.0.0a0
- cuspatial==24.10.*,>=0.0.0a0
- cuproj==24.10.*,>=0.0.0a0

depends_on_librmm:
common:
Expand Down
Loading