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
dependencies.yaml Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
isVoid and others added 3 commits November 16, 2022 19:15
dependencies.yaml Outdated Show resolved Hide resolved
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.

dependencies.yaml Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
@isVoid isVoid requested review from harrism and bdice November 17, 2022 21:38
dependencies.yaml Outdated Show resolved Hide resolved
- 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

dependencies.yaml Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
ajschmidt8 and others added 5 commits November 18, 2022 12:23
Co-authored-by: GALI PREM SAGAR <sagarprem75@gmail.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
these will be needed for the GH Actions workflows, so might as well add them now.
@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
dependencies.yaml Outdated Show resolved Hide resolved
@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