Skip to content

CI: Run extended accel mode tests #6377

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

Merged
merged 10 commits into from
Mar 13, 2025

Conversation

csadorf
Copy link
Contributor

@csadorf csadorf commented Feb 27, 2025

No description provided.

Copy link

copy-pr-bot bot commented Feb 27, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added the ci label Feb 27, 2025
@csadorf csadorf added tests Unit testing for project non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Feb 27, 2025
@csadorf
Copy link
Contributor Author

csadorf commented Feb 27, 2025

/ok to test

@github-actions github-actions bot added conda conda issue Cython / Python Cython or Python issue labels Feb 27, 2025
@csadorf
Copy link
Contributor Author

csadorf commented Feb 27, 2025

/ok to test

1 similar comment
@csadorf
Copy link
Contributor Author

csadorf commented Feb 28, 2025

/ok to test

@csadorf csadorf force-pushed the ci/run-extended-accel-tests branch 2 times, most recently from 9dee6aa to 882fcef Compare March 5, 2025 17:30
@csadorf
Copy link
Contributor Author

csadorf commented Mar 5, 2025

/ok to test

@csadorf csadorf force-pushed the ci/run-extended-accel-tests branch from 882fcef to 2d6df62 Compare March 6, 2025 20:21
@csadorf csadorf marked this pull request as ready for review March 6, 2025 21:02
@csadorf csadorf requested review from a team as code owners March 6, 2025 21:02
@csadorf csadorf requested a review from bdice March 6, 2025 21:02
@csadorf csadorf force-pushed the ci/run-extended-accel-tests branch from 273ee11 to ae86706 Compare March 6, 2025 21:24
@csadorf
Copy link
Contributor Author

csadorf commented Mar 7, 2025

The scikit-learn test suite takes about 30-45mins to complete. The job log is about 30MB long (~250k lines). We have a pass rate of about 88%. That's less than I had measured before, will have to investigate the cause for that. I am going to set the required pass rate at 80% for now.

@bdice This is ready for review.

@github-actions github-actions bot removed conda conda issue Cython / Python Cython or Python issue labels Mar 12, 2025
csadorf added 3 commits March 12, 2025 09:05
…-learn tests. Update related test scripts and documentation to reflect these changes, simplifying the test execution process.
@csadorf csadorf force-pushed the ci/run-extended-accel-tests branch from c63c3b8 to 32a2a33 Compare March 12, 2025 16:05
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.

Flushing comments from earlier — forgot to submit. I will re-review soon.

- `build.sh`
Clones and builds the scikit-learn repository.
Options:
- `--scikit-learn-version` : Specify the scikit-learn version to test (default: 1.5.2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid hardcoded versions in this README. It's easy for these things to slip out of date.


set -eu

SCIKIT_LEARN_VERSION="${SCIKIT_LEARN_VERSION:-1.5.2}"
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 a more natural default would be main, and then pin to 1.5.2 where this script is called (rather than hardcoding the version in this script itself).

git clone --depth 1 https://github.com/scikit-learn/scikit-learn.git ${SCIKIT_LEARN_SRC_PATH}
fi

cd ${SCIKIT_LEARN_SRC_PATH}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Often in RAPIDS we use pushd and popd instead of cd so that CI scripts are a little more composable / less stateful.

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.

Looks good. The latest changes are much clearer. Apologies for the delayed review.

@csadorf
Copy link
Contributor Author

csadorf commented Mar 13, 2025

/merge

@rapids-bot rapids-bot bot merged commit c1391ba into rapidsai:branch-25.04 Mar 13, 2025
73 of 75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci improvement Improvement / enhancement to an existing function non-breaking Non-breaking change tests Unit testing for project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants