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

rearrange dependencies.yaml, fix development instructions #1451

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

jameslamb
Copy link
Member

Description

Follow-up to #1448. This proposes some dependencies.yaml changes that I noticed while working on adding libcuspatial wheels in #1450.

I've left inline comments with more details, but in short:

  • not including any cuspatial projects in the conda environment files checked into this repo (which are intended to be used to set up a development environment)
  • removing an unnecessary conda-only list of dependencies from pyproject.toml-specific config for cuproj
  • introduction of depends_on_libcuspatial and depends_on_cuproj lists in dependencies.yaml, to reduce duplication

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jameslamb jameslamb added 2 - In Progress Currenty a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 30, 2024
@github-actions github-actions bot added the conda Related to conda and conda configuration label Aug 30, 2024
@@ -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:

## 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).

@@ -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.

Comment on lines 397 to 401
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.

- 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:

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.

@jameslamb jameslamb changed the title WIP: [NOT READY FOR REVIEW] rearrange dependencies.yaml, fix development instructions rearrange dependencies.yaml, fix development instructions Aug 30, 2024
@jameslamb jameslamb removed the 2 - In Progress Currenty a work in progress label Aug 30, 2024
@jameslamb jameslamb marked this pull request as ready for review August 30, 2024 21:28
@jameslamb jameslamb requested a review from a team as a code owner August 30, 2024 21:28
@jameslamb jameslamb requested a review from bdice August 30, 2024 21:28
@harrism
Copy link
Member

harrism commented Sep 6, 2024

Are you planning on fixing the instructions, as the title of this PR says?

@jameslamb
Copy link
Member Author

Are you planning on fixing the instructions, as the title of this PR says

The changes to dependencies.yaml do fix the instructions... now following this step in the docs will succeed:

conda env create -n cuspatial --file conda/environments/all_cuda-118_arch-x86_64.yaml

where previously that would have failed because libcuspatial-tests is not distributed anywhere.

Sorry if the wording in the title here was unclear, I can see how it makes it seems like this might modify those instructions.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Approving with a question about best practices… which I partly answered for myself. Happy to hear feedback but I think no changes are required.

@@ -573,13 +595,6 @@ dependencies:
- output_types: conda
packages:
- 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.

--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.

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 0bbd71d into rapidsai:branch-24.10 Sep 9, 2024
64 checks passed
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
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants