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

Update Dependency List with dependencies.yaml [skip gpuci] #803

Merged
merged 20 commits into from
Nov 21, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Nov 16, 2022

Description

This PR updates dependency lists with dependencies.yaml.

Checklist

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

@isVoid isVoid added conda Related to conda and conda configuration 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 16, 2022
@isVoid isVoid self-assigned this Nov 16, 2022
@isVoid isVoid requested a review from a team as a code owner November 16, 2022 19:14
includes:
- build
- develop
- py_version
Copy link
Contributor

Choose a reason for hiding this comment

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

Style checks should not be dependent on a specific Python version being specified in the matrix (if no value is specified, the matrix match will fail). The Python requirement in the build list should be sufficient. However, we may want a default (empty matrix in the last entry) of py_version defining >=3.8,<3.10 instead of putting it in build.

Suggested change
- py_version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason doing it just for py_version? Why not also cython?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And are you suggesting removing python entry from build and use py_version in all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you did what I wanted -- remove py_version from the style check dependency list.

As for removing python from build -- is there a C++ library that can be built without Python? If so, I would remove python, cython, ... from build and put them in a separate list like build_python. We're trying to achieve a level of modularity/granularity that is useful for minimizing dependencies at each stage of a build.

@isVoid isVoid requested review from harrism and bdice November 17, 2022 21:38
- conda-forge
- nvidia
dependencies:
- cmake>=3.23.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- cmake>=3.23.1
- cmake>=3.23.1,!=3.25.0

ajschmidt8 and others added 5 commits November 18, 2022 12:23
@ajschmidt8
Copy link
Member

I would have expected libcudf and librmm / rmm in this list, too? Avoid making those transitive dependencies if cuspatial directly relies on them via #include or cimport. (If it is only imported in Python, it is a runtime and not a build time dependency.) Also combine this advice with my other comment to split out the C++ and Python build dependencies (we have yet to do this for cudf, still work in progress).

@bdice, I just pushed some changes. I'll defer to you to add these necessary dependencies. Otherwise, I think this PR looks good to me!

@ajschmidt8 ajschmidt8 changed the title Update Dependency List with dependencies.yaml Update Dependency List with dependencies.yaml [skip gpuci] Nov 18, 2022
@github-actions github-actions bot added the gpuCI Related to Continuous Integration / Automation label Nov 18, 2022
@bdice
Copy link
Contributor

bdice commented Nov 18, 2022

@ajschmidt8 Feel free to merge if you think this is ready, then we can start the GitHub Actions migration. I checked the current CI workflows and I think we have already handled all the features that the cuSpatial repo will need.

@ajschmidt8 ajschmidt8 merged commit 04593ef into rapidsai:branch-22.12 Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team conda Related to conda and conda configuration gpuCI Related to Continuous Integration / Automation improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants