Run relevant CI tests based on what's changed in the ChangeList #2396
Conversation
rlratzel
added
improvement
ci/test.sh
Outdated
| PRHTTP=https://api.github.com/repos/rapidsai/cugraph/pulls/${PR_ID}/files | ||
| fnames=`curl -sb -X GET -H "Accept: application/vnd.github.v3+json" -H "Authorization: token $GHTK" $PRHTTP | python3 -c "import sys, json; print([labels['filename'] for labels in json.load(sys.stdin)])"` | ||
| cpp_changed="false" python_changed="false" nb_changed="false" doc_changed="false" | ||
| for fname in ${fnames[@]} | ||
| do | ||
| if [[ "$fname" == *"cpp/cmake/"* || "$fname" == *"cpp/CMakeLists.txt"* || "$fname" == *"cpp/src/"* || "$fname" == *"cpp/include/"* || "$fname" == *"cpp/tests/"* || "$fname" == *"cpp/libcugraph_etl/"* || "$fname" == *"cpp/scripts/"* ]]; then | ||
| cpp_changed="true" python_changed="true" nb_changed="true" doc_changed="true" | ||
| fi | ||
| if [[ "$fname" == *"python/"* ]]; then | ||
| python_changed="true" nb_changed="true" doc_changed="true" | ||
| fi | ||
| if [[ "$fname" == *"docs/"* ]]; then | ||
| doc_changed="true" | ||
| fi | ||
| if [[ "$fname" == *"notebooks/"* ]]; then | ||
| nb_changed="true" | ||
| fi | ||
| done | ||
|
|
||
| if [[ "$nb_changed" == "false" ]]; then | ||
| export NB_CHANGED="FALSE" | ||
| else | ||
| export NB_CHANGED="TRUE" | ||
| fi | ||
|
|
There was a problem hiding this comment.
I think this code would be more appropriate in ci/gpu/build.sh and have the variables exported and then referenced in the lower-level scripts (like you're doing for the test-notebooks.sh script). The main reason being that test.sh is sometimes also used by devs outside of CI, and this code would prevent that due to how it's trying to access info about a PR. It would also mean you wouldn't have to source test.sh (which can sometimes have unintended side-effects) since the variables would be set in the top-level calling script.
Once this code is moved out, test.sh should be written to also handle cases where the various python_changed, nb_changed, etc. variables are not set, so devs can continue to run test.sh outside of CI.
There was a problem hiding this comment.
Got it, thanks Rick
ci/test.sh
Outdated
| cpp_changed="true" python_changed="true" nb_changed="true" doc_changed="true" | ||
| fi | ||
| if [[ "$fname" == *"python/"* ]]; then | ||
| python_changed="true" nb_changed="true" doc_changed="true" |
There was a problem hiding this comment.
It might be better to make the variable names a little more accurate, since python_changed implies python code changed when it could actually be set by way of a C++ code change. Perhaps the naming convention could be run_python_tests, run_nb_tests, etc.?
ci/gpu/build.sh
Outdated
| ${WORKSPACE}/ci/gpu/test-notebooks.sh 2>&1 | tee nbtest.log | ||
| gpuci_logger "Ran cuGraph notebook test script : return code was: $?, gpu/build.sh exit code is now: $EXITCODE" | ||
| python ${WORKSPACE}/ci/utils/nbtestlog2junitxml.py nbtest.log | ||
| fi | ||
| fi | ||
|
|
||
| if [ -n "${CODECOV_TOKEN}" ]; then |
There was a problem hiding this comment.
I believe the codecov step below will fail if python tests aren't run, since the python tests generate the output needed by codecov. You might try adding something to this if block to prevent codecov from running if python tests did not run.
There was a problem hiding this comment.
Got it, thanks Rick
ci/test.sh
Outdated
| else | ||
| export NB_CHANGED="TRUE" | ||
| fi | ||
|
|
||
| export RAPIDS_DATASET_ROOT_DIR=${RAPIDS_DATASET_ROOT_DIR:-${CUGRAPH_ROOT}/datasets} |
There was a problem hiding this comment.
Based on the CI failures, it looks like this isn't being set properly beause CUGRAPH_ROOT isn't being set properly, because (I think) this script is now being source'd . I think my other suggestion would eliminate the need to source this script and should fix that problem.
ci/test.sh
Outdated
| else | ||
| export NB_CHANGED="TRUE" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Somewhere around here might be a good place to print a summary of what's going to be run, what's going to be skipped, and (most importantly) why, since it might be confusing to devs who are expecting certain tests to run which end up getting skipped.
ci/gpu/build.sh
Outdated
| # Identify relevant testsets to run in CI based on the ChangeList | ||
| ################################################################################ | ||
|
|
||
| PRHTTP=https://api.github.com/repos/rapidsai/cugraph/pulls/${PR_ID}/files |
There was a problem hiding this comment.
This file is used in more than just PR builds. It's also used for nightly branch builds. For those builds, there is no PR_ID number available so these changes will fail.
There was a problem hiding this comment.
Thanks @ajschmidt8 , how to distinguish the nightly builds from the PR builds then? Based on that, I can add an 'if condition' and hide the new code under that 'if block'.
There was a problem hiding this comment.
if [ "$BUILD_MODE" = "pull-request" ]; then
fi
ci/gpu/build.sh
Outdated
| run_cpp_tests="true" run_python_tests="true" run_nb_tests="true" | ||
| fi | ||
| # this will not do anything if the 'fnames' array is empty | ||
| for fname in ${fnames[@]} |
There was a problem hiding this comment.
| for fname in ${fnames[@]} | |
| for fname in "${fnames[@]}" |
Add some quotes here
|
@gpucibot merge |
cpp_changed==cpp+python+notebooks, python_changed==python+notebooks, notebooks_changed==notebooks