Run routing tests selectively based on file changes#924
Open
rgsl888prabhu wants to merge 4 commits intoNVIDIA:mainfrom
Open
Run routing tests selectively based on file changes#924rgsl888prabhu wants to merge 4 commits intoNVIDIA:mainfrom
rgsl888prabhu wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
rgsl888prabhu
added
non-breaking
📝 WalkthroughWalkthroughAdds path-based routing rules to the PR workflow, a utility script that detects routing-related changes, and gates in CI test runners to skip routing tests for PRs that don't modify routing files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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/utils/routing_changed.sh`:
- Around line 12-18: ROUTING_PATTERNS is missing regression routing paths so
changes under regression/routing* are not detected; update the ROUTING_PATTERNS
array to include the regression locations (e.g., add patterns for
'python/cuopt/cuopt/regression/routing/' and 'cpp/.../regression/routing/' or
equivalent regression subpaths used in PR filters) so the routing_changed.sh
detection matches the PR routing filters; modify the ROUTING_PATTERNS variable
accordingly wherever it's defined to include those regression/routing patterns.
- Line 45: The current assignment CHANGED=$(git diff --name-only "${MB}...HEAD"
2>/dev/null) || true swallows git errors and leaves CHANGED empty; change it to
default to a non-empty sentinel when git diff fails so routing tests run (e.g.
CHANGED=$(git diff --name-only "${MB}...HEAD" 2>/dev/null || echo
"__UNKNOWN__")). Update the CHANGED assignment (the command substitution that
invokes git diff) so failures produce a non-empty string that downstream checks
treat as “changed.”
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5f29d26-6838-48f4-90fa-5c8bbdd11b12
📒 Files selected for processing (7)
.github/workflows/pr.yamlci/run_ctests.shci/run_cuopt_pytests.shci/test_cpp.shci/test_python.shci/test_wheel_cuopt.shci/utils/routing_changed.sh
Comment on lines
+12
to
+18
| ROUTING_PATTERNS=( | ||
| 'python/cuopt/cuopt/routing/' | ||
| 'python/cuopt/cuopt/tests/routing/' | ||
| 'cpp/src/routing/' | ||
| 'cpp/include/cuopt/routing/' | ||
| 'cpp/tests/routing/' | ||
| ) |
There was a problem hiding this comment.
Add regression routing paths to the detection set.
ROUTING_PATTERNS misses regression routing paths, while PR routing filters include regression/routing*. A PR changing only regression routing files can incorrectly output false and skip routing tests.
Suggested fix
ROUTING_PATTERNS=(
'python/cuopt/cuopt/routing/'
'python/cuopt/cuopt/tests/routing/'
'cpp/src/routing/'
'cpp/include/cuopt/routing/'
'cpp/tests/routing/'
+ 'regression/routing'
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ROUTING_PATTERNS=( | |
| 'python/cuopt/cuopt/routing/' | |
| 'python/cuopt/cuopt/tests/routing/' | |
| 'cpp/src/routing/' | |
| 'cpp/include/cuopt/routing/' | |
| 'cpp/tests/routing/' | |
| ) | |
| ROUTING_PATTERNS=( | |
| 'python/cuopt/cuopt/routing/' | |
| 'python/cuopt/cuopt/tests/routing/' | |
| 'cpp/src/routing/' | |
| 'cpp/include/cuopt/routing/' | |
| 'cpp/tests/routing/' | |
| 'regression/routing' | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/utils/routing_changed.sh` around lines 12 - 18, ROUTING_PATTERNS is
missing regression routing paths so changes under regression/routing* are not
detected; update the ROUTING_PATTERNS array to include the regression locations
(e.g., add patterns for 'python/cuopt/cuopt/regression/routing/' and
'cpp/.../regression/routing/' or equivalent regression subpaths used in PR
filters) so the routing_changed.sh detection matches the PR routing filters;
modify the ROUTING_PATTERNS variable accordingly wherever it's defined to
include those regression/routing patterns.
ci/utils/routing_changed.sh
Outdated
| exit 0 | ||
| fi | ||
|
|
||
| CHANGED=$(git diff --name-only "${MB}...HEAD" 2>/dev/null) || true |
There was a problem hiding this comment.
Fail safe when git diff cannot be computed.
Line 45 swallows git diff errors and leaves CHANGED empty, which falls through to false (skip tests). On diff failure, this should return true to avoid missing routing test coverage.
Suggested fix
-CHANGED=$(git diff --name-only "${MB}...HEAD" 2>/dev/null) || true
+if ! CHANGED=$(git diff --name-only "${MB}...HEAD" 2>/dev/null); then
+ echo "true"
+ exit 0
+fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/utils/routing_changed.sh` at line 45, The current assignment CHANGED=$(git
diff --name-only "${MB}...HEAD" 2>/dev/null) || true swallows git errors and
leaves CHANGED empty; change it to default to a non-empty sentinel when git diff
fails so routing tests run (e.g. CHANGED=$(git diff --name-only "${MB}...HEAD"
2>/dev/null || echo "__UNKNOWN__")). Update the CHANGED assignment (the command
substitution that invokes git diff) so failures produce a non-empty string that
downstream checks treat as “changed.”
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci/utils/routing_changed.sh (1)
12-18:⚠️ Potential issue | 🟠 MajorAdd regression routing paths to keep test gating aligned.
ROUTING_PATTERNSstill misses regression routing paths, so a PR touching onlyregression/routing*can incorrectly print"false"and skip routing tests.Suggested patch
ROUTING_PATTERNS=( 'python/cuopt/cuopt/routing/' 'python/cuopt/cuopt/tests/routing/' 'cpp/src/routing/' 'cpp/include/cuopt/routing/' 'cpp/tests/routing/' + 'regression/routing' )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/utils/routing_changed.sh` around lines 12 - 18, ROUTING_PATTERNS is missing regression routing entries so changes under regression/routing* are ignored; update the ROUTING_PATTERNS array in routing_changed.sh to include regression routing paths (e.g., add entries like 'regression/routing/' and/or 'regression/routing*') so the gating logic that checks ROUTING_PATTERNS will detect PRs touching regression routing files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ci/utils/routing_changed.sh`:
- Around line 12-18: ROUTING_PATTERNS is missing regression routing entries so
changes under regression/routing* are ignored; update the ROUTING_PATTERNS array
in routing_changed.sh to include regression routing paths (e.g., add entries
like 'regression/routing/' and/or 'regression/routing*') so the gating logic
that checks ROUTING_PATTERNS will detect PRs touching regression routing files.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a465db3-55c1-4be2-b888-a66dc5af3b48
📒 Files selected for processing (1)
ci/utils/routing_changed.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci/utils/routing_changed.sh (1)
12-18:⚠️ Potential issue | 🟠 MajorInclude regression routing paths in
ROUTING_PATTERNS.Line 12-Line 18 still omits regression routing locations, so PRs that only touch regression routing files can incorrectly output
falseand skip routing tests.Suggested fix
ROUTING_PATTERNS=( 'python/cuopt/cuopt/routing/' 'python/cuopt/cuopt/tests/routing/' 'cpp/src/routing/' 'cpp/include/cuopt/routing/' 'cpp/tests/routing/' + 'regression/routing' )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/utils/routing_changed.sh` around lines 12 - 18, ROUTING_PATTERNS currently omits regression routing directories which causes PRs touching only regression routing files to be missed; update the ROUTING_PATTERNS array to include the regression routing locations by adding appropriate pattern entries for the regression subdirectories (i.e., add regression-specific patterns alongside the existing 'python/cuopt/cuopt/routing/', 'python/cuopt/cuopt/tests/routing/', 'cpp/src/routing/', 'cpp/include/cuopt/routing/', and 'cpp/tests/routing/'), ensuring tests that modify regression routing files are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ci/utils/routing_changed.sh`:
- Around line 12-18: ROUTING_PATTERNS currently omits regression routing
directories which causes PRs touching only regression routing files to be
missed; update the ROUTING_PATTERNS array to include the regression routing
locations by adding appropriate pattern entries for the regression
subdirectories (i.e., add regression-specific patterns alongside the existing
'python/cuopt/cuopt/routing/', 'python/cuopt/cuopt/tests/routing/',
'cpp/src/routing/', 'cpp/include/cuopt/routing/', and 'cpp/tests/routing/'),
ensuring tests that modify regression routing files are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 09b55155-198e-470b-a344-fda09944202b
📒 Files selected for processing (1)
ci/utils/routing_changed.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Run routing tests selectively if routing files have changes, else disable the run to reduce overhead of CI.
Checklist