Using Git-diff to reduce the time cost of CI/CD#5753
Conversation
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
…nto gitdiff-v2
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
Signed-off-by: Gongxl972 <141223555+Gongxl972@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds two GitHub Actions helper scripts, build-global-mapping.py and detect_affected_tests.py, that generate and consume a JSON mapping from test files to their deepmd dependencies. It rewrites test_python.yml to add early-detection and build-global-mapping jobs and reworks the testpython job to run affected tests incrementally instead of the full suite. ChangesIncremental Python test selection pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
.github/scripts/detect_affected_tests.py (1)
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtraneous f-string prefix.
Line 113 has an f-string with no placeholders.
🔧 Proposed fix
- debug(f"Mapping file not found, falling back to full test") + debug("Mapping file not found, falling back to full test")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/detect_affected_tests.py at line 113, The debug message in detect_affected_tests.py uses an unnecessary f-string prefix in the fallback logging path. Update the debug call in the affected-tests mapping logic to use a plain string instead of an f-string, since there are no interpolated values in that message..github/workflows/test_python.yml (5)
44-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead variable / unused base for push events.
In the non-PR branch
BASE_SHA=$(git merge-base HEAD origin/master)is computed but never used —COMPARE_SHAis set to${{ github.sha }}^. Note the laterbuild-global-mappingstep (Lines 159-165) instead prefersorigin/mainthenorigin/master; the two jobs use different base-resolution logic, which is worth aligning to avoid confusion. Drop the unused assignment here or use it consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_python.yml around lines 44 - 49, The push-event branch in the compare-SHA setup contains an unused BASE_SHA assignment and uses a different base-resolution strategy than the later build-global-mapping logic. Update the compare-SHA selection in the workflow step that sets compare_sha so it either removes the dead BASE_SHA calculation or uses that value consistently, and align its base choice with the origin/main then origin/master fallback used by build-global-mapping to keep the workflow behavior consistent.
461-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTF2 eager-mode step ignores incremental selection.
This step always runs
source/tests/consistent/io/test_io.py source/jax2tf_testswhenevermatrix.group == 1and not skipped, even if the incremental mapping selected unrelated paths. That's a correctness gap versus the incremental intent (and redundant cost), though harmless to results. Consider gating it on whether those paths are affected, or accept it as an intentional always-run smoke check and document that.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_python.yml around lines 461 - 466, The Test TF2 eager mode step in the workflow currently ignores the incremental selection and always runs the fixed pytest targets, so update the job logic around this step to either gate it on whether source/tests/consistent/io/test_io.py or source/jax2tf_tests were selected by the incremental mapping, or explicitly mark it as an intentional always-run smoke test. Use the existing step_check_skip output and the Test TF2 eager mode step name to keep the behavior consistent with the incremental workflow.
17-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit least-privilege
permissions:block.Only
testpythonsetspermissions;early-detection,build-global-mapping,update_durations, andpassfall back to the repository default token scope (flagged by zizmorexcessive-permissions). Add a top-levelpermissions: contents: read(overriding per-job where more is needed, e.g.id-token: writefor Codecov OIDC) to minimize the token surface. Optionally setpersist-credentials: falseon theactions/checkoutsteps that don't push, per zizmorartipacked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_python.yml around lines 17 - 21, Add an explicit top-level least-privilege permissions block in the workflow so jobs like early-detection, build-global-mapping, update_durations, and pass do not inherit the default token scope. Set the workflow default to contents: read in test_python.yml, then override only the jobs that need more access (for example, testpython for id-token: write if required by Codecov OIDC). Also review the actions/checkout steps in the affected jobs and set persist-credentials: false where the job does not need to push.Source: Linters/SAST tools
450-455: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
--clean-durationsin incremental mode erases durations for tests not run.
--store-durations --clean-durationsprunes any entry not exercised in the current invocation. In incremental mode$SELECTED_PATHSis a small subset, so this run's.test_durationsdrops all other tests;update_durationsthen merges these truncated per-split files viajq -s add, progressively shrinking the duration DB and degradingleast_durationsplitting balance. Consider only storing durations on full-suite runs, or dropping--clean-durationsfor incremental runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_python.yml around lines 450 - 455, The incremental pytest invocation in the test workflow is pruning the duration database because the same command uses both store_durations and clean_durations while running only $SELECTED_PATHS. Update the pytest command in the workflow so the logic around the selected-paths run does not clean durations, or gate --clean-durations so it is used only on full-suite runs; keep the reference point as the pytest command in the workflow and the update_durations flow that merges .test_durations files.
250-285: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftIncremental savings are undercut by 24-way fan-out with full dependency install before scope is known.
The matrix spawns 12 groups × 2 Python versions = 24 jobs, and each installs the full TF/PyTorch/JAX/Paddle stack (Lines 279-285) before
determine_scope/affecteddecide there may be nothing to run (skip_all=true). For small/doc-only PRs this pays the heaviest cost (install + runner startup ×24) even when the test step is skipped, which partly negates the git-diff optimization. Consider gating job creation (or reducing the split count) on the affected-test outcome computed earlier, e.g. compute selection inearly-detection/build-global-mappingand short-circuit the matrix whenskip_all.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test_python.yml around lines 250 - 285, The Python test workflow is creating too many jobs and installing the full dependency stack before it knows whether any tests are needed. Update the `test_python.yml` workflow around the `strategy.matrix`, `determine_scope`, and `affected` flow so the matrix jobs are only created or run when `skip_all` is false, and consider lowering the split count if needed. Keep the heavy installs in the `steps` tied to the `test_python` job, but add an early short-circuit based on the selection computed by `early-detection`/`build-global-mapping` so doc-only or empty-change PRs do not fan out across all 24 jobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/build-global-mapping.py:
- Around line 26-27: The test collection logic in build_global_mapping only
handles ast.FunctionDef, so async tests named test_* are skipped. Update the AST
scan to also recognize ast.AsyncFunctionDef in the same collection path that
appends to test_functions, so async test functions are included in the mapping
output alongside regular test functions.
- Around line 30-50: The import-to-dependency mapping in build-global-mapping.py
does not account for package modules resolved through __init__.py, so tests can
be missed for package-level changes. Update the logic in the AST import handling
so imports like import deepmd and from deepmd.xxx import yyy prefer
source/.../__init__.py when that package file exists, and only fall back to
source/... .py when appropriate. Keep the fix localized to the dependency
collection branch that builds dep and checks os.path.exists, and make sure both
the ast.Import and ast.ImportFrom paths resolve package __init__.py correctly.
- Around line 51-52: The SyntaxError handling in build-global-mapping.py is
swallowing malformed test files, which causes them to disappear from the mapping
without any visibility. Update the exception path in the file-scanning logic to
log the SyntaxError with enough context (at least the affected file name)
instead of silently passing, so CI output shows when a file is skipped; keep the
rest of the mapping flow unchanged around the existing parsing and filtering
logic.
In @.github/scripts/detect_affected_tests.py:
- Around line 137-167: The change detection logic in detect_affected_tests.py
has a gap where files that do not match the existing source/test/workflow paths
are ignored, which can incorrectly lead to skip_all=true. Update the main
changed_files handling in the affected-tests selection flow to add a catch-all
default case after the existing source/deepmd/ and config/workflow checks, so
unrecognized paths like build/config/dependency files are treated as requiring
full test execution. Use the existing affected_tests,
new_source_files_without_tests, and the final skip_all/full-test decision logic
to ensure the fallback routes to need_full_test=true instead of silently
skipping.
In @.github/workflows/test_python.yml:
- Around line 430-455: The pytest invocation in the test step is masking
failures because of the trailing `|| true`, causing the job and downstream
`alls-green` gate to pass even when tests fail. Remove the `|| true` from the
`Run tests via pytest` step so the step exits non-zero on failures, and rely on
the existing `if: always()` post-processing steps to continue running; use the
`pytest` command block in this workflow step as the place to fix it.
- Around line 294-321: The syntax-check step in the test workflow is
incrementing SYNTAX_ERRORS inside a piped while loop, so the update happens in a
subshell and is lost before the final check. Update the shell block in the
“Syntax check changed source files” step to avoid piping into the while loop, or
make the loop fail fast by exiting immediately when python -m py_compile fails,
so the workflow correctly reports syntax errors.
- Around line 479-487: The “Save failed tests cache” step is reading from
.test_durations, but that file only contains timing data and never pytest
failure markers, so the failed-tests cache is always cleared. Update the
workflow step to source failures from pytest output instead, using a pytest
artifact such as junitxml or last-failed data, and keep the FAILED_TESTS_FILE
generation logic tied to that failure source; if no reliable failure source is
available, remove this step entirely.
---
Nitpick comments:
In @.github/scripts/detect_affected_tests.py:
- Line 113: The debug message in detect_affected_tests.py uses an unnecessary
f-string prefix in the fallback logging path. Update the debug call in the
affected-tests mapping logic to use a plain string instead of an f-string, since
there are no interpolated values in that message.
In @.github/workflows/test_python.yml:
- Around line 44-49: The push-event branch in the compare-SHA setup contains an
unused BASE_SHA assignment and uses a different base-resolution strategy than
the later build-global-mapping logic. Update the compare-SHA selection in the
workflow step that sets compare_sha so it either removes the dead BASE_SHA
calculation or uses that value consistently, and align its base choice with the
origin/main then origin/master fallback used by build-global-mapping to keep the
workflow behavior consistent.
- Around line 461-466: The Test TF2 eager mode step in the workflow currently
ignores the incremental selection and always runs the fixed pytest targets, so
update the job logic around this step to either gate it on whether
source/tests/consistent/io/test_io.py or source/jax2tf_tests were selected by
the incremental mapping, or explicitly mark it as an intentional always-run
smoke test. Use the existing step_check_skip output and the Test TF2 eager mode
step name to keep the behavior consistent with the incremental workflow.
- Around line 17-21: Add an explicit top-level least-privilege permissions block
in the workflow so jobs like early-detection, build-global-mapping,
update_durations, and pass do not inherit the default token scope. Set the
workflow default to contents: read in test_python.yml, then override only the
jobs that need more access (for example, testpython for id-token: write if
required by Codecov OIDC). Also review the actions/checkout steps in the
affected jobs and set persist-credentials: false where the job does not need to
push.
- Around line 450-455: The incremental pytest invocation in the test workflow is
pruning the duration database because the same command uses both store_durations
and clean_durations while running only $SELECTED_PATHS. Update the pytest
command in the workflow so the logic around the selected-paths run does not
clean durations, or gate --clean-durations so it is used only on full-suite
runs; keep the reference point as the pytest command in the workflow and the
update_durations flow that merges .test_durations files.
- Around line 250-285: The Python test workflow is creating too many jobs and
installing the full dependency stack before it knows whether any tests are
needed. Update the `test_python.yml` workflow around the `strategy.matrix`,
`determine_scope`, and `affected` flow so the matrix jobs are only created or
run when `skip_all` is false, and consider lowering the split count if needed.
Keep the heavy installs in the `steps` tied to the `test_python` job, but add an
early short-circuit based on the selection computed by
`early-detection`/`build-global-mapping` so doc-only or empty-change PRs do not
fan out across all 24 jobs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 65d2d48c-fce4-463d-8e48-490dc26de898
📒 Files selected for processing (3)
.github/scripts/build-global-mapping.py.github/scripts/detect_affected_tests.py.github/workflows/test_python.yml
| if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"): | ||
| test_functions.append(node.name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing ast.AsyncFunctionDef — async test functions won't be collected.
If any test files define async def test_*(...), those functions won't appear in the mapping's test_functions list.
🔧 Proposed fix
- if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"):
+ if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith("test_"):
test_functions.append(node.name)📝 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.
| if isinstance(node, ast.FunctionDef) and node.name.startswith("test_"): | |
| test_functions.append(node.name) | |
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith("test_"): | |
| test_functions.append(node.name) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/build-global-mapping.py around lines 26 - 27, The test
collection logic in build_global_mapping only handles ast.FunctionDef, so async
tests named test_* are skipped. Update the AST scan to also recognize
ast.AsyncFunctionDef in the same collection path that appends to test_functions,
so async test functions are included in the mapping output alongside regular
test functions.
| if isinstance(node, ast.Import): | ||
| for alias in node.names: | ||
| if alias.name.startswith("deepmd"): | ||
| module_path = alias.name.replace(".", "/") + ".py" | ||
| dep = "source/" + module_path | ||
| dependencies.append(dep) | ||
|
|
||
| # collect: from deepmd.xxx import yyy | ||
| elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"): | ||
| base = node.module.replace(".", "/") | ||
| if node.names: | ||
| for alias in node.names: | ||
| if alias.name == "*": | ||
| dep = f"source/{base}.py" | ||
| else: | ||
| dep = f"source/{base}/{alias.name}.py" | ||
|
|
||
| if not os.path.exists(dep): | ||
| dep = f"source/{base}.py" | ||
|
|
||
| dependencies.append(dep) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Package import resolution doesn't handle __init__.py files, causing missed test selections.
import deepmd maps to source/deepmd.py (line 33-34), but deepmd is a package — the actual file is source/deepmd/__init__.py. Changes to __init__.py won't match this path, so dependent tests won't be selected.
Similarly, from deepmd.xxx import yyy where yyy is a class/function defined in a package's __init__.py falls back only to source/deepmd/xxx.py (line 48), missing source/deepmd/xxx/__init__.py.
🔧 Proposed fix for package import resolution
if isinstance(node, ast.Import):
for alias in node.names:
if alias.name.startswith("deepmd"):
module_path = alias.name.replace(".", "/")
- dep = "source/" + module_path + ".py"
- dependencies.append(dep)
+ # Package: try __init__.py first, then module .py
+ dep_pkg = f"source/{module_path}/__init__.py"
+ dep_mod = f"source/{module_path}.py"
+ if os.path.exists(dep_pkg):
+ dependencies.append(dep_pkg)
+ else:
+ dependencies.append(dep_mod)
elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"):
base = node.module.replace(".", "/")
if node.names:
for alias in node.names:
if alias.name == "*":
dep = f"source/{base}.py"
else:
dep = f"source/{base}/{alias.name}.py"
if not os.path.exists(dep):
- dep = f"source/{base}.py"
+ dep_init = f"source/{base}/__init__.py"
+ if os.path.exists(dep_init):
+ dep = dep_init
+ else:
+ dep = f"source/{base}.py"
dependencies.append(dep)📝 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.
| if isinstance(node, ast.Import): | |
| for alias in node.names: | |
| if alias.name.startswith("deepmd"): | |
| module_path = alias.name.replace(".", "/") + ".py" | |
| dep = "source/" + module_path | |
| dependencies.append(dep) | |
| # collect: from deepmd.xxx import yyy | |
| elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"): | |
| base = node.module.replace(".", "/") | |
| if node.names: | |
| for alias in node.names: | |
| if alias.name == "*": | |
| dep = f"source/{base}.py" | |
| else: | |
| dep = f"source/{base}/{alias.name}.py" | |
| if not os.path.exists(dep): | |
| dep = f"source/{base}.py" | |
| dependencies.append(dep) | |
| if isinstance(node, ast.Import): | |
| for alias in node.names: | |
| if alias.name.startswith("deepmd"): | |
| module_path = alias.name.replace(".", "/") | |
| # Package: try __init__.py first, then module .py | |
| dep_pkg = f"source/{module_path}/__init__.py" | |
| dep_mod = f"source/{module_path}.py" | |
| if os.path.exists(dep_pkg): | |
| dependencies.append(dep_pkg) | |
| else: | |
| dependencies.append(dep_mod) | |
| # collect: from deepmd.xxx import yyy | |
| elif isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("deepmd"): | |
| base = node.module.replace(".", "/") | |
| if node.names: | |
| for alias in node.names: | |
| if alias.name == "*": | |
| dep = f"source/{base}.py" | |
| else: | |
| dep = f"source/{base}/{alias.name}.py" | |
| if not os.path.exists(dep): | |
| dep_init = f"source/{base}/__init__.py" | |
| if os.path.exists(dep_init): | |
| dep = dep_init | |
| else: | |
| dep = f"source/{base}.py" | |
| dependencies.append(dep) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/build-global-mapping.py around lines 30 - 50, The
import-to-dependency mapping in build-global-mapping.py does not account for
package modules resolved through __init__.py, so tests can be missed for
package-level changes. Update the logic in the AST import handling so imports
like import deepmd and from deepmd.xxx import yyy prefer source/.../__init__.py
when that package file exists, and only fall back to source/... .py when
appropriate. Keep the fix localized to the dependency collection branch that
builds dep and checks os.path.exists, and make sure both the ast.Import and
ast.ImportFrom paths resolve package __init__.py correctly.
| except SyntaxError: | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
SyntaxError is silently swallowed — malformed test files vanish from the mapping.
A test file with a syntax error returns empty lists, gets filtered out at line 90, and won't appear in the mapping. If the file is later fixed but the mapping cache isn't rebuilt, tests for that file won't be selected.
At minimum, log the error so it's visible in CI output.
🔧 Proposed fix
- except SyntaxError:
- pass
+ except SyntaxError as e:
+ print(f"Syntax error parsing {test_file}: {e}", file=sys.stderr)📝 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.
| except SyntaxError: | |
| pass | |
| except SyntaxError as e: | |
| print(f"Syntax error parsing {test_file}: {e}", file=sys.stderr) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/build-global-mapping.py around lines 51 - 52, The
SyntaxError handling in build-global-mapping.py is swallowing malformed test
files, which causes them to disappear from the mapping without any visibility.
Update the exception path in the file-scanning logic to log the SyntaxError with
enough context (at least the affected file name) instead of silently passing, so
CI output shows when a file is skipped; keep the rest of the mapping flow
unchanged around the existing parsing and filtering logic.
| for cf in changed_files: | ||
| debug(f"Processing changed file: {cf}") | ||
|
|
||
| # Case A: Changed file is a test file itself | ||
| if cf.startswith("source/tests/"): | ||
| debug(f"Changed file is a test file, adding to affected tests: {cf}") | ||
| affected_tests.add(cf) | ||
| continue | ||
|
|
||
| # Case B: Changed file is a workflow/config file - trigger full test | ||
| if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"): | ||
| debug(f"Changed workflow/script file {cf}, triggering full test") | ||
| print("selected_paths=source/tests") | ||
| print("skip_all=false") | ||
| print("need_full_test=true") | ||
| sys.exit(0) | ||
|
|
||
| # Case C: Changed file is a source file, look for dependent tests | ||
| if cf.startswith("source/deepmd/"): | ||
| found_match = False | ||
| for test_file, meta in mapping.items(): | ||
| dependencies = meta.get("dependencies", []) | ||
| if cf in dependencies: | ||
| debug(f"Found test {test_file} depends on {cf}, adding to affected tests") | ||
| affected_tests.add(test_file) | ||
| found_match = True | ||
|
|
||
| if not found_match: | ||
| debug(f"Changed source file {cf} has no test coverage, marking for full test") | ||
| new_source_files_without_tests.append(cf) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unrecognized changed files are silently ignored, leading to skip_all=true and tests being skipped.
Changed files outside source/tests/, .github/workflows/, .github/scripts/, and source/deepmd/ (e.g., pyproject.toml, source/setup.py, CMakeLists.txt, requirements.txt) don't match any case. If all changed files fall into this gap, both affected_tests and new_source_files_without_tests remain empty, hitting the else branch at line 182 which sets skip_all=true — all tests are skipped.
This is a safe-default inversion: unrecognized changes should trigger full tests, not skip them.
🔧 Proposed fix — add a catch-all default case
for cf in changed_files:
debug(f"Processing changed file: {cf}")
# Case A: Changed file is a test file itself
if cf.startswith("source/tests/"):
debug(f"Changed file is a test file, adding to affected tests: {cf}")
affected_tests.add(cf)
continue
# Case B: Changed file is a workflow/config file - trigger full test
if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"):
debug(f"Changed workflow/script file {cf}, triggering full test")
print("selected_paths=source/tests")
print("skip_all=false")
print("need_full_test=true")
sys.exit(0)
# Case C: Changed file is a source file, look for dependent tests
if cf.startswith("source/deepmd/"):
found_match = False
for test_file, meta in mapping.items():
dependencies = meta.get("dependencies", [])
if cf in dependencies:
debug(f"Found test {test_file} depends on {cf}, adding to affected tests")
affected_tests.add(test_file)
found_match = True
if not found_match:
debug(f"Changed source file {cf} has no test coverage, marking for full test")
new_source_files_without_tests.append(cf)
+ continue
+
+ # Case D: Unrecognized file — safe default is full test
+ debug(f"Unrecognized changed file {cf}, triggering full test")
+ print("selected_paths=source/tests")
+ print("skip_all=false")
+ print("need_full_test=true")
+ sys.exit(0)📝 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.
| for cf in changed_files: | |
| debug(f"Processing changed file: {cf}") | |
| # Case A: Changed file is a test file itself | |
| if cf.startswith("source/tests/"): | |
| debug(f"Changed file is a test file, adding to affected tests: {cf}") | |
| affected_tests.add(cf) | |
| continue | |
| # Case B: Changed file is a workflow/config file - trigger full test | |
| if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"): | |
| debug(f"Changed workflow/script file {cf}, triggering full test") | |
| print("selected_paths=source/tests") | |
| print("skip_all=false") | |
| print("need_full_test=true") | |
| sys.exit(0) | |
| # Case C: Changed file is a source file, look for dependent tests | |
| if cf.startswith("source/deepmd/"): | |
| found_match = False | |
| for test_file, meta in mapping.items(): | |
| dependencies = meta.get("dependencies", []) | |
| if cf in dependencies: | |
| debug(f"Found test {test_file} depends on {cf}, adding to affected tests") | |
| affected_tests.add(test_file) | |
| found_match = True | |
| if not found_match: | |
| debug(f"Changed source file {cf} has no test coverage, marking for full test") | |
| new_source_files_without_tests.append(cf) | |
| for cf in changed_files: | |
| debug(f"Processing changed file: {cf}") | |
| # Case A: Changed file is a test file itself | |
| if cf.startswith("source/tests/"): | |
| debug(f"Changed file is a test file, adding to affected tests: {cf}") | |
| affected_tests.add(cf) | |
| continue | |
| # Case B: Changed file is a workflow/config file - trigger full test | |
| if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"): | |
| debug(f"Changed workflow/script file {cf}, triggering full test") | |
| print("selected_paths=source/tests") | |
| print("skip_all=false") | |
| print("need_full_test=true") | |
| sys.exit(0) | |
| # Case C: Changed file is a source file, look for dependent tests | |
| if cf.startswith("source/deepmd/"): | |
| found_match = False | |
| for test_file, meta in mapping.items(): | |
| dependencies = meta.get("dependencies", []) | |
| if cf in dependencies: | |
| debug(f"Found test {test_file} depends on {cf}, adding to affected tests") | |
| affected_tests.add(test_file) | |
| found_match = True | |
| if not found_match: | |
| debug(f"Changed source file {cf} has no test coverage, marking for full test") | |
| new_source_files_without_tests.append(cf) | |
| continue | |
| # Case D: Unrecognized file — safe default is full test | |
| debug(f"Unrecognized changed file {cf}, triggering full test") | |
| print("selected_paths=source/tests") | |
| print("skip_all=false") | |
| print("need_full_test=true") | |
| sys.exit(0) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/detect_affected_tests.py around lines 137 - 167, The change
detection logic in detect_affected_tests.py has a gap where files that do not
match the existing source/test/workflow paths are ignored, which can incorrectly
lead to skip_all=true. Update the main changed_files handling in the
affected-tests selection flow to add a catch-all default case after the existing
source/deepmd/ and config/workflow checks, so unrecognized paths like
build/config/dependency files are treated as requiring full test execution. Use
the existing affected_tests, new_source_files_without_tests, and the final
skip_all/full-test decision logic to ensure the fallback routes to
need_full_test=true instead of silently skipping.
| - name: Syntax check changed source files | ||
| run: | | ||
| set -x | ||
| echo "==========================================" | ||
| echo "SYNTAX CHECKING CHANGED SOURCE FILES" | ||
| echo "==========================================" | ||
|
|
||
| COMPARE_SHA="${{ needs.early-detection.outputs.compare_sha }}" | ||
| CHANGED_FILES=$(git diff --name-only $COMPARE_SHA ${{ github.sha }}) | ||
|
|
||
| SYNTAX_ERRORS=0 | ||
| echo "$CHANGED_FILES" | grep '^source/deepmd/.*\.py$' | while read -r pyfile; do | ||
| if [ -f "$pyfile" ]; then | ||
| echo "Checking syntax of $pyfile..." | ||
| python -m py_compile "$pyfile" || { | ||
| echo "SYNTAX ERROR in $pyfile" | ||
| SYNTAX_ERRORS=$((SYNTAX_ERRORS + 1)) | ||
| } | ||
| fi | ||
| done | ||
|
|
||
| if [ $SYNTAX_ERRORS -gt 0 ]; then | ||
| echo "Found $SYNTAX_ERRORS syntax errors!" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "All changed source files passed syntax check" | ||
| echo "==========================================" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow section around the cited lines
sed -n '280,335p' .github/workflows/test_python.yml
# Confirm the shell used by this workflow job if it's declared nearby
rg -n "shell:|runs-on:|name: Syntax check changed source files|early-detection" .github/workflows/test_python.yml
# Small read-only probe: demonstrate whether a pipeline-fed while loop can preserve a variable in bash
python3 - <<'PY'
import subprocess, textwrap, json, os, sys
script = r'''
SYNTAX_ERRORS=0
printf '%s\n' a | while read -r x; do
SYNTAX_ERRORS=$((SYNTAX_ERRORS + 1))
done
printf 'after_pipeline=%s\n' "$SYNTAX_ERRORS"
SYNTAX_ERRORS=0
while read -r x; do
SYNTAX_ERRORS=$((SYNTAX_ERRORS + 1))
done < <(printf '%s\n' a)
printf 'after_process_substitution=%s\n' "$SYNTAX_ERRORS"
'''
res = subprocess.run(["bash", "-lc", script], capture_output=True, text=True)
print(res.stdout)
print(res.stderr, file=sys.stderr)
PYRepository: deepmodeling/deepmd-kit
Length of output: 4078
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import subprocess, textwrap, sys
scripts = {
"pipeline": r'''
SYNTAX_ERRORS=0
printf '%s\n' a | while read -r x; do
SYNTAX_ERRORS=$((SYNTAX_ERRORS + 1))
done
printf '%s\n' "$SYNTAX_ERRORS"
''',
"redirect": r'''
SYNTAX_ERRORS=0
while read -r x; do
SYNTAX_ERRORS=$((SYNTAX_ERRORS + 1))
done <<EOF
a
EOF
printf '%s\n' "$SYNTAX_ERRORS"
''',
}
for name, script in scripts.items():
res = subprocess.run(
["bash", "--noprofile", "--norc", "-lc", script],
capture_output=True,
text=True,
)
print(f"== {name} ==")
print("rc:", res.returncode)
print("stdout:", res.stdout.strip())
print("stderr:", res.stderr.strip())
PYRepository: deepmodeling/deepmd-kit
Length of output: 241
Avoid piping into the while loop here SYNTAX_ERRORS is updated in the pipeline subshell, so the increment is discarded and the final if never trips. Feed the loop via redirection/process substitution, or exit 1 directly on py_compile failure.
🧰 Tools
🪛 zizmor (1.26.1)
[info] 301-301: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test_python.yml around lines 294 - 321, The syntax-check
step in the test workflow is incrementing SYNTAX_ERRORS inside a piped while
loop, so the update happens in a subshell and is lost before the final check.
Update the shell block in the “Syntax check changed source files” step to avoid
piping into the while loop, or make the loop fail fast by exiting immediately
when python -m py_compile fails, so the workflow correctly reports syntax
errors.
| - name: Run tests via pytest | ||
| if: steps.step_check_skip.outputs.skip_all != 'true' | ||
| timeout-minutes: 180 | ||
| run: | | ||
| echo "STARTING PYTEST" | ||
| echo "=============================================" | ||
|
|
||
| if [ "${{ needs.early-detection.outputs.force_full_test }}" == "true" ] || [ "${{ steps.affected.outputs.need_full_test }}" == "true" ]; then | ||
| echo "MODE: FULL TEST SUITE" | ||
| SELECTED_PATHS="source/tests" | ||
| else | ||
| echo "MODE: INCREMENTAL TEST" | ||
| SELECTED_PATHS="${{ steps.affected.outputs.selected_paths }}" | ||
| fi | ||
|
|
||
| echo "Selected paths: $SELECTED_PATHS" | ||
|
|
||
| export PYTHONPATH=$GITHUB_WORKSPACE:$PYTHONPATH | ||
| echo "PYTHONPATH configured: $PYTHONPATH" | ||
|
|
||
| pytest -vv --cov=deepmd $SELECTED_PATHS \ | ||
| --splits 12 --group ${{ matrix.group }} \ | ||
| --store-durations --clean-durations \ | ||
| --durations-path=.test_durations \ | ||
| --splitting-algorithm least_duration \ | ||
| --disable-warnings || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Critical: || true on the pytest run silently swallows all test failures.
With pytest ... --disable-warnings || true, the step always exits 0, so a failing test suite will not fail the testpython job, and the pass job (alls-green) will report green. This defeats the purpose of CI gating. The post-processing steps that follow already use if: always(), so || true is not needed to keep them running.
🛠️ Proposed fix
pytest -vv --cov=deepmd $SELECTED_PATHS \
--splits 12 --group ${{ matrix.group }} \
--store-durations --clean-durations \
--durations-path=.test_durations \
--splitting-algorithm least_duration \
- --disable-warnings || true
+ --disable-warnings📝 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.
| - name: Run tests via pytest | |
| if: steps.step_check_skip.outputs.skip_all != 'true' | |
| timeout-minutes: 180 | |
| run: | | |
| echo "STARTING PYTEST" | |
| echo "=============================================" | |
| if [ "${{ needs.early-detection.outputs.force_full_test }}" == "true" ] || [ "${{ steps.affected.outputs.need_full_test }}" == "true" ]; then | |
| echo "MODE: FULL TEST SUITE" | |
| SELECTED_PATHS="source/tests" | |
| else | |
| echo "MODE: INCREMENTAL TEST" | |
| SELECTED_PATHS="${{ steps.affected.outputs.selected_paths }}" | |
| fi | |
| echo "Selected paths: $SELECTED_PATHS" | |
| export PYTHONPATH=$GITHUB_WORKSPACE:$PYTHONPATH | |
| echo "PYTHONPATH configured: $PYTHONPATH" | |
| pytest -vv --cov=deepmd $SELECTED_PATHS \ | |
| --splits 12 --group ${{ matrix.group }} \ | |
| --store-durations --clean-durations \ | |
| --durations-path=.test_durations \ | |
| --splitting-algorithm least_duration \ | |
| --disable-warnings || true | |
| - name: Run tests via pytest | |
| if: steps.step_check_skip.outputs.skip_all != 'true' | |
| timeout-minutes: 180 | |
| run: | | |
| echo "STARTING PYTEST" | |
| echo "=============================================" | |
| if [ "${{ needs.early-detection.outputs.force_full_test }}" == "true" ] || [ "${{ steps.affected.outputs.need_full_test }}" == "true" ]; then | |
| echo "MODE: FULL TEST SUITE" | |
| SELECTED_PATHS="source/tests" | |
| else | |
| echo "MODE: INCREMENTAL TEST" | |
| SELECTED_PATHS="${{ steps.affected.outputs.selected_paths }}" | |
| fi | |
| echo "Selected paths: $SELECTED_PATHS" | |
| export PYTHONPATH=$GITHUB_WORKSPACE:$PYTHONPATH | |
| echo "PYTHONPATH configured: $PYTHONPATH" | |
| pytest -vv --cov=deepmd $SELECTED_PATHS \ | |
| --splits 12 --group ${{ matrix.group }} \ | |
| --store-durations --clean-durations \ | |
| --durations-path=.test_durations \ | |
| --splitting-algorithm least_duration \ | |
| --disable-warnings |
🧰 Tools
🪛 zizmor (1.26.1)
[info] 437-437: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 437-437: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 442-442: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test_python.yml around lines 430 - 455, The pytest
invocation in the test step is masking failures because of the trailing `||
true`, causing the job and downstream `alls-green` gate to pass even when tests
fail. Remove the `|| true` from the `Run tests via pytest` step so the step
exits non-zero on failures, and rely on the existing `if: always()`
post-processing steps to continue running; use the `pytest` command block in
this workflow step as the place to fix it.
| - name: Save failed tests cache | ||
| if: always() | ||
| run: | | ||
| FAILED_TESTS_FILE=".failed_tests_${{ matrix.python }}_${{ matrix.group }}" | ||
| if grep -q "FAILED" .test_durations; then | ||
| grep "FAILED" .test_durations | sed 's/.*FAILED //' | sed 's/::.*//' | sort -u > "$FAILED_TESTS_FILE" | ||
| else | ||
| rm -f "$FAILED_TESTS_FILE" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test_python workflow: relevant sections ---'
sed -n '300,360p' .github/workflows/test_python.yml
printf '%s\n' '---'
sed -n '460,500p' .github/workflows/test_python.yml
printf '%s\n' '--- search for .test_durations usage ---'
rg -n '\.test_durations|FAILED_TESTS_FILE|failed_tests' .github/workflows/test_python.yml
printf '%s\n' '--- search for pytest-split / durations generation in repo ---'
rg -n 'pytest-split|test_durations|durations' .Repository: deepmodeling/deepmd-kit
Length of output: 5192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- failed-tests cache references ---'
rg -n '\.failed_tests_|failed-tests-' .github workflows . || true
printf '%s\n' '--- test scope / rerun logic around failed-tests cache ---'
sed -n '360,460p' .github/workflows/test_python.yml
printf '%s\n' '--- pytest-split config / docs in repo ---'
rg -n 'durations-path|pytest-split|test_durations|failed tests|last-failed|junitxml' pyproject.toml .github README* docs* . || trueRepository: deepmodeling/deepmd-kit
Length of output: 6835
🌐 Web query:
pytest-split --store-durations durations-path JSON nodeid seconds failure status
💡 Result:
The pytest-split plugin stores test execution durations in a JSON file to facilitate splitting test suites into balanced groups [1][2]. When you use the --store-durations flag, the plugin generates (or updates) this file at the location specified by --durations-path (which defaults to.test_durations in the current working directory) [3]. The JSON file is structured as a simple dictionary (key-value pairs) where [3]: - The key is the test node ID (a string uniquely identifying the test). - The value is the test duration in seconds (a float). Example of the JSON content: { "tests/test_file.py::test_example": 0.12345, "tests/test_file.py::test_another": 0.05678 } The plugin does not natively store the "failure status" in this specific durations file; its primary purpose is to record the execution time of tests to optimize future test suite splitting algorithms (such as the default duration_based_chunks) [1][3]. If a test is not found in this file during a subsequent run, pytest-split estimates its duration based on the average of stored test execution times [1][2].
Citations:
- 1: https://jerry-git.github.io/pytest-split/
- 2: https://github.com/jerry-git/pytest-split/
- 3: https://github.com/jerry-git/pytest-split/blob/40fcefbea693b35360f23ac4c420bfeb173abdd0/src/pytest_split/plugin.py
Save failed tests cache from pytest failures, not .test_durations. .test_durations is only a JSON map of nodeid -> seconds, so grep "FAILED" will never hit and .failed_tests_* will always be removed. Capture failures from pytest directly (for example --junitxml or --last-failed), or drop this step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test_python.yml around lines 479 - 487, The “Save failed
tests cache” step is reading from .test_durations, but that file only contains
timing data and never pytest failure markers, so the failed-tests cache is
always cleared. Update the workflow step to source failures from pytest output
instead, using a pytest artifact such as junitxml or last-failed data, and keep
the FAILED_TESTS_FILE generation logic tied to that failure source; if no
reliable failure source is available, remove this step entirely.
| dep = f"source/{base}.py" | ||
|
|
||
| dependencies.append(dep) | ||
| except SyntaxError: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5753 +/- ##
==========================================
- Coverage 79.62% 78.87% -0.75%
==========================================
Files 1014 1014
Lines 115422 115420 -2
Branches 4274 4276 +2
==========================================
- Hits 91901 91037 -864
- Misses 21981 22834 +853
- Partials 1540 1549 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
iProzd
left a comment
There was a problem hiding this comment.
Thanks for tackling CI time — the three-stage structure (early-detection → mapping → sharded run) is a solid exploration. Before this can land, though, I think the design needs to be re-anchored on one principle:
Test selection must never gate a merge; it may only speed up PR-iteration feedback.
This repo already uses a GitHub merge queue (merge_group trigger). That gives us the right place to be strict: keep the FULL suite as the required check at merge_group (and on push to master), and only apply incremental selection on pull_request. With that split, an imperfect selector can at most cost a contributor one extra iteration — it can never let a regression reach master. As written, the selection logic also runs for non-PR events, which weakens the final gate itself.
Two more principles that must hold regardless of the selector's cleverness:
- Fail-closed: anything the selector cannot prove is irrelevant → run FULL. Today the default is inverted (unmatched changes fall through to
skip_all=true). - The dependency signal must be sound. Static top-level
import deepmd.*scanning is an under-approximation: it misses transitive deps, registry/string-key dispatch (descriptors/models/fittings resolved from JSON keys), cross-backendconsistent/+universal/parity tests, and all non-Python (C++/CUDA/LAMMPS/data) changes. For a numerically-sensitive multi-backend library this will silently skip real regressions. Prefer coverage-trace selection (pytest-testmon, which this fork is even named after) or a conservative path→backend-directory mapping over per-file import scanning.
Blocking items are in the inline comments (wrong source/deepmd/ path prefix, skip_all safe-default inversion, || true swallowing failures, non-Python changes not covered). Also worth doing first: the biggest, zero-risk win is caching/reusing the compiled build (build one wheel per Python version instead of recompiling the C++ extension in all 24 shards) — that likely saves more wall-clock than selection, with no correctness trade-off. Happy to pair on the merge-queue split.
| sys.exit(0) | ||
|
|
||
| # Case C: Changed file is a source file, look for dependent tests | ||
| if cf.startswith("source/deepmd/"): |
There was a problem hiding this comment.
[blocking] Wrong path prefix — real source changes never match, so normal PRs skip all tests.
The package lives at repo root deepmd/ (there is no source/deepmd/; the workflow itself runs pytest --cov=deepmd). But git diff --name-only emits source changes as deepmd/..., while this check tests source/deepmd/. A PR that only edits deepmd/** therefore matches none of Case A/B/C, both affected_tests and new_source_files_without_tests stay empty, and we hit the else → skip_all=true → the entire Python suite is skipped and pass goes green. The same wrong prefix is in build-global-mapping.py (dep = "source/" + module_path) and in the workflow's new_source_files / syntax-check greps (^source/deepmd/). Note the asymmetry: tests are correctly source/tests/, only the package prefix is wrong. Fix: use deepmd/ everywhere for the package.
| print(f"selected_paths={selected}") | ||
| print("skip_all=false") | ||
| print("need_full_test=false") | ||
| else: |
There was a problem hiding this comment.
[blocking] Safe-default is inverted (fail-open). When no rule matches, the selector currently skips tests. A merge gate must fail-closed: anything not provably irrelevant → FULL. Please remove the free skip_all=true path; only allow skipping for an explicit docs-only allowlist (e.g. all changed files match *.md). Every other bucket — unmatched paths, non-Python, data/fixtures, build/CI config, new files, deletes/renames, missing or unparsable mapping — must set need_full_test=true.
| continue | ||
|
|
||
| # Case B: Changed file is a workflow/config file - trigger full test | ||
| if cf.startswith(".github/workflows/") or cf.startswith(".github/scripts/"): |
There was a problem hiding this comment.
[blocking] Non-Python / C++ / build changes are not covered. Besides the workflow/scripts handled here, changes under source/{lib,op,api_cc,api_c,lmp}/**, pyproject.toml, CMakeLists.txt, source/install/**, and test data/fixtures (*.pth, *.pbtxt, *.npy) match no case and fall through to skip_all=true — yet many Python tests validate behavior through the compiled ops. These need an analogous force-FULL branch. (Covers CodeRabbit's pyproject.toml/CMakeLists.txt point too.)
| --store-durations --clean-durations \ | ||
| --durations-path=.test_durations \ | ||
| --splitting-algorithm least_duration \ | ||
| --disable-warnings || true |
There was a problem hiding this comment.
[blocking] Two issues here:
|| truemakes the step always exit 0, so failing tests never fail the job andalls-greenreports green — CI stops gating. The follow-up steps already useif: always(), so drop|| true.- Selection should be gated to PRs only. Wrap the detect/incremental path in
if: github.event_name == 'pull_request', and forceselected_paths=source/tests(FULL) formerge_groupandpush. That keeps the merge-queue gate authoritative while still giving fast PR feedback.
Modify the test_python.ymlworkflow and add several Python helper scripts under .github/scripts/.
The new scripts discover all test files, analyze test-to-source dependencies, and generate a .global_test_mapping.jsoncache file.
Subsequent test runs will only execute tests mapped to changed source files, reducing CI runtime.
Summary by CodeRabbit
New Features
Bug Fixes