Test macos #381
Test macos #381
Conversation
| Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 36 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughAdds an optional hostname_localhost parameter across wrapper constructors (ASE, concurrent, extended) and forwards it into executor/LammpsBase initialization; updates tests to pass the new flag and adjust one ASE test to skip on Darwin. Also adds two macOS GitHub Actions jobs to run MPICH and OpenMPI MPI tests. Changes
Sequence Diagram(s)sequenceDiagram autonumber participant T as Test / Client participant ASE as LammpsASELibrary participant Lib as LammpsLibrary participant Con as LammpsConcurrent participant Exec as SingleNodeExecutor participant LMP as LammpsBase / MPI T->>ASE: __init__(..., hostname_localhost) ASE->>Lib: build/initialize(..., hostname_localhost) Lib->>Con: __init__(..., hostname_localhost) Con->>Exec: init_function(..., hostname_localhost) Exec->>LMP: start(..., hostname_localhost) Note over LMP,Exec: hostname_localhost propagated for localhost resolution/binding Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 #381 +/- ## ======================================= Coverage 85.17% 85.17% ======================================= Files 6 6 Lines 796 796 ======================================= Hits 678 678 Misses 118 118 ☔ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_ase_interface.py (1)
25-34: Set hostname_localhost on LammpsLibrary instantiations
Includehostname_localhost=Truewhen constructing LammpsLibrary so the flag is honored:- library=LammpsLibrary(cores=2), + library=LammpsLibrary(cores=2, hostname_localhost=True),Apply to all
LammpsLibrary(cores=2)occurrences in tests/test_ase_interface.py.pylammpsmpi/wrapper/extended.py (1)
255-265: Movehostname_localhostto the end of__init__to avoid breaking positional calls
Apply:def __init__( self, cores: int = 1, oversubscribe: bool = False, working_directory: str = ".", - hostname_localhost: Optional[bool] = None, client: Any = None, cmdargs: Optional[list[str]] = None, executor: Optional[BaseExecutor] = None, + hostname_localhost: Optional[bool] = None, ) -> None:Also add
hostname_localhostto the class docstring’s Args section.
🧹 Nitpick comments (3)
.github/workflows/pipeline.yml (1)
114-133: Consider parity with Linux matrix and longer timeout on macOS.
- Add a small Python version matrix (e.g., 3.12 alongside 3.13) for macOS to mirror Linux coverage.
- macOS runners are slower; bump timeout-minutes to 10 to reduce flakiness.
pylammpsmpi/wrapper/ase.py (1)
35-35: Document the new hostname_localhost parameter.Add it to the Args section so users discover it from the docstring.
pylammpsmpi/wrapper/concurrent.py (1)
76-79: Document the new hostname_localhost parameter and its behavior.Add it to the init docstring (and note it’s ignored when an external executor is supplied).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/pipeline.yml(2 hunks)pylammpsmpi/wrapper/ase.py(2 hunks)pylammpsmpi/wrapper/concurrent.py(2 hunks)pylammpsmpi/wrapper/extended.py(2 hunks)tests/test_ase_interface.py(8 hunks)tests/test_base.py(2 hunks)tests/test_concurrent.py(1 hunks)tests/test_executor.py(1 hunks)tests/test_pylammpsmpi_local.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_pylammpsmpi_local.py (1)
pylammpsmpi/wrapper/extended.py (1)
LammpsLibrary(242-333)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: unittest_openmpi_mac
🔇 Additional comments (9)
pylammpsmpi/wrapper/ase.py (1)
66-71: Good propagation of hostname_localhost to LammpsBase.This correctly threads the flag into the multi-core path.
tests/test_concurrent.py (1)
19-21: LGTM: LammpsConcurrent now receives hostname_localhost.Matches the concurrent wrapper change and executor propagation.
tests/test_executor.py (1)
19-28: LGTM: executor+ASE path covers hostname_localhost.Both SingleNodeExecutor and LammpsASELibrary get the flag. Good coverage.
tests/test_pylammpsmpi_local.py (2)
20-22: LGTM: LammpsLibrary now receives hostname_localhost.This ensures the flag reaches LammpsConcurrent in local-mode tests.
119-123: LGTM: SingleNodeExecutor configured with hostname_localhost.Consistent with the new executor parameter.
pylammpsmpi/wrapper/concurrent.py (1)
106-112: Good: pass hostname_localhost through to SingleNodeExecutor.This cleanly localizes hostname behavior in the executor.
.github/workflows/pipeline.yml (1)
123-126: Environments omit any Python spec—no constraints conflict with Python 3.13.tests/test_base.py (1)
22-24: LGTM: LammpsBase inherits init from LammpsConcurrent, which accepts hostname_localhost.pylammpsmpi/wrapper/extended.py (1)
269-276: LGTM: new parameter is properly forwarded to LammpsConcurrent.Propagation via
hostname_localhost=hostname_localhostis correct.
| unittest_mpich_mac: | ||
| needs: [black] | ||
| runs-on: macos-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Conda config | ||
| run: echo -e "channels:\n - conda-forge\n" > .condarc | ||
| - uses: conda-incubator/setup-miniconda@v3 | ||
| with: | ||
| python-version: '3.13' | ||
| miniforge-version: latest | ||
| condarc-file: .condarc | ||
| environment-file: .ci_support/environment-mpich.yml | ||
| - name: Test | ||
| shell: bash -l {0} | ||
| timeout-minutes: 5 | ||
| run: | | ||
| pip install . --no-deps --no-build-isolation | ||
| python -m unittest discover tests | ||
There was a problem hiding this comment.
Gate macOS MPI jobs in the merge flow (add to required/needs).
The new macOS jobs aren’t included in the autobot job’s needs (or branch protection). As-is, PRs can auto-merge even if these jobs fail.
Please add unittest_mpich_mac and unittest_openmpi_mac to the gating set (and update repo required checks accordingly).
Also applies to: 157-176
🤖 Prompt for AI Agents
In .github/workflows/pipeline.yml around lines 114-133 (and also update the similar block at 157-176), the new macOS MPI jobs unittest_mpich_mac and unittest_openmpi_mac are not added to the autobot/merge gating needs; update the workflow so the autobot/merge job's needs array includes both unittest_mpich_mac and unittest_openmpi_mac, and then update the repository branch-protection / required checks list to include these two job names so PRs cannot auto-merge until they pass. There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_mpi_backend.py (1)
121-128: Prefer using the unittest float helpers over manual rounding
assertAlmostEqual(ornp.testing.assert_allclose) keeps the intent clear, avoids an extra NumPy dependency in the check, and gives better failure diagnostics. Suggest refactoring both assertions accordingly.- self.assertEqual( - np.round(select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"]), 8), - np.round(1.1298532212880312, 8), - ) + self.assertAlmostEqual( + select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"]), + 1.1298532212880312, + places=8, + ) @@ - self.assertEqual( - np.round(select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"]), 8), - np.round(1.1298532212880312, 8), - ) + self.assertAlmostEqual( + select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"]), + 1.1298532212880312, + places=8, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_mpi_backend.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_mpi_backend.py (1)
pylammpsmpi/mpi/lmpmpi.py (1)
select_cmd(393-443)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unittest_mpich_mac
- GitHub Check: unittest_openmpi_mac
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_ase_interface.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_ase_interface.py (1)
pylammpsmpi/wrapper/ase.py (1)
LammpsASELibrary(17-465)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unittest_openmpi_mac
- GitHub Check: unittest_openmpi (3.13)
- GitHub Check: unittest_openmpi (3.11)
- GitHub Check: unittest_openmpi (3.12)
- GitHub Check: unittest_mpich_mac
- GitHub Check: coverage
Summary by CodeRabbit
New Features
Tests
Chores