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

[FEA] Add third-party library integration testing of cudf.pandas to cudf #16645

Merged

Conversation

Matt711
Copy link
Contributor

@Matt711 Matt711 commented Aug 23, 2024

Description

Closes #16580

Checklist

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

@Matt711 Matt711 added feature request New feature or request non-breaking Non-breaking change cudf.pandas Issues specific to cudf.pandas labels Aug 23, 2024
@Matt711 Matt711 self-assigned this Aug 23, 2024
@github-actions github-actions bot added the Python Affects Python cuDF API. label Aug 23, 2024
Copy link
Contributor Author

@Matt711 Matt711 left a comment

Choose a reason for hiding this comment

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

I think I've migrated the files we need from cudf-pandas-integration (ie. the tests and some of the shell scripts). And I've combined the three jobs before in to one.

I'm not sure if there's a proper way to test these changes, so I'll open the PR for review and get some feedback as it is.

@Matt711 Matt711 marked this pull request as ready for review August 23, 2024 16:58
@Matt711 Matt711 requested review from a team as code owners August 23, 2024 16:58
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I've recommended moving the test from nightly.yaml to test.yaml, but for the lifetime of this PR can we also add it to pr.yaml? That way we can test it out during this PR and iron out any issues, then remove that before the end so that the test only runs nightly.

Copy link

copy-pr-bot bot commented Aug 24, 2024

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/workflows/pr.yaml Outdated Show resolved Hide resolved
@Matt711 Matt711 added the 2 - In Progress Currently a work in progress label Aug 28, 2024
@Matt711 Matt711 force-pushed the feat/cudf-pandas-integration-tests branch from c794216 to 9561ea2 Compare August 28, 2024 22:31
@Matt711
Copy link
Contributor Author

Matt711 commented Aug 29, 2024

/ok to test

@Matt711 Matt711 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Aug 29, 2024
@Matt711 Matt711 requested a review from vyasr August 29, 2024 04:03
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.

I didn't read the tests that closely since we have previously reviewed them. I focused on the integration pieces. This seems fine to me! Thanks for working on this.

Comment on lines +138 to +141
- matrix:
cuda: "11"
packages:
- cuda-version=11.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both cuda: "11" and cuda: "11.8"? We typically do not list major-only versions in our dependencies files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got errors like "Cannot find matching value in matrix with cuda:11/12", so I added both.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

LGTM. I didn't re-review the actual tests since those have been looked at before, just the CI/GHA scripts and dependencies.yaml.

ci/cudf_pandas_scripts/run_tests.sh Show resolved Hide resolved
.github/workflows/pr.yaml Outdated Show resolved Hide resolved
@Matt711
Copy link
Contributor Author

Matt711 commented Aug 29, 2024

/ok to test

@Matt711
Copy link
Contributor Author

Matt711 commented Aug 30, 2024

/merge

@rapids-bot rapids-bot bot merged commit 62a53b3 into rapidsai:branch-24.10 Aug 30, 2024
88 checks passed
rapids-bot bot pushed a commit that referenced this pull request Aug 30, 2024
…CI job (#16704)

Following up #16645, and adding a gpu node type to the nightly CI job

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #16704
vyasr added a commit to rapidsai/workflows that referenced this pull request Sep 3, 2024
rapids-bot bot pushed a commit that referenced this pull request Sep 4, 2024
Following up #16645 with a couple improvements

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #16728
res-life pushed a commit to res-life/cudf that referenced this pull request Sep 11, 2024
Following up rapidsai#16645 with a couple improvements

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: rapidsai#16728
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 cudf.pandas Issues specific to cudf.pandas feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEA] Add integration testing of cudf.pandas
4 participants