Add optional CI job for integration tests with cudf.pandas #5881
Conversation
dantegd
added
improvement
.github/workflows/pr.yaml
Outdated
| @@ -75,6 +76,13 @@ jobs: | |||
| with: | |||
| build_type: pull-request | |||
| script: "ci/test_python_singlegpu.sh" | |||
| conda-python-tests-integration: | |||
There was a problem hiding this comment.
Naming nitpick: can we call it "...-cudf-pandas-integration"? Something to hint at that the integration that is being tested is with cudf.pandas.
| set +e | ||
|
|
||
| rapids-logger "pytest cuml integration" | ||
| ./ci/run_cuml_integration_pytests.sh \ |
There was a problem hiding this comment.
continued naming nitpick: can we call it run_cudf_pandas_integration_pytests.sh? We are in the cuml repo, so "cuml integration tests" feels like a tautology, while "cudf pandas integration testing" tells me something about what we are testing. Without having to go through the layers of shell scripts to then read and understand the meaning of the option in pytest command. Three weeks ago I am not sure I'd have quickly grasped that -p cudf.pandas is the relevant but in the pytest command line and that it enables integration testing.
There was a problem hiding this comment.
That's a good point, will rename it
|
Some naming nitpick comments. Otherwise looks fine to me. The fact that we need a few levels of indirection instead of having |
.github/workflows/pr.yaml
Outdated
| @@ -75,6 +76,13 @@ jobs: | |||
| with: | |||
| build_type: pull-request | |||
| script: "ci/test_python_singlegpu.sh" | |||
| conda-python-tests-integration: | |||
There was a problem hiding this comment.
I was expecting to see something here/in this section of the yaml that marks it as optional. Is it just not done yet or taken care of somewhere else?
There was a problem hiding this comment.
I tried to add it as optional in a few commits like 4cabd82 but seems like I'm doing it wrong, so been asking how to do it and will update the PR accordingly
There was a problem hiding this comment.
It turns out I also don't know/can't quickly find how to do this! I thought it would be easy but apparently not :-/
There was a problem hiding this comment.
Ended up needing the help from AJ and now in the last commit d6979c5 is optional now
|
/merge |
cc @betatim
This PR adds the CI jobs proposed in #5876