Use ASE_LAMMPSRUN_COMMAND as a fallback option#313
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 #313 +/- ##
==========================================
+ Coverage 82.36% 82.39% +0.03%
==========================================
Files 11 11
Lines 1157 1159 +2
==========================================
+ Hits 953 955 +2
Misses 204 204 ☔ 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
🧹 Nitpick comments (1)
src/lammpsparser/compatibility/file.py (1)
55-90: Docstring missinglmp_commandparameter andASE_LAMMPSRUN_COMMANDenv-var documentation.The new optional parameter and its resolution logic (env-var lookup → hardcoded fallback) are not documented, which leaves callers unaware of the configuration mechanism.
📝 Suggested docstring addition
units (str): Units for LAMMPS + lmp_command (str, optional): Full command used to invoke LAMMPS (e.g. + ``"mpiexec -n 1 lmp_mpi -in lmp.in"``). When *None* (default), + the value of the ``ASE_LAMMPSRUN_COMMAND`` environment variable is + used as the executable prefix and ``" -in lmp.in"`` is appended. + Falls back to ``"mpiexec -n 1 --oversubscribe lmp_mpi -in lmp.in"`` + when the environment variable is absent or empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lammpsparser/compatibility/file.py` around lines 55 - 90, The docstring for the calculate function is missing the new optional lmp_command parameter and the ASE_LAMMPSRUN_COMMAND environment variable resolution; update the Args section to document lmp_command (str|None): explicit LAMMPS command to run, and explain resolution order used by calculate: if lmp_command is provided use it, else check ASE_LAMMPSRUN_COMMAND env var, else fall back to the library's hardcoded default LAMMPS command. Also note that lmp_command overrides env var and include a short example or format expected for ASE_LAMMPSRUN_COMMAND.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lammpsparser/compatibility/file.py`:
- Around line 96-100: The code sets lmp_command from
os.getenv("ASE_LAMMPSRUN_COMMAND", ...) which yields the default only if the
variable is absent, so an explicitly empty ASE_LAMMPSRUN_COMMAND yields a broken
value like " -in lmp.in"; update the logic around lmp_command (the os.getenv
call and its assignment) to treat an empty or whitespace-only environment
variable as unset and fall back to the intended default ("mpiexec -n 1
--oversubscribe lmp_mpi"), e.g. read the env with os.environ.get or
os.getenv(..., None), strip and check truthiness before concatenating " -in
lmp.in", and ensure the final lmp_command used with subprocess.check_output is a
non-empty, valid command string.
---
Nitpick comments:
In `@src/lammpsparser/compatibility/file.py`:
- Around line 55-90: The docstring for the calculate function is missing the new
optional lmp_command parameter and the ASE_LAMMPSRUN_COMMAND environment
variable resolution; update the Args section to document lmp_command (str|None):
explicit LAMMPS command to run, and explain resolution order used by calculate:
if lmp_command is provided use it, else check ASE_LAMMPSRUN_COMMAND env var,
else fall back to the library's hardcoded default LAMMPS command. Also note that
lmp_command overrides env var and include a short example or format expected for
ASE_LAMMPSRUN_COMMAND.
| if lmp_command is None: | ||
| lmp_command = ( | ||
| os.getenv("ASE_LAMMPSRUN_COMMAND", "mpiexec -n 1 --oversubscribe lmp_mpi") | ||
| + " -in lmp.in" | ||
| ) |
There was a problem hiding this comment.
ASE_LAMMPSRUN_COMMAND="" produces a broken command silently.
os.getenv(key, default) only returns the default when the variable is absent. If the variable is set to an empty string, lmp_command becomes " -in lmp.in", which subprocess.check_output(..., shell=True) will invoke and fail with a cryptic "command not found" error.
🛠️ Proposed fix
- if lmp_command is None:
- lmp_command = (
- os.getenv("ASE_LAMMPSRUN_COMMAND", "mpiexec -n 1 --oversubscribe lmp_mpi")
- + " -in lmp.in"
- )
+ if lmp_command is None:
+ lmp_command = (
+ os.getenv("ASE_LAMMPSRUN_COMMAND") or "mpiexec -n 1 --oversubscribe lmp_mpi"
+ ) + " -in lmp.in"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lammpsparser/compatibility/file.py` around lines 96 - 100, The code sets
lmp_command from os.getenv("ASE_LAMMPSRUN_COMMAND", ...) which yields the
default only if the variable is absent, so an explicitly empty
ASE_LAMMPSRUN_COMMAND yields a broken value like " -in lmp.in"; update the logic
around lmp_command (the os.getenv call and its assignment) to treat an empty or
whitespace-only environment variable as unset and fall back to the intended
default ("mpiexec -n 1 --oversubscribe lmp_mpi"), e.g. read the env with
os.environ.get or os.getenv(..., None), strip and check truthiness before
concatenating " -in lmp.in", and ensure the final lmp_command used with
subprocess.check_output is a non-empty, valid command string.
1 participant
@Gitdowski As we just discussed the different options to set the LAMMPS executable in the last meeting, this pull request adds the option to specify the LAMMPS executable via the ASE environment variable.
Summary by CodeRabbit
ASE_LAMMPSRUN_COMMANDenvironment variable, enabling customization for different computing environments and deployment scenarios. Backward compatibility is maintained for existing code that doesn't set the environment variable, ensuring seamless transitions.