[FEA] Add selective test execution for PR builds#931
[FEA] Add selective test execution for PR builds#931bala-bhargav wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a detector script that sets CUOPT_TEST_COMPONENTS (routing, lp, mip, or all) from diffs or defaults, and updates CI test scripts to conditionally prepare datasets and run only the relevant C++ gtests and Python pytest directories. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/run_ctests.sh`:
- Around line 28-34: The UNIT_TEST and PRESOLVE_TEST blocks currently only allow
running when components contains "mip", which skips them for LP-only runs;
update the conditional in both the UNIT_TEST and PRESOLVE_TEST cases (refer to
the UNIT_TEST and PRESOLVE_TEST labels and the components variable) to also
accept "lp" (e.g., check for "mip" or "lp" inside components) so those tests run
for LP-selected PRs as well.
- Line 25: The case pattern contains a redundant subpattern "MIP_TERMINATION_*"
because the earlier "MIP_*" already matches it; update the case arm that
currently reads 'MIP_*|PROBLEM_*|...|MIP_TERMINATION_*|...' by removing the
"MIP_TERMINATION_*" token so the pattern list no longer contains the unreachable
subpattern (keep all other alternatives unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9450e103-1ed3-4907-9d70-8f2b748e6975
📒 Files selected for processing (6)
ci/detect_test_components.shci/run_ctests.shci/run_cuopt_pytests.shci/test_cpp.shci/test_python.shci/test_wheel_cuopt.sh
bala-bhargav
force-pushed
the
feat/ci-selective-test-execution-861
branch
from
f9cf547 to
e7be4f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/detect_test_components.sh`:
- Around line 17-19: The script's early-return branch checks
CUOPT_TEST_COMPONENTS but doesn't export it, so child processes may not see a
non-exported shell variable; update the conditional in detect_test_components.sh
to export the variable before returning (e.g., call export CUOPT_TEST_COMPONENTS
inside the if [[ -n "${CUOPT_TEST_COMPONENTS:-}" ]]; then block) so the existing
value is preserved for child scripts without changing the branch logic.
In `@ci/test_python.sh`:
- Around line 45-50: The current conditional uses a combined check that causes
both ./datasets/linear_programming/download_pdlp_test_dataset.sh and
./datasets/mip/download_miplib_test_dataset.sh to run when CUOPT_TEST_COMPONENTS
contains either "lp" or "mip"; change the logic to perform separate checks so
each dataset script runs only when its component is requested. Replace the
single if-block that references CUOPT_TEST_COMPONENTS with two conditional
checks: one that runs download_pdlp_test_dataset.sh only when
CUOPT_TEST_COMPONENTS equals "all" or contains "lp", and another that runs
download_miplib_test_dataset.sh only when CUOPT_TEST_COMPONENTS equals "all" or
contains "mip"; keep the rapids-logger fallback message for when neither dataset
is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34c2f55c-352a-4220-9b3d-41df53cfc3f1
📒 Files selected for processing (6)
ci/detect_test_components.shci/run_ctests.shci/run_cuopt_pytests.shci/test_cpp.shci/test_python.shci/test_wheel_cuopt.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- ci/test_cpp.sh
- ci/run_ctests.sh
- ci/test_wheel_cuopt.sh
Add component-level test filtering to CI scripts so that pull requests only run tests relevant to the changed files (routing, LP, or MIP). Nightly builds continue to run all tests. - New: ci/detect_test_components.sh — detects changed components from the PR diff and exports CUOPT_TEST_COMPONENTS - Modified: run_ctests.sh — added should_run_test() to filter gtests by component - Modified: run_cuopt_pytests.sh — select pytest directories by component - Modified: test_cpp.sh, test_python.sh, test_wheel_cuopt.sh — conditional dataset downloads based on detected components Closes NVIDIA#861
bala-bhargav
force-pushed
the
feat/ci-selective-test-execution-861
branch
from
e7be4f5 to
b93bd54
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/detect_test_components.sh`:
- Around line 28-37: The script currently only verifies HEAD~1 exists before
using git diff HEAD~1..HEAD, which can under-select tests on non-merge commits;
update the pre-check to also verify that HEAD is a merge commit (has at least
two parents) before using HEAD~1..HEAD. In the block that checks git rev-parse
--verify HEAD~1 and before running git diff, add a check (e.g., test for HEAD^2
via git rev-parse --verify HEAD^2 or inspect parents) and if that check fails
set CUOPT_TEST_COMPONENTS="all" and return; keep the existing changed_files
variable and subsequent git diff HEAD~1..HEAD logic unchanged otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 334b8f94-22a6-4673-8238-9c35f0b7294f
📒 Files selected for processing (6)
ci/detect_test_components.shci/run_ctests.shci/run_cuopt_pytests.shci/test_cpp.shci/test_python.shci/test_wheel_cuopt.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/test_wheel_cuopt.sh
| # In RAPIDS CI pull-request builds, the branch is a merge commit so | ||
| # HEAD~1..HEAD gives the full PR diff. If the parent isn't reachable | ||
| # (e.g. shallow clone or non-merge workflow), fall back to running all. | ||
| local changed_files | ||
| if ! git rev-parse --verify HEAD~1 &>/dev/null; then | ||
| export CUOPT_TEST_COMPONENTS="all" | ||
| return | ||
| fi | ||
| if ! changed_files=$(git diff --name-only HEAD~1..HEAD 2>/dev/null); then | ||
| # If git diff fails, run all tests to be safe |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full script context to understand the code better
cd /tmp && git ls-files | head -20Repository: NVIDIA/cuopt
Length of output: 124
🏁 Script executed:
# Find and examine the script
find . -name "detect_test_components.sh" -type f 2>/dev/nullRepository: NVIDIA/cuopt
Length of output: 86
🏁 Script executed:
# Check GitHub Actions workflows
find .github/workflows -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -20Repository: NVIDIA/cuopt
Length of output: 505
🏁 Script executed:
# Read the full detect_test_components.sh script
cat -n ci/detect_test_components.shRepository: NVIDIA/cuopt
Length of output: 6237
🏁 Script executed:
# Check the PR workflow configuration
cat -n .github/workflows/pr.yamlRepository: NVIDIA/cuopt
Length of output: 20632
🏁 Script executed:
# Also check test.yaml
cat -n .github/workflows/test.yamlRepository: NVIDIA/cuopt
Length of output: 4785
🏁 Script executed:
# Look for any documentation about how RAPIDS CI handles PR checkouts
find . -name "*.md" -o -name "*.txt" | xargs grep -l "RAPIDS_BUILD_TYPE\|pull-request" 2>/dev/null | head -5Repository: NVIDIA/cuopt
Length of output: 73
🏁 Script executed:
# Check if there are any comments/docs in the script about the merge commit assumption
grep -n "merge\|parent\|HEAD~1" ci/detect_test_components.shRepository: NVIDIA/cuopt
Length of output: 427
🏁 Script executed:
# Search for any references to the merge commit handling in other CI scripts
grep -r "HEAD~1\|merge.*commit" ci/ --include="*.sh" 2>/dev/null | head -10Repository: NVIDIA/cuopt
Length of output: 452
Add a merge-commit check before using HEAD~1..HEAD for PR diff.
The current check at line 32 only verifies that HEAD~1 exists, but doesn't guarantee HEAD is a merge commit. On a single-parent commit, HEAD~1..HEAD shows only that commit, not the full PR diff—potentially under-selecting tests. The code comment already acknowledges non-merge workflows as a fallback case, so add the verification:
Proposed fix
if ! git rev-parse --verify HEAD~1 &>/dev/null; then
export CUOPT_TEST_COMPONENTS="all"
return
fi
+ # Require merge-commit context for HEAD~1..HEAD PR diff assumption.
+ # `git rev-list --parents -n 1 HEAD` returns: <sha> <parent1> [parent2...]
+ if [[ $(git rev-list --parents -n 1 HEAD | awk '{print NF}') -lt 3 ]]; then
+ export CUOPT_TEST_COMPONENTS="all"
+ return
+ fi
if ! changed_files=$(git diff --name-only HEAD~1..HEAD 2>/dev/null); then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/detect_test_components.sh` around lines 28 - 37, The script currently only
verifies HEAD~1 exists before using git diff HEAD~1..HEAD, which can
under-select tests on non-merge commits; update the pre-check to also verify
that HEAD is a merge commit (has at least two parents) before using
HEAD~1..HEAD. In the block that checks git rev-parse --verify HEAD~1 and before
running git diff, add a check (e.g., test for HEAD^2 via git rev-parse --verify
HEAD^2 or inspect parents) and if that check fails set
CUOPT_TEST_COMPONENTS="all" and return; keep the existing changed_files variable
and subsequent git diff HEAD~1..HEAD logic unchanged otherwise.
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Why is this being done in shell scripts?
We already have a GitHub Action that runs on PR CI to do exactly this, and would prefer to not have multiple competing implementations of the same behavior.
cuopt/.github/workflows/pr.yaml
Lines 112 to 114 in d61b196
Hi @jameslamb, The issue stems from multiple solvers under same library, currently routing is in good state and we don't change anything related to it often, so we wanted to reduce routing test overhead. Currently we are are using your approach a whole for the complete test, but here we need to selectively not run part of the tests. Can you please suggest a better approach where we can use existing method to disable the tests? Should we separate these tests into different workflow or is there a way we can pass env variable to enable or disable these tests based on the files ? |
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Can you please suggest a better approach where we can use existing method to disable the tests? Should we separate these tests into different workflow or is there a way we can pass env variable to enable or disable these tests based on the files ?
Yes, definitely!
Looking at this more closely and with the benefit of your description, I do understand how this is a bit different than the top-level "run / don't run an entire workflow" changed-files gate.
I think you could re-use the output of the changed-files workflow for this purpose though. Each of the changed-files groups holds a boolean saying whether or not a group of file patterns was matched.
cuopt/.github/workflows/pr.yaml
Line 296 in d862480
I think you could re-use those to pass environment variables to the code (doing what ci/detect_test_components.sh is attempting to do).
Something like this:
conda-python-tests:
needs: [conda-python-build, changed-files, compute-matrix-filters]
uses: rapidsai/shared-workflows/.github/workflows/conda-python-tests.yaml@main
if: fromJSON(needs.changed-files.outputs.changed_file_groups).test_python_conda
with:
script: |
CUOPT_ROUTING_FILES_CHANGED="${{ fromJSON(needs.changed-files.outputs.changed_file_groups).test_routing }}"
export CUOPT_ROUTING_FILES_CHANGED
ci/test_python.shAnd then handle those variables in test scripts however you want.
Untested, but in theory I think multi-line scripts like this should work: https://github.com/rapidsai/shared-workflows/blob/52b43724e7c9edba0acf675aaf6f526a22b3111f/.github/workflows/conda-python-tests.yaml#L304
If not, you can combine everything together with ;.
Benefits of this approach:
- no need to re-implement the "figure out which files changed in the PR" logic (it is not trivial!)
- less shell code to maintain
- allows the groups to be re-used and combined for other jobs
- puts all of the "which files affect which workflows" logic in one place (
changed-files:configuration inpr.yaml) instead
And whether or not you do that, please do consider how these things will work when the tests scripts are run in workflow NOT triggered by a PR.
| # In RAPIDS CI pull-request builds, the branch is a merge commit so | ||
| # HEAD~1..HEAD gives the full PR diff. If the parent isn't reachable | ||
| # (e.g. shallow clone or non-merge workflow), fall back to running all. | ||
| local changed_files | ||
| if ! git rev-parse --verify HEAD~1 &>/dev/null; then | ||
| export CUOPT_TEST_COMPONENTS="all" | ||
| return | ||
| fi | ||
| if ! changed_files=$(git diff --name-only HEAD~1..HEAD 2>/dev/null); then |
There was a problem hiding this comment.
This is not true. copy-pr-bot copies commits from the PR branch to a branch on the repo called pull-request/{pr-number} and there HEAD is exactly HEAD of the PR branch.
It isn't like in other open-source projects on GitHub where HEAD is a merge commit created by GitHub that merges the PR branch into the target branch.
That is why in the RAPIDS changed-files action, we do some work to figure out what the target branch of the PR was:
I think this check HEAD~1..HEAD is only going to compare between the difference between the latest and previous-to-that commit on the PR branch, which is not what you want.
You may want to look into something like step-security/changed-files, which the existing RAPIDS changed-files action uses:
- https://github.com/rapidsai/shared-actions/blob/f722a09c99e883ccca580bbb7135945d8efb90e0/changed-files/action.yml#L48
- https://github.com/step-security/changed-files
It produces an output listing all the files that changed: https://github.com/step-security/changed-files?tab=readme-ov-file#outputs-
4 participants
Implements component-level selective test execution for pull request CI builds. When a PR only touches routing code, only routing tests and datasets are downloaded/run. Same for LP and MIP. Nightly builds continue running everything unchanged.
#861
Summary by CodeRabbit