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

Re-enable cuspatial tests and dependency. #1253

Merged
merged 16 commits into from
Oct 5, 2023
Merged

Re-enable cuspatial tests and dependency. #1253

merged 16 commits into from
Oct 5, 2023

Conversation

thomcom
Copy link
Contributor

@thomcom thomcom commented Aug 7, 2023 •
edited
Loading

Description

A poorly understood cuproj->cuspatial dependency required that cuspatial be removed from cuproj's tests during the 23.08->23.10 merge rebase. This PR re-enables those tests.

Checklist

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

@thomcom thomcom self-assigned this Aug 7, 2023
@thomcom thomcom requested a review from a team as a code owner August 7, 2023 17:13
@thomcom thomcom requested review from trxcllnt and isVoid August 7, 2023 17:13
@github-actions github-actions bot added the Python Related to Python code label Aug 7, 2023
@thomcom thomcom requested review from harrism and removed request for trxcllnt August 7, 2023 17:13
@thomcom thomcom added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 7, 2023
@thomcom thomcom requested a review from a team as a code owner August 7, 2023 17:55
@isVoid
Copy link
Contributor

isVoid commented Aug 10, 2023 •
edited
Loading

I was a bit out of loop of this in 23.08, in which PR did cuproj 23.08 tests become disabled?

@thomcom
Copy link
Contributor Author

thomcom commented Aug 10, 2023

They weren't disabled in 23.08, they were disabled in the 23.08 -> 23.10 merge commit because of the dependency that cuproj has on cuspatial, which caused ci to load the cuspatial-nightly build instead of the in-PR build of cuspatial, running cuproj 23.10 tests against cuspatial 23.08.

harrism
harrism reviewed Aug 14, 2023
View reviewed changes
dependencies.yaml Outdated
@@ -442,3 +442,4 @@ dependencies:
- pytest-cov
- pytest-xdist
- geopandas>=0.11.0
- cuspatial=23.10.*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- cuspatial=23.10.*
- cuspatial==23.10.*

And re-run pre-commit.

@copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 21, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the conda Related to conda and conda configuration label Sep 21, 2023
@github-actions github-actions bot removed the conda Related to conda and conda configuration label Sep 21, 2023
@harrism
Copy link
Member

harrism commented Sep 25, 2023

/ok to test

@github-actions github-actions bot added the conda Related to conda and conda configuration label Sep 27, 2023
@harrism
Copy link
Member

harrism commented Sep 28, 2023

@thomcom your .devcontainer folder doesn't match the latest for 23.10, which has separate folders:

.devcontainer/
├── cuda11.8-conda
│   └── devcontainer.json
├── cuda11.8-pip
│   └── devcontainer.json
├── cuda12.0-conda
│   └── devcontainer.json
├── cuda12.0-pip
│   └── devcontainer.json
├── Dockerfile
└── README.md

@harrism
Copy link
Member

harrism commented Sep 28, 2023

Please remember:

/ok to test

@harrism
Copy link
Member

harrism commented Sep 28, 2023

/ok to test

@thomcom
Copy link
Contributor Author

thomcom commented Sep 28, 2023

/ok to test

@harrism
Copy link
Member

harrism commented Oct 4, 2023

/ok to test

harrism
harrism requested changes Oct 4, 2023
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

One dependency request. Remember to run pre-commit run before committing when you make this change.

dependencies.yaml Outdated Show resolved Hide resolved
@thomcom @harrism
Update dependencies.yaml
5d31269 
Co-authored-by: Mark Harris <[email protected]>
@harrism
Copy link
Member

harrism commented Oct 4, 2023

/ok to test

@harrism
Copy link
Member

harrism commented Oct 4, 2023

/ok to test

@github-actions github-actions bot added the ci label Oct 5, 2023
@github-actions github-actions bot removed the conda Related to conda and conda configuration label Oct 5, 2023
@harrism
Copy link
Member

harrism commented Oct 5, 2023

/ok to test

@harrism
Copy link
Member

harrism commented Oct 5, 2023

/ok to test

@bdice
Copy link
Contributor

bdice commented Oct 5, 2023

/merge

@rapids-bot rapids-bot bot merged commit 6c9ac11 into rapidsai:branch-23.10 Oct 5, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isVoid isVoid isVoid approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@bdice bdice bdice approved these changes

@raydouglass raydouglass raydouglass approved these changes

@harrism harrism harrism approved these changes

Assignees

@thomcom thomcom

Labels
ci improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to Python code
Projects
cuSpatial
Status: Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@thomcom @isVoid @harrism @bdice @raydouglass @ajschmidt8