test(lmp): avoid duplicate pb conversions in tests#5400
Open
njzjz-bot wants to merge 8 commits intodeepmodeling:masterfrom
Open
test(lmp): avoid duplicate pb conversions in tests#5400njzjz-bot wants to merge 8 commits intodeepmodeling:masterfrom
njzjz-bot wants to merge 8 commits intodeepmodeling:masterfrom
Conversation
Cache converted pb models in source/lmp/tests so repeated imports do not rerun the same pbtxt conversion when the output is already present and up to date. Also add a focused regression test for the helper and switch the affected LAMMPS tests to use it for shared graph outputs. Authored by OpenClaw (model: gpt-5.4)
for more information, see https://pre-commit.ci
Contributor
📝 WalkthroughWalkthroughAdded a new test helper module that performs pbtxt→pb conversion with file-based locking and staleness checks, and replaced many test-module inline subprocess conversions with calls to that helper during module setup (conditioned on TensorFlow enablement where applicable). Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant ModelConvert as model_convert.ensure_converted_pb
participant DeepMD as "deepmd CLI\n(python -m deepmd)"
participant FS as FileSystem
Test->>ModelConvert: ensure_converted_pb(source, output)
ModelConvert->>FS: check if output exists & up-to-date
alt up-to-date
ModelConvert-->>Test: return output
else not up-to-date
ModelConvert->>FS: attempt create lock file (atomic O_EXCL)
alt lock acquired
ModelConvert->>FS: write PID into lock
ModelConvert->>FS: create tmp file in output dir
ModelConvert->>DeepMD: run convert (subprocess)
DeepMD-->>ModelConvert: conversion complete
ModelConvert->>FS: write tmp -> atomic replace output
ModelConvert->>FS: remove tmp (if any) and remove lock
ModelConvert-->>Test: return output
else lock exists
ModelConvert->>FS: poll lock status / check PID liveness / staleness
alt lock becomes stale or removed
ModelConvert->>FS: break stale lock (remove)
ModelConvert->>ModelConvert: retry acquire
else timeout
ModelConvert-->>Test: raise TimeoutError
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
source/lmp/tests/test_lammps_dpa_pt_nopbc.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalAdd missing
subprocessimport used bytest_pair_deepmd_mpi.The
test_pair_deepmd_mpifunction at line 725 callssp.check_call(...), but the import block does not definesp. This causes anF821undefined name error and will fail CI linting checks.Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_dpa_pt_nopbc.py` around lines 2 - 19, The test file is missing the subprocess import used by test_pair_deepmd_mpi (it calls sp.check_call); add the missing import (e.g., import subprocess as sp) to the top import block so that the symbol sp is defined, ensuring test_pair_deepmd_mpi can call sp.check_call without causing an F821 undefined name error.source/lmp/tests/test_lammps_pd.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalRestore the
subprocess as spimport used bytest_pair_deepmd_mpi.The function at line 759 calls
sp.check_call(...)but the import is missing. Ruff reports F821 (undefined name) at that location, which will fail CI.import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_pd.py` around lines 2 - 19, The test file calls sp.check_call(...) in the test_pair_deepmd_mpi flow but never imports subprocess as sp, causing an F821 undefined name; add the missing import "import subprocess as sp" alongside the other stdlib imports (e.g., with import os, shutil, sys, tempfile) so that sp.check_call in the function referenced at line ~759 resolves correctly.source/lmp/tests/test_lammps_spin_nopbc_pt.py (1)
2-18:⚠️ Potential issue | 🔴 CriticalAdd the missing
subprocessimport used by the MPI test.
test_pair_deepmd_mpicallssp.check_call(...)at line 225, but the import block is missingimport subprocess as sp. This causes an F821 linting error and will raiseNameErrorat runtime.Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_spin_nopbc_pt.py` around lines 2 - 18, The test uses subprocess under the name sp in test_pair_deepmd_mpi (calls sp.check_call(...)), but the module is not imported; add the missing import line "import subprocess as sp" to the top-level imports in source/lmp/tests/test_lammps_spin_nopbc_pt.py so that sp is defined and lint/runtime errors are resolved.source/lmp/tests/test_lammps.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalRestore the
subprocessalias used by the MPI test.
test_pair_deepmd_mpicallssp.check_call(...)at line 736, but the import block no longer definessp, causing aNameErrorat runtime and failing linting withruff check(F821 undefined name).import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps.py` around lines 2 - 19, The MPI test test_pair_deepmd_mpi uses the subprocess alias sp (calls sp.check_call(...)) but the import block in source/lmp/tests/test_lammps.py no longer defines sp, causing a NameError; restore the alias by adding an import for subprocess as sp (or re-introduce the sp alias) in the top-level imports so sp.check_call in test_pair_deepmd_mpi resolves correctly, ensuring lint (ruff) and runtime pass.source/lmp/tests/test_lammps_jax.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalRestore the
subprocessalias used by the MPI test.
test_pair_deepmd_mpicallssp.check_call(...)at line 729, but the import block does not definesp. This causes ruff to fail with F821 (undefined name) and the test to raiseNameErrorat runtime.Fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_jax.py` around lines 2 - 19, The test imports removed the subprocess alias used by test_pair_deepmd_mpi, causing sp to be undefined; restore it by adding an import for subprocess with the alias sp (e.g., import subprocess as sp) in the top-level import block so that the test_pair_deepmd_mpi function can call sp.check_call(...) without NameError.source/lmp/tests/test_lammps_dpa_sel_pt.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalRestore the
subprocessalias used by the MPI test.
test_pair_deepmd_mpicallssp.check_call(...)at line 726, but the import block does not definesp, causing a NameError at runtime and failing ruff linting with F821.🐛 Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_dpa_sel_pt.py` around lines 2 - 19, The tests import block is missing the subprocess alias used by test_pair_deepmd_mpi (it calls sp.check_call), causing a NameError; restore the alias by adding an import for subprocess as sp in the top-level imports so that sp is defined for the test (refer to the test function name test_pair_deepmd_mpi and the module source/lmp/tests/test_lammps_dpa_sel_pt.py).source/lmp/tests/test_lammps_spin_pt.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalRestore the
subprocessalias used by the MPI test.
test_pair_deepmd_mpiat line 357 callssp.check_call(...), but the import block does not definesp. This causes an F821 linting error and will raiseNameErrorat runtime if executed.🐛 Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfileAs per coding guidelines,
**/*.py: Install linter and runruff check .before committing changes or the CI will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_spin_pt.py` around lines 2 - 19, The test references the alias sp (used as sp.check_call(...) in test_pair_deepmd_mpi) but the import block doesn’t define sp; add an import for subprocess as sp in the module import section so sp is available for test_pair_deepmd_mpi and any other MPI-related subprocess calls (update the top-level import list where PyLammps, numpy, pytest, etc. are imported).source/lmp/tests/test_lammps_dpa_jax.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalAdd missing
subprocess as spimport to fix ruff F821 error.Line 728 references
sp.check_call(...)without importingsubprocess as sp. Ruff reports this as an undefined name (F821), which will cause CI to fail per the coding guidelines.Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_dpa_jax.py` around lines 2 - 19, The test file references sp.check_call(...) but never imports subprocess as sp, causing an F821 undefined name; add the missing import by adding "import subprocess as sp" near the other top-level imports in source/lmp/tests/test_lammps_dpa_jax.py so that sp.check_call and any other sp usages resolve correctly.source/lmp/tests/test_lammps_spin_nopbc.py (1)
2-18:⚠️ Potential issue | 🔴 CriticalAdd missing subprocess import for MPI test.
Line 378 calls
sp.check_call(...), butimport subprocess as spis missing from the import block. The linter reportsF821 Undefined name 'sp'and the test will fail.Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_spin_nopbc.py` around lines 2 - 18, The test references sp.check_call in source/lmp/tests/test_lammps_spin_nopbc.py but the subprocess alias is not imported, causing an undefined name error; add the missing import by importing subprocess as sp at the top of the file (alongside the other imports) so that sp.check_call(...) used around the MPI invocation (the call at line where sp.check_call is invoked) resolves correctly.source/lmp/tests/test_lammps_pt.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalAdd the missing
subprocessimport alias required by the MPI test.Line 727 calls
sp.check_call(...), but the import section does not definesp. This causes ruff to reportF821 Undefined name 'sp'and will fail the test at runtime.Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_pt.py` around lines 2 - 19, The test references sp.check_call (used in the MPI-related test, e.g., around the sp.check_call call) but there is no subprocess alias defined; add an import for subprocess as sp in the module import block (alongside importlib, os, shutil, sys, tempfile) so sp is defined before the tests run.source/lmp/tests/test_lammps_dpa_pt.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalKeep
import subprocess as spbecause the MPI test still uses it.Line 737 calls
sp.check_call(...), but the file lacks the import. This causes ruff lint error F821 (Undefined name) and runtime failure.🐛 Fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_dpa_pt.py` around lines 2 - 19, The test file is calling sp.check_call (in test_lammps_dpa_pt.py) but never imports subprocess, causing NameError/ruff F821; add "import subprocess as sp" to the imports near the top of test_lammps_dpa_pt.py (alongside other imports like importlib, os, tempfile) so sp.check_call can be resolved at runtime and by linters.source/lmp/tests/test_lammps_spin.py (1)
2-19:⚠️ Potential issue | 🔴 CriticalRestore
import subprocess as spfor the MPI subprocess call.Line 419 uses
sp.check_call(...), but thesubprocess as spimport is missing. Ruff reports error F821 (Undefined namesp), which will fail the linter as required by CI before committing.🐛 Proposed fix
import importlib import os import shutil +import subprocess as sp import sys import tempfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_spin.py` around lines 2 - 19, The test references sp.check_call at runtime (see the MPI call around line 419 in test_lammps_spin.py) but the file no longer imports subprocess as sp, causing an undefined name error; restore the missing import by adding import subprocess as sp near the other top-level imports in source/lmp/tests/test_lammps_spin.py so sp.check_call (and any other sp.* usages) resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lmp/tests/model_convert.py`:
- Around line 43-45: The except FileExistsError block that raises TimeoutError
needs explicit exception chaining to satisfy Ruff B904; update the raise in that
block (the TimeoutError raised referencing lock_file and _LOCK_TIMEOUT_SECONDS)
to use "from None" so the FileExistsError is not shown as the direct cause —
locate the except FileExistsError handler around the lock acquisition
(variables: started, _LOCK_TIMEOUT_SECONDS, lock_file) and change the raise
TimeoutError(...) to raise TimeoutError(...) from None.
In `@source/lmp/tests/test_model_convert.py`:
- Around line 7-8: Replace the pair of consecutive touch() calls that assume
filesystem mtime ordering with explicit deterministic mtimes: import os and
time, choose a base timestamp (e.g., now = int(time.time())), set output mtime
with os.utime(output, (now, now)) and then set source mtime to a strictly later
value (e.g., os.utime(source, (now + 1, now + 1))) so that _is_up_to_date()
reliably sees source as newer than output; update the test code that touches the
files (variables named source and output) accordingly.
---
Outside diff comments:
In `@source/lmp/tests/test_lammps_dpa_jax.py`:
- Around line 2-19: The test file references sp.check_call(...) but never
imports subprocess as sp, causing an F821 undefined name; add the missing import
by adding "import subprocess as sp" near the other top-level imports in
source/lmp/tests/test_lammps_dpa_jax.py so that sp.check_call and any other sp
usages resolve correctly.
In `@source/lmp/tests/test_lammps_dpa_pt_nopbc.py`:
- Around line 2-19: The test file is missing the subprocess import used by
test_pair_deepmd_mpi (it calls sp.check_call); add the missing import (e.g.,
import subprocess as sp) to the top import block so that the symbol sp is
defined, ensuring test_pair_deepmd_mpi can call sp.check_call without causing an
F821 undefined name error.
In `@source/lmp/tests/test_lammps_dpa_pt.py`:
- Around line 2-19: The test file is calling sp.check_call (in
test_lammps_dpa_pt.py) but never imports subprocess, causing NameError/ruff
F821; add "import subprocess as sp" to the imports near the top of
test_lammps_dpa_pt.py (alongside other imports like importlib, os, tempfile) so
sp.check_call can be resolved at runtime and by linters.
In `@source/lmp/tests/test_lammps_dpa_sel_pt.py`:
- Around line 2-19: The tests import block is missing the subprocess alias used
by test_pair_deepmd_mpi (it calls sp.check_call), causing a NameError; restore
the alias by adding an import for subprocess as sp in the top-level imports so
that sp is defined for the test (refer to the test function name
test_pair_deepmd_mpi and the module source/lmp/tests/test_lammps_dpa_sel_pt.py).
In `@source/lmp/tests/test_lammps_jax.py`:
- Around line 2-19: The test imports removed the subprocess alias used by
test_pair_deepmd_mpi, causing sp to be undefined; restore it by adding an import
for subprocess with the alias sp (e.g., import subprocess as sp) in the
top-level import block so that the test_pair_deepmd_mpi function can call
sp.check_call(...) without NameError.
In `@source/lmp/tests/test_lammps_pd.py`:
- Around line 2-19: The test file calls sp.check_call(...) in the
test_pair_deepmd_mpi flow but never imports subprocess as sp, causing an F821
undefined name; add the missing import "import subprocess as sp" alongside the
other stdlib imports (e.g., with import os, shutil, sys, tempfile) so that
sp.check_call in the function referenced at line ~759 resolves correctly.
In `@source/lmp/tests/test_lammps_pt.py`:
- Around line 2-19: The test references sp.check_call (used in the MPI-related
test, e.g., around the sp.check_call call) but there is no subprocess alias
defined; add an import for subprocess as sp in the module import block
(alongside importlib, os, shutil, sys, tempfile) so sp is defined before the
tests run.
In `@source/lmp/tests/test_lammps_spin_nopbc_pt.py`:
- Around line 2-18: The test uses subprocess under the name sp in
test_pair_deepmd_mpi (calls sp.check_call(...)), but the module is not imported;
add the missing import line "import subprocess as sp" to the top-level imports
in source/lmp/tests/test_lammps_spin_nopbc_pt.py so that sp is defined and
lint/runtime errors are resolved.
In `@source/lmp/tests/test_lammps_spin_nopbc.py`:
- Around line 2-18: The test references sp.check_call in
source/lmp/tests/test_lammps_spin_nopbc.py but the subprocess alias is not
imported, causing an undefined name error; add the missing import by importing
subprocess as sp at the top of the file (alongside the other imports) so that
sp.check_call(...) used around the MPI invocation (the call at line where
sp.check_call is invoked) resolves correctly.
In `@source/lmp/tests/test_lammps_spin_pt.py`:
- Around line 2-19: The test references the alias sp (used as sp.check_call(...)
in test_pair_deepmd_mpi) but the import block doesn’t define sp; add an import
for subprocess as sp in the module import section so sp is available for
test_pair_deepmd_mpi and any other MPI-related subprocess calls (update the
top-level import list where PyLammps, numpy, pytest, etc. are imported).
In `@source/lmp/tests/test_lammps_spin.py`:
- Around line 2-19: The test references sp.check_call at runtime (see the MPI
call around line 419 in test_lammps_spin.py) but the file no longer imports
subprocess as sp, causing an undefined name error; restore the missing import by
adding import subprocess as sp near the other top-level imports in
source/lmp/tests/test_lammps_spin.py so sp.check_call (and any other sp.*
usages) resolve correctly.
In `@source/lmp/tests/test_lammps.py`:
- Around line 2-19: The MPI test test_pair_deepmd_mpi uses the subprocess alias
sp (calls sp.check_call(...)) but the import block in
source/lmp/tests/test_lammps.py no longer defines sp, causing a NameError;
restore the alias by adding an import for subprocess as sp (or re-introduce the
sp alias) in the top-level imports so sp.check_call in test_pair_deepmd_mpi
resolves correctly, ensuring lint (ruff) and runtime pass.
🪄 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: 09fb19e0-0159-45b7-af4a-3a260b117e50
📒 Files selected for processing (18)
source/lmp/tests/model_convert.pysource/lmp/tests/test_deeptensor.pysource/lmp/tests/test_dplr.pysource/lmp/tests/test_lammps.pysource/lmp/tests/test_lammps_3types.pysource/lmp/tests/test_lammps_dpa_jax.pysource/lmp/tests/test_lammps_dpa_pt.pysource/lmp/tests/test_lammps_dpa_pt_nopbc.pysource/lmp/tests/test_lammps_dpa_sel_pt.pysource/lmp/tests/test_lammps_faparam.pysource/lmp/tests/test_lammps_jax.pysource/lmp/tests/test_lammps_pd.pysource/lmp/tests/test_lammps_pt.pysource/lmp/tests/test_lammps_spin.pysource/lmp/tests/test_lammps_spin_nopbc.pysource/lmp/tests/test_lammps_spin_nopbc_pt.pysource/lmp/tests/test_lammps_spin_pt.pysource/lmp/tests/test_model_convert.py
Drop the extra helper regression test, restore the subprocess imports still needed by the MPI checks, and make the lock timeout raise explicit for Ruff. Authored by OpenClaw (model: gpt-5.4)
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/lmp/tests/model_convert.py (1)
64-76: Nit: capture subprocess stderr into the exception for debuggability.
sp.check_output(..., stderr=sp.STDOUT)(orsp.run(..., capture_output=True, check=True)) would surface the actualdeepmd convert-from pbtxterror output in CI when the conversion fails. CurrentlyCalledProcessErrorwill only carry stdout, making failures harder to diagnose than the pre-refactor call sites which typically inherited the child's stderr.- sp.check_output( + sp.check_output( [ sys.executable, "-m", "deepmd", "convert-from", "pbtxt", "-i", str(source), "-o", str(tmp_path), - ] + ], + stderr=sp.STDOUT, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/model_convert.py` around lines 64 - 76, The subprocess call using sp.check_output in the test (the block invoking sp.check_output([... sys.executable, "-m", "deepmd", "convert-from", "pbtxt", ...])) does not capture stderr, so CalledProcessError may lack error details; update this invocation to capture stderr by passing stderr=sp.STDOUT (or replace with sp.run(..., capture_output=True, check=True)) so the child's stderr is included in the exception and test logs for easier debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lmp/tests/model_convert.py`:
- Around line 35-48: The loop that acquires lock_file can get permanently wedged
by stale lock files; change the FileExistsError handling to detect and break
stale locks before sleeping: when catching FileExistsError for lock_file, read
the PID inside (if present) and check liveness (e.g., os.kill(pid, 0) on POSIX)
and/or stat(lock_file).st_mtime against _LOCK_TIMEOUT_SECONDS (compare to
time.monotonic() or time.time()); if the PID is not alive or the mtime is older
than _LOCK_TIMEOUT_SECONDS, remove/unlink lock_file and retry the os.open
attempt immediately, otherwise keep the existing timeout/sleep behavior; ensure
this logic lives in the same loop that calls _is_up_to_date and still raises
TimeoutError only after total wait exceeds _LOCK_TIMEOUT_SECONDS.
---
Nitpick comments:
In `@source/lmp/tests/model_convert.py`:
- Around line 64-76: The subprocess call using sp.check_output in the test (the
block invoking sp.check_output([... sys.executable, "-m", "deepmd",
"convert-from", "pbtxt", ...])) does not capture stderr, so CalledProcessError
may lack error details; update this invocation to capture stderr by passing
stderr=sp.STDOUT (or replace with sp.run(..., capture_output=True, check=True))
so the child's stderr is included in the exception and test logs for easier
debugging.
🪄 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: 1db09741-3029-44cb-aea8-f0ae6066e2f9
📒 Files selected for processing (13)
source/lmp/tests/model_convert.pysource/lmp/tests/test_lammps.pysource/lmp/tests/test_lammps_dpa_jax.pysource/lmp/tests/test_lammps_dpa_pt.pysource/lmp/tests/test_lammps_dpa_pt_nopbc.pysource/lmp/tests/test_lammps_dpa_sel_pt.pysource/lmp/tests/test_lammps_jax.pysource/lmp/tests/test_lammps_pd.pysource/lmp/tests/test_lammps_pt.pysource/lmp/tests/test_lammps_spin.pysource/lmp/tests/test_lammps_spin_nopbc.pysource/lmp/tests/test_lammps_spin_nopbc_pt.pysource/lmp/tests/test_lammps_spin_pt.py
🚧 Files skipped from review as they are similar to previous changes (9)
- source/lmp/tests/test_lammps_spin_pt.py
- source/lmp/tests/test_lammps_dpa_pt_nopbc.py
- source/lmp/tests/test_lammps_jax.py
- source/lmp/tests/test_lammps_pd.py
- source/lmp/tests/test_lammps_dpa_pt.py
- source/lmp/tests/test_lammps_spin_nopbc_pt.py
- source/lmp/tests/test_lammps_pt.py
- source/lmp/tests/test_lammps_dpa_sel_pt.py
- source/lmp/tests/test_lammps_dpa_jax.py
Contributor
There was a problem hiding this comment.
Pull request overview
Reduces redundant .pbtxt → .pb conversions during LAMMPS test collection by centralizing conversion logic behind a cached, lock-protected helper.
Changes:
- Added
ensure_converted_pb()helper with mtime-based freshness checks, lock file coordination, and atomic replace. - Refactored multiple LAMMPS test modules to call the helper instead of invoking
deepmd convert-fromdirectly. - Cleaned up now-unneeded
subprocess/sysimports in some test modules.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| source/lmp/tests/model_convert.py | New shared conversion helper with caching + locking to avoid duplicate work/races. |
| source/lmp/tests/test_deeptensor.py | Switched fixture conversion to ensure_converted_pb. |
| source/lmp/tests/test_dplr.py | Switched conversion to helper (and added dipole conversion call). |
| source/lmp/tests/test_lammps.py | Switched conversions to helper. |
| source/lmp/tests/test_lammps_3types.py | Switched conversions to helper and removed direct subprocess usage. |
| source/lmp/tests/test_lammps_dpa_jax.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_dpa_pt.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_dpa_pt_nopbc.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_dpa_sel_pt.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_faparam.py | Switched conversion to helper and removed direct subprocess usage. |
| source/lmp/tests/test_lammps_jax.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_pd.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_pt.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_spin.py | Switched conversions to helper. |
| source/lmp/tests/test_lammps_spin_nopbc.py | Switched conversions to helper. |
| source/lmp/tests/test_lammps_spin_nopbc_pt.py | Switched conversion to helper. |
| source/lmp/tests/test_lammps_spin_pt.py | Switched conversion to helper. |
Comments suppressed due to low confidence (16)
source/lmp/tests/test_lammps.py:233
- The conversions are still executed at import time, before
setup_module()checksENABLE_TENSORFLOW. If conversion can’t run (missing deps) the module import will fail instead of skipping the tests. Consider moving theensure_converted_pb(...)calls intosetup_module()after the skip check (or a fixture).
ensure_converted_pb(pbtxt_file, pb_file)
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_TENSORFLOW", "1") != "1":
pytest.skip(
source/lmp/tests/test_lammps_3types.py:255
- The conversions are executed at module import time, before
setup_module()checksENABLE_TENSORFLOW. That can cause collection/import failures even when the tests should be skipped. Move theensure_converted_pb(...)calls intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file, pb_file)
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_TENSORFLOW", "1") != "1":
pytest.skip(
"Skip test because TensorFlow support is not enabled.",
source/lmp/tests/test_lammps_spin_nopbc.py:220
- The conversions run at import time, before
setup_module()checksENABLE_TENSORFLOW. If conversion deps aren’t available, the module can fail to import even though tests should be skipped. Moveensure_converted_pb(...)intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file, pb_file)
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_TENSORFLOW", "1") != "1":
pytest.skip(
source/lmp/tests/test_lammps_pd.py:236
ensure_converted_pb(...)runs at import time, beforesetup_module()checksENABLE_PADDLE. That means the module can fail to import (conversion deps missing) even though tests are meant to be skipped. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module():
if os.environ.get("ENABLE_PADDLE", "1") != "1":
pytest.skip(
"Skip test because Paddle support is not enabled.",
)
source/lmp/tests/test_lammps_jax.py:235
ensure_converted_pb(...)is called at import time, beforesetup_module()checksENABLE_JAX. If conversion can’t run, import/collection fails rather than skipping. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module():
if os.environ.get("ENABLE_JAX", "1") != "1":
pytest.skip(
"Skip test because JAX support is not enabled.",
)
source/lmp/tests/test_lammps_spin.py:222
- The conversions are executed at import time, before
setup_module()checksENABLE_TENSORFLOW. If conversion can’t run, the module import fails rather than skipping. Consider movingensure_converted_pb(...)intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file, pb_file)
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_TENSORFLOW", "1") != "1":
pytest.skip(
source/lmp/tests/test_lammps_spin_nopbc_pt.py:99
ensure_converted_pb(...)runs at import time beforesetup_module()checksENABLE_PYTORCH. This can make the module fail to import in environments where the test should simply be skipped. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_PYTORCH", "1") != "1":
pytest.skip(
"Skip test because PyTorch support is not enabled.",
source/lmp/tests/test_lammps_pt.py:232
ensure_converted_pb(...)is executed at import time, beforesetup_module()checksENABLE_PYTORCH. If conversion can’t run, collection/import fails instead of skipping. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_PYTORCH", "1") != "1":
pytest.skip(
"Skip test because PyTorch support is not enabled.",
source/lmp/tests/test_lammps_dpa_pt.py:232
ensure_converted_pb(...)is invoked at import time, beforesetup_module()checksENABLE_PYTORCH. If conversion can’t run, the module may fail to import instead of skipping. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_PYTORCH", "1") != "1":
pytest.skip(
"Skip test because PyTorch support is not enabled.",
source/lmp/tests/test_lammps_faparam.py:143
- Conversion runs at import time, before
setup_module()checksENABLE_TENSORFLOW. If conversion depends on unavailable components, the module import can fail even though the test would be skipped. Moveensure_converted_pb(...)intosetup_module()after the skip check (or a fixture).
ensure_converted_pb(pbtxt_file, pb_file)
def setup_module() -> None:
if os.environ.get("ENABLE_TENSORFLOW", "1") != "1":
pytest.skip(
source/lmp/tests/test_lammps_dpa_jax.py:235
ensure_converted_pb(...)runs at import time, beforesetup_module()checksENABLE_JAX. If conversion dependencies aren’t available, the module import can fail even though tests should be skipped. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module():
if os.environ.get("ENABLE_JAX", "1") != "1":
pytest.skip(
"Skip test because JAX support is not enabled.",
)
source/lmp/tests/test_lammps_dpa_pt_nopbc.py:230
ensure_converted_pb(...)is executed at import time, beforesetup_module()checksENABLE_PYTORCH. This can cause import/collection failures even when the test should be skipped. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_PYTORCH", "1") != "1":
pytest.skip(
"Skip test because PyTorch support is not enabled.",
source/lmp/tests/test_lammps_dpa_sel_pt.py:236
ensure_converted_pb(...)runs at import time, beforesetup_module()checksENABLE_PYTORCH. If conversion can’t run, the module import can fail rather than skipping. Move conversion intosetup_module()after the skip guard (or a fixture).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_PYTORCH", "1") != "1":
pytest.skip(
"Skip test because PyTorch support is not enabled.",
)
source/lmp/tests/test_deeptensor.py:67
ensure_converted_pb(...)runs at import time, before theENABLE_TENSORFLOWskip guard insetup_module(). If conversion requires unavailable deps, the module import can fail even though the tests should be skipped. Move the conversion intosetup_module()(after the skip check) or into a fixture that only runs when tests are enabled.
ensure_converted_pb(pbtxt_file, pb_file)
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_TENSORFLOW", "1") != "1":
pytest.skip(
source/lmp/tests/test_dplr.py:275
- The model conversions run at import time (before
setup_module()checksENABLE_TENSORFLOW). This can break test collection in environments where TensorFlow/deepmd conversion isn’t available but the tests are meant to be skipped. Move theseensure_converted_pb(...)calls intosetup_module()after the skip guard (or into a fixture).
ensure_converted_pb(pbtxt_file, pb_file)
ensure_converted_pb(dipole_pbtxt_file, dipole_pb_file)
def setup_module() -> None:
if os.environ.get("ENABLE_TENSORFLOW", "1") != "1":
pytest.skip(
source/lmp/tests/test_lammps_spin_pt.py:191
ensure_converted_pb(...)is invoked at import time, beforesetup_module()checksENABLE_PYTORCH. If conversion can’t run (or deepmd isn’t available) the module import can fail even though the test should be skipped. Move conversion intosetup_module()after the skip guard (or into a fixture used by the tests).
ensure_converted_pb(pbtxt_file2, pb_file2)
def setup_module() -> None:
if os.environ.get("ENABLE_PYTORCH", "1") != "1":
pytest.skip(
"Skip test because PyTorch support is not enabled.",
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move the shared pb conversion calls out of import time so modules can still be collected and skipped cleanly when the required backend is disabled. Also harden the conversion lock handling against stale lock files and switch the conversion subprocess to a non-buffering checked run. Authored by OpenClaw (model: gpt-5.4)
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lmp/tests/model_convert.py`:
- Around line 50-61: In _should_break_stale_lock: don't treat mtime expiry as
definitive; first read the PID via _read_lock_pid(lock_file) and if a PID exists
return False when _pid_is_running(pid) is True, otherwise (PID missing or not
running) fall back to computing lock_age against _LOCK_TIMEOUT_SECONDS; preserve
the FileNotFoundError early return and ensure you only return True for breaking
the lock when either the pid is absent/malformed and the mtime is older than
_LOCK_TIMEOUT_SECONDS or the pid is present but _pid_is_running(pid) is False.
- Around line 106-119: The subprocess invocation using sp.run([...], check=True)
(the call to sp.run that executes sys.executable -m deepmd convert-from ...) is
triggering Ruff S603; add a targeted inline suppression by appending "# noqa:
S603" to that sp.run line to silence the warning since the inputs are
test-controlled and safe, ensuring no other changes to the call or arguments.
In `@source/lmp/tests/test_lammps_dpa_sel_pt.py`:
- Around line 234-235: The MPI model-deviation test is still using pb_file2 even
when TensorFlow is disabled; replicate the same ENABLE_TENSORFLOW check used
before ensure_converted_pb(pbtxt_file2, pb_file2) so the MPI test does not
receive or reference pb_file2 when TF is off. Modify the test_pair_deepmd_mpi
invocation (and the similar block at the other occurrence around the 725-741
region) to either skip that MPI test when os.environ.get("ENABLE_TENSORFLOW",
"1") != "1" or conditionally pass pb_file2 only when ensure_converted_pb was
executed; update references to pbtxt_file2, pb_file2, ensure_converted_pb and
the test_pair_deepmd_mpi call accordingly.
🪄 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: 840eb9d7-bae3-444d-828d-7c112f62deb3
📒 Files selected for processing (17)
source/lmp/tests/model_convert.pysource/lmp/tests/test_deeptensor.pysource/lmp/tests/test_dplr.pysource/lmp/tests/test_lammps.pysource/lmp/tests/test_lammps_3types.pysource/lmp/tests/test_lammps_dpa_jax.pysource/lmp/tests/test_lammps_dpa_pt.pysource/lmp/tests/test_lammps_dpa_pt_nopbc.pysource/lmp/tests/test_lammps_dpa_sel_pt.pysource/lmp/tests/test_lammps_faparam.pysource/lmp/tests/test_lammps_jax.pysource/lmp/tests/test_lammps_pd.pysource/lmp/tests/test_lammps_pt.pysource/lmp/tests/test_lammps_spin.pysource/lmp/tests/test_lammps_spin_nopbc.pysource/lmp/tests/test_lammps_spin_nopbc_pt.pysource/lmp/tests/test_lammps_spin_pt.py
🚧 Files skipped from review as they are similar to previous changes (11)
- source/lmp/tests/test_lammps_dpa_pt_nopbc.py
- source/lmp/tests/test_lammps_spin_nopbc_pt.py
- source/lmp/tests/test_lammps_spin_nopbc.py
- source/lmp/tests/test_lammps_dpa_jax.py
- source/lmp/tests/test_lammps.py
- source/lmp/tests/test_deeptensor.py
- source/lmp/tests/test_lammps_jax.py
- source/lmp/tests/test_lammps_dpa_pt.py
- source/lmp/tests/test_lammps_pd.py
- source/lmp/tests/test_lammps_spin.py
- source/lmp/tests/test_lammps_3types.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5400 +/- ##
==========================================
- Coverage 80.35% 80.35% -0.01%
==========================================
Files 819 819
Lines 85445 85446 +1
Branches 4139 4139
==========================================
Hits 68661 68661
- Misses 15508 15509 +1
Partials 1276 1276 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Tighten stale-lock handling so a live PID wins over mtime expiry, silence the targeted subprocess security lint for the test-controlled conversion command, and skip the DPA select MPI model-deviation test when TensorFlow is disabled. Authored by OpenClaw (model: gpt-5.4)
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/lmp/tests/model_convert.py (1)
106-119:⚠️ Potential issue | 🟡 MinorRestore the targeted Ruff S603 suppression.
Ruff still flags Line 106. Since this helper invokes a fixed, test-controlled command, keep the call but add the inline suppression so CI linting passes. As per coding guidelines,
**/*.py: Install linter and runruff check .before committing changes or the CI will fail.Proposed fix
- sp.run( + sp.run( # noqa: S603 - trusted test helper invokes this Python's deepmd module [ sys.executable, "-m",Verify with:
#!/bin/bash # Description: Confirm the targeted subprocess call no longer triggers Ruff S603. ruff check source/lmp/tests/model_convert.py --select S603🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/model_convert.py` around lines 106 - 119, The subprocess invocation using sp.run([...], check=True) (which calls sys.executable -m deepmd convert-from pbtxt ...) is intentionally safe for tests but is triggering Ruff S603; add an inline Ruff suppression to that call (e.g., append a noqa for S603 on the sp.run line) so the linter ignores this specific test-controlled subprocess usage while preserving the call in function/test_model_convert or wherever sp.run is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/lmp/tests/model_convert.py`:
- Around line 106-119: The subprocess invocation using sp.run([...], check=True)
(which calls sys.executable -m deepmd convert-from pbtxt ...) is intentionally
safe for tests but is triggering Ruff S603; add an inline Ruff suppression to
that call (e.g., append a noqa for S603 on the sp.run line) so the linter
ignores this specific test-controlled subprocess usage while preserving the call
in function/test_model_convert or wherever sp.run is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 992dae47-caa9-4528-adad-ba7f5e6adddc
📒 Files selected for processing (2)
source/lmp/tests/model_convert.pysource/lmp/tests/test_lammps_dpa_sel_pt.py
🚧 Files skipped from review as they are similar to previous changes (1)
- source/lmp/tests/test_lammps_dpa_sel_pt.py
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
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.
Problem
source/lmp/testsconverted several shared.pbtxtfixtures at module import time.graph.pb,graph2.pb, anddeepspin_nlist-2.pb, so collecting those tests reran the same conversion work multiple times.Change
ensure_converted_pb()helper that skips conversion when the output.pbalready exists and is newer than the source.pbtxtNotes
python3 -m unittest source.lmp.tests.test_model_convertpython3 -m compileall source/lmp/tests/model_convert.py source/lmp/tests/test_model_convert.py source/lmp/tests/test_deeptensor.py source/lmp/tests/test_dplr.py source/lmp/tests/test_lammps.py source/lmp/tests/test_lammps_3types.py source/lmp/tests/test_lammps_dpa_jax.py source/lmp/tests/test_lammps_dpa_pt.py source/lmp/tests/test_lammps_dpa_pt_nopbc.py source/lmp/tests/test_lammps_dpa_sel_pt.py source/lmp/tests/test_lammps_faparam.py source/lmp/tests/test_lammps_jax.py source/lmp/tests/test_lammps_pd.py source/lmp/tests/test_lammps_pt.py source/lmp/tests/test_lammps_spin.py source/lmp/tests/test_lammps_spin_nopbc.py source/lmp/tests/test_lammps_spin_nopbc_pt.py source/lmp/tests/test_lammps_spin_pt.pyAuthored by OpenClaw (model: gpt-5.4)
Summary by CodeRabbit