feat(lammps): load installed backends dynamically#5707
Conversation
📝 WalkthroughWalkthroughThe PR refactors ChangesLAMMPS environment refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant lmp as deepmd/lmp.py (import time)
participant tfHelper as _get_tensorflow_library_paths
participant ptHelper as _get_pytorch_library_paths
participant env as os.environ
lmp->>tfHelper: get TF lib/preload paths
tfHelper-->>lmp: paths or empty (if TF missing)
lmp->>ptHelper: get PyTorch lib path
ptHelper-->>lmp: path or empty (if torch missing)
lmp->>lmp: compute CUDA paths (Linux)
lmp->>env: set preload_env via get_env
lmp->>env: set lib_env via get_env
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/test_lmp.py (1)
33-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the TF ≥ 2.12 branch.
Current coverage only exercises the
TF_VERSION < 2.12path (libpython preload). Add a complementary test withTF_VERSION="2.12.0"(or later) asserting the preload list stays empty, to guard the version-gated branch in_get_tensorflow_library_pathsagainst regressions.🤖 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 `@source/tests/test_lmp.py` around lines 33 - 54, Add a complementary test for the TF_VERSION >= 2.12 branch in _get_tensorflow_library_paths, similar to test_tensorflow_library_paths_include_libpython_for_old_tensorflow. Mock deepmd.tf.env with TF_VERSION set to 2.12.0 or later and assert the returned TensorFlow library paths are still correct while the libpython preload list is empty, ensuring the version-gated logic does not regress.
🤖 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.
Nitpick comments:
In `@source/tests/test_lmp.py`:
- Around line 33-54: Add a complementary test for the TF_VERSION >= 2.12 branch
in _get_tensorflow_library_paths, similar to
test_tensorflow_library_paths_include_libpython_for_old_tensorflow. Mock
deepmd.tf.env with TF_VERSION set to 2.12.0 or later and assert the returned
TensorFlow library paths are still correct while the libpython preload list is
empty, ensuring the version-gated logic does not regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 256487fb-b134-4331-9a78-ff4abb1c8d12
📒 Files selected for processing (3)
deepmd/lmp.pydoc/install/easy-install.mdsource/tests/test_lmp.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5707 +/- ##
==========================================
- Coverage 81.22% 81.19% -0.04%
==========================================
Files 980 990 +10
Lines 109352 111035 +1683
Branches 4205 4232 +27
==========================================
+ Hits 88821 90153 +1332
- Misses 19011 19355 +344
- Partials 1520 1527 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| def _get_tensorflow_library_paths() -> tuple[list[str], list[str]]: | ||
| """Get TensorFlow library and preload paths when TensorFlow is installed.""" | ||
| try: | ||
| tf_env = import_module("deepmd.tf.env") | ||
| except ModuleNotFoundError as exc: | ||
| if exc.name == "tensorflow": | ||
| return [], [] | ||
| raise | ||
|
|
||
| tf_dir = tf_env.tf.sysconfig.get_lib() | ||
| preload_paths = [] | ||
| if Version(tf_env.TF_VERSION) < Version("2.12"): | ||
| find_libpython = import_module("find_libpython").find_libpython | ||
| libpython = find_libpython() | ||
| if libpython is not None: | ||
| preload_paths.append(libpython) | ||
| return [tf_dir, os.path.join(tf_dir, "python")], preload_paths | ||
|
|
||
|
|
||
| def _get_pytorch_library_paths() -> list[str]: | ||
| """Get PyTorch library paths when PyTorch is installed.""" | ||
| try: | ||
| torch = import_module("torch") | ||
| except ModuleNotFoundError as exc: | ||
| if exc.name == "torch": | ||
| return [] | ||
| raise | ||
| return [os.path.join(torch.__path__[0], "lib")] |
There was a problem hiding this comment.
Non-blocking (test coverage): source/tests/test_lmp.py is a welcome addition, but a few reachable branches of these new helpers aren't exercised. _get_tensorflow_library_paths is only tested with TF 2.11.0, so the TF >= 2.12 path (no libpython preload) and the find_libpython() is None case are untested; _get_pytorch_library_paths's success path (torch installed -> [torch/lib]) is never run because the _configure_lammps_environment tests monkeypatch the helper and the only direct torch test covers the missing-torch case; and the exc.name != "tensorflow"/"torch" re-raise branches (which intentionally propagate unrelated import errors) have no coverage. A couple of small cases would close these.
There was a problem hiding this comment.
Addressed in 4a4995b by adding coverage for the TF >= 2.12 no-libpython-preload path, the TF < 2.12 find_libpython() is None path, the installed PyTorch library path, and the unrelated ModuleNotFoundError re-raise branches. Validated with venv/bin/python -m pytest source/tests/test_lmp.py -q and venv/bin/ruff check . --exclude venv.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/test_lmp.py (1)
57-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract a shared
import_modulefake/fixture.The
fake_import_moduleclosure with anAssertionErrorfallback for unexpected imports is repeated near-identically across these five tests. A small parametrized helper (e.g., mappingmodule_name -> return value or exception) would reduce duplication, but current tests are correct and readable as-is.🤖 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 `@source/tests/test_lmp.py` around lines 57 - 154, The five tests repeat the same `fake_import_module` pattern with only the module-specific return/raise behavior changing. Extract a shared helper or fixture around `fake_import_module` used by `_get_tensorflow_library_paths()` and `_get_pytorch_library_paths()`, ideally driven by a module-name-to-result mapping, so the tests stay concise while preserving the current `AssertionError` fallback for unexpected imports.
🤖 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.
Nitpick comments:
In `@source/tests/test_lmp.py`:
- Around line 57-154: The five tests repeat the same `fake_import_module`
pattern with only the module-specific return/raise behavior changing. Extract a
shared helper or fixture around `fake_import_module` used by
`_get_tensorflow_library_paths()` and `_get_pytorch_library_paths()`, ideally
driven by a module-name-to-result mapping, so the tests stay concise while
preserving the current `AssertionError` fallback for unexpected imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 38afed2c-fb79-4c84-9767-372c267b9b92
📒 Files selected for processing (1)
source/tests/test_lmp.py
Summary
Tests
Fork CI runs:
Summary by CodeRabbit