Add integration test#354
Conversation
for more information, see https://pre-commit.ci
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR establishes a multi-MPI integration testing framework for LAMMPS compatibility. It introduces environment-specific conda configurations for MPICH and OpenMPI, wires them into parallel GitHub Actions jobs that build environments and run tests, and adds an integration test that validates force calculations on fixed-atom structures. ChangesLAMMPS Integration Testing Infrastructure
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
=======================================
Coverage 89.61% 89.61%
=======================================
Files 13 13
Lines 1204 1204
=======================================
Hits 1079 1079
Misses 125 125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/pipeline.yml (2)
213-257: ⚡ Quick win
integration_openmpiandintegration_mpichare near-identical; consider a matrix.The two jobs differ only by the integration env filename. A
strategy.matrixover the MPI variant would remove the duplication and keep them in sync.♻️ Example consolidation
integration: needs: [black] runs-on: ubuntu-latest strategy: matrix: mpi: [openmpi, mpich] steps: - uses: actions/checkout@v4 - name: Merge integration environment run: | cp .ci_support/environment.yml environment.yml tail --lines=+4 .ci_support/environment-integration-${{ matrix.mpi }}.yml >> environment.yml - name: Setup Mambaforge uses: conda-incubator/setup-miniconda@v3 with: python-version: "3.13" miniforge-version: latest channels: conda-forge conda-remove-defaults: "true" environment-file: environment.yml - name: Test shell: bash -l {0} run: | pip install . --no-deps --no-build-isolation python -m unittest tests/test_compatibility_integration.py🤖 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/pipeline.yml around lines 213 - 257, Replace the duplicated jobs integration_openmpi and integration_mpich with a single parameterized job (e.g., integration) that uses a strategy.matrix over an mpi variable (values: openmpi, mpich); update the Merge Notebook environment step to reference the matrix value (e.g., tail ...-integration-${{ matrix.mpi }}.yml) and keep the Checkout, Setup Mambaforge and Test steps unchanged but using the matrix job name to run both variants. Ensure the new job name (integration) replaces the two originals and that any job dependencies (needs: [black]) remain on the consolidated job.
218-218: 💤 Low valueMisleading step name in the integration jobs.
Both new jobs label the env-merge step
Merge Notebook environment(copy-pasted from thenotebooksjob), which is confusing since these build the LAMMPS integration env.♻️ Suggested rename
- - name: Merge Notebook environment + - name: Merge integration environmentAlso applies to: 241-241
🤖 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/pipeline.yml at line 218, The step label "Merge Notebook environment" is misleading in the two integration jobs; update the step name string to reflect the LAMMPS integration (e.g. "Merge LAMMPS integration environment" or "Merge LAMMPS environment") wherever the literal "Merge Notebook environment" appears in the pipeline job definitions so the step names accurately describe the work being done.tests/test_compatibility_integration.py (1)
31-31: 💤 Low valueUnused unpacked return values.
shell_outputandjob_crashedare never used (Ruff RUF059). Prefix with underscores.♻️ Diff
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _shell_output, parsed_output, _job_crashed = lammps_file_interface_function(🤖 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 `@tests/test_compatibility_integration.py` at line 31, The test unpacks three values from lammps_file_interface_function but only uses parsed_output; rename the unused variables to start with underscores to silence Ruff RUF059—change the call in tests/test_compatibility_integration.py from "shell_output, parsed_output, job_crashed = lammps_file_interface_function(...)" to use "_shell_output, parsed_output, _job_crashed = lammps_file_interface_function(...)" so the function name lammps_file_interface_function and the used variable parsed_output remain unchanged.
🤖 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/workflows/pipeline.yml:
- Around line 230-234: The CI currently runs
tests/test_compatibility_integration.py but a `@unittest.skipIf`(skip_lammps_test,
...) can make the whole test silently skip (skip_lammps_test is set when import
lammps fails), so update the "Test" job to fail when the integration test is
skipped: run unittest with verbosity (python -m unittest
tests/test_compatibility_integration.py -v) and add a post-check that the test
run reported zero skipped tests (or replace the skip guard by failing the job
when skip_lammps_test is true); reference the skip_lammps_test symbol and the
tests/test_compatibility_integration.py file when implementing the check so the
workflow explicitly fails the job if the LAMMPS integration test did not
actually execute.
In `@tests/test_compatibility_integration.py`:
- Around line 45-46: The assertions in tests/test_compatibility_integration.py
use exact equality on floating sums
(np.sum(parsed_output["generic"]["forces"][:, 0]) == 0.0 and [:, 2]) which is
fragile; change these to tolerance-based checks using np.isclose or np.allclose
(e.g. assert np.isclose(np.sum(parsed_output["generic"]["forces"][:, 0]), 0.0,
atol=1e-8)) for the x and z columns (or use np.allclose on a slice like
parsed_output["generic"]["forces"][:, [0,2]]), and either add the y-column check
or a clear comment in the test explaining why the y-component is intentionally
omitted given the FixAtoms constraint.
- Around line 7-13: The test import guard is catching an ImportError because it
tries to import lammps_file_interface_function from a non-existent module
(compatibility.lammps); update the import in
tests/test_compatibility_integration.py to import lammps_file_interface_function
from lammpsparser.compatibility.file (i.e., replace "from
lammpsparser.compatibility.lammps import lammps_file_interface_function" with an
import that points to lammpsparser.compatibility.file), or alternatively add a
re-export in the package so compatibility.lammps exposes
lammps_file_interface_function; ensure the try/except only wraps the external
lammps import and not the local import of lammps_file_interface_function so
skip_lammps_test is set only when the external lammps package is missing.
---
Nitpick comments:
In @.github/workflows/pipeline.yml:
- Around line 213-257: Replace the duplicated jobs integration_openmpi and
integration_mpich with a single parameterized job (e.g., integration) that uses
a strategy.matrix over an mpi variable (values: openmpi, mpich); update the
Merge Notebook environment step to reference the matrix value (e.g., tail
...-integration-${{ matrix.mpi }}.yml) and keep the Checkout, Setup Mambaforge
and Test steps unchanged but using the matrix job name to run both variants.
Ensure the new job name (integration) replaces the two originals and that any
job dependencies (needs: [black]) remain on the consolidated job.
- Line 218: The step label "Merge Notebook environment" is misleading in the two
integration jobs; update the step name string to reflect the LAMMPS integration
(e.g. "Merge LAMMPS integration environment" or "Merge LAMMPS environment")
wherever the literal "Merge Notebook environment" appears in the pipeline job
definitions so the step names accurately describe the work being done.
In `@tests/test_compatibility_integration.py`:
- Line 31: The test unpacks three values from lammps_file_interface_function but
only uses parsed_output; rename the unused variables to start with underscores
to silence Ruff RUF059—change the call in
tests/test_compatibility_integration.py from "shell_output, parsed_output,
job_crashed = lammps_file_interface_function(...)" to use "_shell_output,
parsed_output, _job_crashed = lammps_file_interface_function(...)" so the
function name lammps_file_interface_function and the used variable parsed_output
remain unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f51feee-4d1b-466c-ad3b-3a10e3e7516f
📒 Files selected for processing (4)
.ci_support/environment-integration-mpich.yml.ci_support/environment-integration-openmpi.yml.github/workflows/pipeline.ymltests/test_compatibility_integration.py
| self.assertTrue(np.sum(parsed_output["generic"]["forces"][:, 0]) == 0.0) | ||
| self.assertTrue(np.sum(parsed_output["generic"]["forces"][:, 2]) == 0.0) |
There was a problem hiding this comment.
Avoid exact floating-point equality for summed forces.
np.sum(...) == 0.0 is fragile: after an MD run the column sums are unlikely to be bit-exactly zero, so a tiny residual will fail the assertion. Use a tolerance. Also note only the x and z components are checked—please confirm omitting y is intentional given the alternating FixAtoms constraint.
🛠️ Suggested tolerance-based check
- self.assertTrue(np.sum(parsed_output["generic"]["forces"][:, 0]) == 0.0)
- self.assertTrue(np.sum(parsed_output["generic"]["forces"][:, 2]) == 0.0)
+ forces = parsed_output["generic"]["forces"]
+ self.assertAlmostEqual(float(np.sum(forces[:, 0])), 0.0, places=8)
+ self.assertAlmostEqual(float(np.sum(forces[:, 2])), 0.0, places=8)📝 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.
| self.assertTrue(np.sum(parsed_output["generic"]["forces"][:, 0]) == 0.0) | |
| self.assertTrue(np.sum(parsed_output["generic"]["forces"][:, 2]) == 0.0) | |
| forces = parsed_output["generic"]["forces"] | |
| self.assertAlmostEqual(float(np.sum(forces[:, 0])), 0.0, places=8) | |
| self.assertAlmostEqual(float(np.sum(forces[:, 2])), 0.0, places=8) |
🤖 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 `@tests/test_compatibility_integration.py` around lines 45 - 46, The assertions
in tests/test_compatibility_integration.py use exact equality on floating sums
(np.sum(parsed_output["generic"]["forces"][:, 0]) == 0.0 and [:, 2]) which is
fragile; change these to tolerance-based checks using np.isclose or np.allclose
(e.g. assert np.isclose(np.sum(parsed_output["generic"]["forces"][:, 0]), 0.0,
atol=1e-8)) for the x and z columns (or use np.allclose on a slice like
parsed_output["generic"]["forces"][:, [0,2]]), and either add the y-column check
or a clear comment in the test explaining why the y-component is intentionally
omitted given the FixAtoms constraint.
1 participant
Summary by CodeRabbit
Tests
Chores