Fix set_fix_external_callback #366
Conversation
for more information, see https://pre-commit.ci
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRemoved MPI rank-0 guards so many data accessors and neighlist helpers run and return on all ranks; _gather_data_from_all_processors now returns flattened data on every rank; added cloudpickle-based serialization in the wrapper and deserialization in the MPI layer to transmit executable callbacks and caller context for set_fix_external_callback. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Wrapper as Concurrent Wrapper
participant Transport as IPC/RPC
participant MPI as MPI Layer (lmpmpi.py)
participant LAMMPS as LAMMPS Job
User->>Wrapper: set_fix_external_callback(fix_id, callback, caller)
Note right of Wrapper #E8F6EF: cloudpickle.dumps(callback) and cloudpickle.dumps(caller)\nReplace LammpsLibrary instances with "lammps"
Wrapper->>Transport: send {cmd: set_fix_external_callback, data:[fix_id, serialized_callback, serialized_caller]}
Transport->>MPI: deliver request
Note right of MPI #FFF4E6: cloudpickle.loads to reconstruct callback & caller\nReplace "lammps" placeholder with live LAMMPS instance
MPI->>LAMMPS: job.set_fix_external_callback(fix_id, callback, caller)
LAMMPS-->>MPI: ack/result
MPI-->>Transport: response
Transport-->>Wrapper: response
Wrapper-->>User: result
sequenceDiagram
autonumber
participant Rank0 as MPI Rank 0
participant RankN as MPI Other Ranks
participant LMP as lmpmpi.py Functions
par All ranks invoke data accessors
Rank0->>LMP: extract_atom/extract_variable/get_natoms/get_neighlist(...)
RankN->>LMP: extract_atom/extract_variable/get_natoms/get_neighlist(...)
and All ranks receive aggregated data
LMP-->>Rank0: flattened data from _gather_data_from_all_processors
LMP-->>RankN: flattened data from _gather_data_from_all_processors
end
Note over Rank0,RankN: Calls now return results on every rank (no rank-0 gating)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pylammpsmpi/wrapper/concurrent.py (1)
412-418: Docstring: fix typos and clarify placeholder behavior.Minor grammar and clarity improvements.
Apply this diff:
- Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). - The `caller` arguemnt is a list of inputs to the callback function, including class instances and, importantly, the LammpsLibrary instance itself. - Since LAMMPS's set_fix_external_callback allows passing a self-reference, we want to keep this feature. - To transfer the callback function and its associated caller list to the layer containing the actual LAMMPS instance, we serialize them using `dill`. - Because the LAMMPS instance is not available at this stage, we insert a placeholder in its position, which will later be replaced with the actual LAMMPS instance during deserialization. + Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). + The `caller` argument is a list of inputs to the callback function, which may include class instances and, optionally, the LammpsLibrary instance itself. + Since LAMMPS allows passing a self-reference, we preserve this feature. + To transfer the callback and its caller list to the MPI layer that holds the actual LAMMPS instance, we serialize them using `dill`. + Because the LAMMPS instance is not available at this stage, we insert a placeholder ("lammps") in its position; it will be replaced with the actual instance during deserialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pylammpsmpi/mpi/lmpmpi.py(7 hunks)pylammpsmpi/wrapper/concurrent.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pipeline
pylammpsmpi/wrapper/concurrent.py
[error] 11-11: ModuleNotFoundError: No module named 'dill'
🔇 Additional comments (3)
pylammpsmpi/mpi/lmpmpi.py (3)
83-85: LGTM: per-atom compute length derived from natoms.Using
job.get_natoms()to setlengthfor style==1 is correct and symmetric across ranks.
90-90: LGTM: remove rank-0 guards for simple accessors.Returning values unconditionally on all ranks is fine since only rank 0 sends results upstream.
Also applies to: 108-110, 112-114, 116-118, 150-151, 160-162, 171-172, 306-307, 310-311, 314-315, 318-319, 322-323, 326-327
121-147: LGTM: extract_atom now safe on all ranks and returns dense numpy array.Validation against
atom_properties, guarded extract, and consistent shape construction look good.
pylammpsmpi/mpi/lmpmpi.py
Outdated
| @@ -5,6 +5,7 @@ | |||
| from ctypes import c_double, c_int | |||
|
|
|||
| import numpy as np | |||
| from dill import loads | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix CI breakage: avoid top-level dill import; import lazily in the only consumer.
Top-level from dill import loads is causing the pipeline failure: ModuleNotFoundError: No module named 'dill'. Make dill an optional dependency by importing it inside set_fix_external_callback, or add it to runtime dependencies.
Apply this diff to remove the top-level import:
-from dill import loadsA follow-up comment below updates set_fix_external_callback to import lazily and handle absence of dill gracefully.
📝 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.
| from dill import loads |
🤖 Prompt for AI Agents
In pylammpsmpi/mpi/lmpmpi.py around line 8, remove the top-level "from dill
import loads" import and instead import dill lazily inside the
set_fix_external_callback function; wrap the import in a try/except ImportError
(or ModuleNotFoundError) and handle the absence of dill gracefully by either
setting the callback to a safe no-op or raising a clear, actionable RuntimeError
when the callback is actually invoked, ensuring that dill remains an optional
runtime dependency.
pylammpsmpi/mpi/lmpmpi.py
Outdated
| """ | ||
| Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). | ||
| This layer has access to the actual LAMMPS instance. | ||
| We deserialize the callback function and the objects in the caller list so they can be passed directly to the LAMMPS instance. | ||
| If present, the placeholder for the LAMMPS instance is replaced with the actual instance before calling the callback. | ||
| """ | ||
| data = [funct_args[0], loads(funct_args[1]), [loads(f) for f in funct_args[2]]] | ||
| data[2] = [job if isinstance(d, str) and d == "lammps" else d for d in data[2]] | ||
| job.set_fix_external_callback(*data) | ||
| return 1 |
There was a problem hiding this comment.
Harden deserialization and make dill optional in set_fix_external_callback.
Current code will:
- Crash if the wrapper sends
[None](attemptsloads(None)). - Fail CI due to top-level
dillimport. - Not work when dill is not installed.
Import dill locally, handle caller being omitted or None, and replace the placeholder safely.
Apply this diff:
def set_fix_external_callback(job, funct_args):
- """
- Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None).
- This layer has access to the actual LAMMPS instance.
- We deserialize the callback function and the objects in the caller list so they can be passed directly to the LAMMPS instance.
- If present, the placeholder for the LAMMPS instance is replaced with the actual instance before calling the callback.
- """
- data = [funct_args[0], loads(funct_args[1]), [loads(f) for f in funct_args[2]]]
- data[2] = [job if isinstance(d, str) and d == "lammps" else d for d in data[2]]
- job.set_fix_external_callback(*data)
+ """
+ Follows LAMMPS set_fix_external_callback(fix_id, callback, caller=None).
+ Deserializes callback and caller list; replaces any "lammps" placeholder with the actual instance.
+ """
+ try:
+ from dill import loads as _loads # local import to avoid hard dep at module import time
+ except ModuleNotFoundError as e:
+ raise ModuleNotFoundError(
+ "Optional dependency 'dill' is required for set_fix_external_callback. "
+ "Install it via `pip install dill`."
+ ) from e
+
+ fix_id = funct_args[0]
+ callback = _loads(funct_args[1])
+ callers_ser = funct_args[2] if len(funct_args) > 2 else None
+
+ # Allow None or [None] to mean "no caller context"
+ if callers_ser is None or callers_ser == [None]:
+ caller = None
+ else:
+ caller = [_loads(f) for f in callers_ser]
+ caller = [job if isinstance(d, str) and d == "lammps" else d for d in caller]
+
+ job.set_fix_external_callback(fix_id, callback, caller)
return 1📝 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.
| """ | |
| Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). | |
| This layer has access to the actual LAMMPS instance. | |
| We deserialize the callback function and the objects in the caller list so they can be passed directly to the LAMMPS instance. | |
| If present, the placeholder for the LAMMPS instance is replaced with the actual instance before calling the callback. | |
| """ | |
| data = [funct_args[0], loads(funct_args[1]), [loads(f) for f in funct_args[2]]] | |
| data[2] = [job if isinstance(d, str) and d == "lammps" else d for d in data[2]] | |
| job.set_fix_external_callback(*data) | |
| return 1 | |
| def set_fix_external_callback(job, funct_args): | |
| """ | |
| Follows LAMMPS set_fix_external_callback(fix_id, callback, caller=None). | |
| Deserializes callback and caller list; replaces any "lammps" placeholder with the actual instance. | |
| """ | |
| try: | |
| from dill import loads as _loads # local import to avoid hard dep at module import time | |
| except ModuleNotFoundError as e: | |
| raise ModuleNotFoundError( | |
| "Optional dependency 'dill' is required for set_fix_external_callback. " | |
| "Install it via `pip install dill`." | |
| ) from e | |
| fix_id = funct_args[0] | |
| callback = _loads(funct_args[1]) | |
| callers_ser = funct_args[2] if len(funct_args) > 2 else None | |
| # Allow None or [None] to mean "no caller context" | |
| if callers_ser is None or callers_ser == [None]: | |
| caller = None | |
| else: | |
| caller = [_loads(f) for f in callers_ser] | |
| caller = [job if isinstance(d, str) and d == "lammps" else d for d in caller] | |
| job.set_fix_external_callback(fix_id, callback, caller) | |
| return 1 |
🤖 Prompt for AI Agents
In pylammpsmpi/mpi/lmpmpi.py around lines 293-302, the deserialization blindly
calls loads on values and relies on a top-level dill import; change it to import
dill locally with a fallback to pickle if dill isn't installed, only call loads
when the serialized value is not None (and treat None or missing caller as an
empty list), and replace any string placeholder "lammps" in the caller list with
the actual job instance safely; also wrap deserialization in a small try/except
to raise a clear error if loading fails.
pylammpsmpi/mpi/lmpmpi.py
Outdated
| data_gather = MPI.COMM_WORLD.gather(data, root=0) | ||
| if MPI.COMM_WORLD.rank == 0: | ||
| data = [] | ||
| for vl in data_gather: | ||
| for v in vl: | ||
| data.append(v) | ||
| return data | ||
| return [v for vl in data_gather for v in vl] | ||
|
|
There was a problem hiding this comment.
Bug: _gather_data_from_all_processors breaks on non-root ranks; broadcast the flattened result.
On non-root ranks, MPI.COMM_WORLD.gather(..., root=0) returns None. Iterating and flattening will raise TypeError, causing crashes/hangs once rank guards were removed.
Apply this diff:
def _gather_data_from_all_processors(data):
- data_gather = MPI.COMM_WORLD.gather(data, root=0)
- return [v for vl in data_gather for v in vl]
+ comm = MPI.COMM_WORLD
+ data_gather = comm.gather(data, root=0)
+ if comm.rank == 0:
+ flat = [v for vl in data_gather for v in vl]
+ else:
+ flat = None
+ flat = comm.bcast(flat, root=0)
+ return flat📝 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.
| data_gather = MPI.COMM_WORLD.gather(data, root=0) | |
| if MPI.COMM_WORLD.rank == 0: | |
| data = [] | |
| for vl in data_gather: | |
| for v in vl: | |
| data.append(v) | |
| return data | |
| return [v for vl in data_gather for v in vl] | |
| def _gather_data_from_all_processors(data): | |
| - data_gather = MPI.COMM_WORLD.gather(data, root=0) | |
| comm = MPI.COMM_WORLD | |
| data_gather = comm.gather(data, root=0) | |
| if comm.rank == 0: | |
| flat = [v for vl in data_gather for v in vl] | |
| else: | |
| flat = None | |
| flat = comm.bcast(flat, root=0) | |
| return flat |
🤖 Prompt for AI Agents
In pylammpsmpi/mpi/lmpmpi.py around lines 439 to 441, MPI.COMM_WORLD.gather
returns None on non-root ranks so the current flattening of data_gather fails on
non-root ranks; change the flow so rank 0 performs the gather and flattens the
result, then broadcast the flattened list to all ranks (use MPI.COMM_WORLD.bcast
with root=0) and return the broadcasted flattened list on every rank. Ensure
non-root ranks do not attempt to iterate over None; only root flattens, then
bcast the flattened result and return that.
pylammpsmpi/wrapper/concurrent.py
Outdated
| @@ -8,6 +8,7 @@ | |||
| from threading import Thread | |||
| from typing import Any, Optional | |||
|
|
|||
| from dill import dumps | |||
There was a problem hiding this comment.
Fix CI breakage: avoid top-level dill import; import lazily inside the method.
Top-level from dill import dumps causes ModuleNotFoundError in CI. Make dill optional and import it only when set_fix_external_callback is called.
Apply this diff:
-from dill import dumpsA follow-up comment below updates set_fix_external_callback accordingly.
📝 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.
| from dill import dumps |
🧰 Tools
🪛 GitHub Actions: Pipeline
[error] 11-11: ModuleNotFoundError: No module named 'dill'
🤖 Prompt for AI Agents
In pylammpsmpi/wrapper/concurrent.py around line 11, the top-level statement
"from dill import dumps" causes CI failures when dill is not installed; remove
this top-level import and instead import dill.dumps lazily inside the
set_fix_external_callback function. Specifically, delete the top-level import
and add a local import (e.g., from dill import dumps) at the start of
set_fix_external_callback (with any needed try/except to raise a clear error if
dill is missing), then use the locally imported dumps within that function.
pylammpsmpi/wrapper/concurrent.py
Outdated
| def set_fix_external_callback(self, *args): | ||
| """ | ||
| Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). | ||
| The `caller` arguemnt is a list of inputs to the callback function, including class instances and, importantly, the LammpsLibrary instance itself. | ||
| Since LAMMPS's set_fix_external_callback allows passing a self-reference, we want to keep this feature. | ||
| To transfer the callback function and its associated caller list to the layer containing the actual LAMMPS instance, we serialize them using `dill`. | ||
| Because the LAMMPS instance is not available at this stage, we insert a placeholder in its position, which will later be replaced with the actual LAMMPS instance during deserialization. | ||
| """ | ||
| caller = [ | ||
| ( | ||
| "lammps" | ||
| if hasattr(arg, "__class__") | ||
| and arg.__class__.__name__ == "LammpsLibrary" | ||
| else arg | ||
| ) | ||
| for arg in args[2] | ||
| ] | ||
| data = [ | ||
| args[0], | ||
| dumps(args[1]), | ||
| [dumps(cl) for cl in caller] if len(args) == 3 else [None], | ||
| ] | ||
| return self._send_and_receive_dict( | ||
| command="set_fix_external_callback", data=list(args) | ||
| command="set_fix_external_callback", data=data | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding dill to runtime dependencies or document it as optional.
Even with lazy imports, users invoking this API will need dill. Decide whether dill is a hard dependency (add to packaging) or optional (document and gate at runtime).
Would you like me to open a follow-up PR to add dill to your packaging? If yes, I’ll target whichever of these files exist:
- pyproject.toml (project.dependencies)
- setup.cfg (install_requires)
- setup.py (install_requires)
- requirements*.txt
Run this to locate packaging files:
🏁 Script executed:
#!/bin/bash
fd -H -a -t f 'pyproject.toml|setup.cfg|setup.py|requirements*.txt'Length of output: 131
#!/bin/bash
# Check for dill imports in the concurrent wrapper
grep -R "from dill" -n pylammpsmpi/wrapper/concurrent.py || true
grep -R "import dill" -n pylammpsmpi/wrapper/concurrent.py || true
# Check usage of dumps to see where it comes from
grep -R "dumps" -n pylammpsmpi/wrapper/concurrent.py || true
# Verify whether dill is already listed in packaging
grep -R "dill" -n pyproject.toml setup.py || true🏁 Script executed:
#!/bin/bash
# Check for dill imports in the concurrent wrapper
grep -R "import dill" -n pylammpsmpi/wrapper/concurrent.py || true
grep -R "from dill" -n pylammpsmpi/wrapper/concurrent.py || true
# Check usage of dumps to locate its origin
grep -R "dumps" -n pylammpsmpi/wrapper/concurrent.py || true
# Verify whether dill is already listed in project dependencies
grep -R "dill" -n pyproject.toml setup.py || trueLength of output: 388
#!/bin/bash
# Show project table and dependencies in pyproject.toml
grep -n -C2 '^\[project\]' pyproject.toml || true
grep -n -C2 'dependencies' pyproject.toml || true
# Show install_requires in setup.py
grep -n -C2 'install_requires' setup.py || trueAdd dill to runtime dependencies or document it as optional
The implementation of set_fix_external_callback uses from dill import dumps, so any user invoking this API will encounter an ImportError unless dill is installed. We have two packaging files in this project that currently do not list dill:
pyproject.tomlsetup.py
Please decide whether to:
- Make
dilla hard dependency by adding it to[project].dependenciesinpyproject.toml, and/orinstall_requiresinsetup.py
- Keep
dilloptional by documenting it in your README and gating the import with a clear runtime check/error message.
Let me know if you’d like me to open a follow-up PR to update these files.
🤖 Prompt for AI Agents
In pylammpsmpi/wrapper/concurrent.py around lines 411 to 435, the code uses
dill.dumps but the project packaging does not declare dill as a dependency;
either make dill a hard dependency by adding "dill" to pyproject.toml
[project].dependencies and to setup.py install_requires (bump packaging version
if needed), or make it optional by moving the import inside this function
(try/except ImportError) and raising a clear runtime error instructing the user
to install dill (and document this optional dependency in README); choose one
approach and apply it consistently across packaging and docs.
pylammpsmpi/wrapper/concurrent.py
Outdated
| caller = [ | ||
| ( | ||
| "lammps" | ||
| if hasattr(arg, "__class__") | ||
| and arg.__class__.__name__ == "LammpsLibrary" | ||
| else arg | ||
| ) | ||
| for arg in args[2] | ||
| ] | ||
| data = [ | ||
| args[0], | ||
| dumps(args[1]), | ||
| [dumps(cl) for cl in caller] if len(args) == 3 else [None], | ||
| ] | ||
| return self._send_and_receive_dict( | ||
| command="set_fix_external_callback", data=list(args) | ||
| command="set_fix_external_callback", data=data | ||
| ) |
There was a problem hiding this comment.
Handle caller=None, import dill lazily, and align signature with LAMMPS API.
This implementation assumes args[2] exists and is iterable. It will raise if caller is omitted or None. It also relies on a top-level dill import (breaking CI). The PR objective states the method should accept three arguments with an optional caller.
Apply this diff to make the method robust and match the intended API:
- def set_fix_external_callback(self, *args):
+ def set_fix_external_callback(self, fix_id, callback, caller=None):
"""
- Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None).
- The `caller` arguemnt is a list of inputs to the callback function, including class instances and, importantly, the LammpsLibrary instance itself.
- Since LAMMPS's set_fix_external_callback allows passing a self-reference, we want to keep this feature.
- To transfer the callback function and its associated caller list to the layer containing the actual LAMMPS instance, we serialize them using `dill`.
- Because the LAMMPS instance is not available at this stage, we insert a placeholder in its position, which will later be replaced with the actual LAMMPS instance during deserialization.
+ Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None).
+ The `caller` argument is a list of inputs to the callback function, potentially including a self-reference to LammpsLibrary.
"""
- caller = [
- (
- "lammps"
- if hasattr(arg, "__class__")
- and arg.__class__.__name__ == "LammpsLibrary"
- else arg
- )
- for arg in args[2]
- ]
- data = [
- args[0],
- dumps(args[1]),
- [dumps(cl) for cl in caller] if len(args) == 3 else [None],
- ]
- return self._send_and_receive_dict(
- command="set_fix_external_callback", data=data
- )
+ try:
+ from dill import dumps as _dumps # local import to avoid hard dep at module import time
+ except ModuleNotFoundError as e:
+ raise ModuleNotFoundError(
+ "Optional dependency 'dill' is required for set_fix_external_callback. "
+ "Install it via `pip install dill`."
+ ) from e
+
+ serialized_caller = None
+ if caller is not None:
+ if not isinstance(caller, (list, tuple)):
+ raise TypeError("caller must be a list or tuple, or None")
+ processed = [
+ (
+ "lammps"
+ if getattr(arg, "__class__", None)
+ and arg.__class__.__name__ == "LammpsLibrary"
+ else arg
+ )
+ for arg in caller
+ ]
+ serialized_caller = [_dumps(cl) for cl in processed]
+
+ data = [fix_id, _dumps(callback), serialized_caller]
+ return self._send_and_receive_dict(command="set_fix_external_callback", data=data)Notes:
- Signature is now explicit: (fix_id, callback, caller=None), matching LAMMPS.
caller=Noneand empty lists are handled.- Dill is imported lazily with a clear error message if missing.
📝 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.
| caller = [ | |
| ( | |
| "lammps" | |
| if hasattr(arg, "__class__") | |
| and arg.__class__.__name__ == "LammpsLibrary" | |
| else arg | |
| ) | |
| for arg in args[2] | |
| ] | |
| data = [ | |
| args[0], | |
| dumps(args[1]), | |
| [dumps(cl) for cl in caller] if len(args) == 3 else [None], | |
| ] | |
| return self._send_and_receive_dict( | |
| command="set_fix_external_callback", data=list(args) | |
| command="set_fix_external_callback", data=data | |
| ) | |
| def set_fix_external_callback(self, fix_id, callback, caller=None): | |
| """ | |
| Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). | |
| The `caller` argument is a list of inputs to the callback function, potentially including a self-reference to LammpsLibrary. | |
| """ | |
| try: | |
| from dill import dumps as _dumps # local import to avoid hard dep at module import time | |
| except ModuleNotFoundError as e: | |
| raise ModuleNotFoundError( | |
| "Optional dependency 'dill' is required for set_fix_external_callback. " | |
| "Install it via `pip install dill`." | |
| ) from e | |
| serialized_caller = None | |
| if caller is not None: | |
| if not isinstance(caller, (list, tuple)): | |
| raise TypeError("caller must be a list or tuple, or None") | |
| processed = [ | |
| ( | |
| "lammps" | |
| if getattr(arg, "__class__", None) | |
| and arg.__class__.__name__ == "LammpsLibrary" | |
| else arg | |
| ) | |
| for arg in caller | |
| ] | |
| serialized_caller = [_dumps(cl) for cl in processed] | |
| data = [fix_id, _dumps(callback), serialized_caller] | |
| return self._send_and_receive_dict(command="set_fix_external_callback", data=data) |
🤖 Prompt for AI Agents
In pylammpsmpi/wrapper/concurrent.py around lines 419-435, the method currently
assumes args[2] exists and uses a top-level dill import; change the function
signature to explicitly be (fix_id, callback, caller=None) to match the LAMMPS
API, handle caller being None or an empty list by treating it as no caller,
lazily import dill inside the function and raise a clear ImportError message if
dill is not installed, serialize the callback and each caller element with
dill.dumps (only if caller is provided) and build data as [fix_id,
dumps(callback), [dumps(c) for c in caller] if caller else [None]] before
calling self._send_and_receive_dict(command="set_fix_external_callback",
data=data).
…mmpsmpi into fix_exernal_callback
for more information, see https://pre-commit.ci
Remove wild dill Co-authored-by: Jan Janssen <jan-janssen@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
- Coverage 81.93% 80.22% -1.72%
==========================================
Files 5 7 +2
Lines 559 814 +255
==========================================
+ Hits 458 653 +195
- Misses 101 161 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pylammpsmpi/wrapper/concurrent.py (1)
11-11: Make the serializer import lazy to avoid optional-dependency import errors at module import timeTop-level
from cloudpickle import dumpswill break environments/CI where cloudpickle isn’t installed. Import it insideset_fix_external_callback(with a clear error message) or add cloudpickle as a hard runtime dependency.Apply this diff to remove the top-level import:
-from cloudpickle import dumpsThen see my suggested method rewrite below for a local import pattern.
To verify packaging alignment, please run:
#!/bin/bash # Check whether cloudpickle is declared as a dependency rg -n --hidden -C2 '(dependencies|install_requires)' pyproject.toml setup.cfg setup.py || true rg -n 'cloudpickle' pyproject.toml setup.cfg setup.py || true
🧹 Nitpick comments (3)
pylammpsmpi/wrapper/concurrent.py (3)
412-418: Docstring fixes: serializer mismatch and typos
- The text references “dill” but the implementation uses cloudpickle.
- Typo: “arguemnt” → “argument”.
- Consider documenting that this method returns a Future.
If you prefer to keep the current function body, at minimum adjust the docstring:
- The `caller` arguemnt is a list of inputs to the callback function, including class instances and, importantly, the LammpsLibrary instance itself. + The `caller` argument is a list of inputs to the callback function, including class instances and, importantly, the LammpsLibrary instance itself. - To transfer the callback function and its associated caller list to the layer containing the actual LAMMPS instance, we serialize them using `dill`. + To transfer the callback function and its associated caller list to the layer containing the actual LAMMPS instance, we serialize them using `cloudpickle`. + + Returns + ------- + Future + A Future resolving to the result of the underlying MPI call.
420-421: Style: collapse nested conditionals as suggested by static analysis (SIM102)Combine the nested checks into one for clarity.
- if len(args) == 3: - if args[2] is not None: + if len(args) == 3 and args[2] is not None:Note: This is moot if you adopt the explicit signature refactor above.
425-431: Reduce brittleness in LammpsLibrary detection and placeholder collision risk
- Matching on
__class__.__name__ == "LammpsLibrary"is brittle. If feasible, prefer an explicit isinstance check (importing the type), or at least usegetattr(arg, "__class__", None)defensively.- Using the plain string "lammps" risks accidental collisions if a user actually passes the string "lammps" in
caller. Consider a dedicated sentinel value.If you want a low-friction improvement without importing the class:
- if hasattr(arg, "__class__") - and arg.__class__.__name__ == "LammpsLibrary" + if (getattr(arg, "__class__", None) and arg.__class__.__name__ == "LammpsLibrary")Optionally, define a collision-resistant sentinel at module scope and use it instead of "lammps":
# at module top-level (outside the changed range) LAMMPS_SENTINEL = "__pylammpsmpi__LAMMPS_SENTINEL_v1__"Then, inside the method:
processed = [LAMMPS_SENTINEL if is_lammps else arg for arg in caller] data.append([_dumps(LAMMPS_SENTINEL) if x == LAMMPS_SENTINEL else _dumps(x) for x in processed])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pylammpsmpi/mpi/lmpmpi.py(7 hunks)pylammpsmpi/wrapper/concurrent.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pylammpsmpi/mpi/lmpmpi.py
🧰 Additional context used
🧬 Code graph analysis (1)
pylammpsmpi/wrapper/concurrent.py (2)
pylammpsmpi/mpi/lmpmpi.py (1)
command(382-384)pylammpsmpi/wrapper/base.py (1)
command(363-376)
🪛 Ruff (0.12.2)
pylammpsmpi/wrapper/concurrent.py
420-421: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
pylammpsmpi/wrapper/concurrent.py
Outdated
| data = [args[0], dumps(args[1])] | ||
| if len(args) == 3: | ||
| if args[2] is not None: | ||
| data.append( | ||
| [ | ||
| ( | ||
| dumps("lammps") | ||
| if hasattr(arg, "__class__") | ||
| and arg.__class__.__name__ == "LammpsLibrary" | ||
| else dumps(arg) | ||
| ) | ||
| for arg in args[2] | ||
| ] | ||
| ) | ||
| return self._send_and_receive_dict( |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure serializer consistency across layers (wrapper vs. MPI receiver) and packaging
This side now serializes with cloudpickle. The MPI side must deserialize with the same library. Mismatched (dill vs. cloudpickle) will fail at loads().
Run this to confirm consistency and packaging:
Expected:
- MPI receiver uses cloudpickle.loads.
- Packaging lists cloudpickle, or we keep it optional with the lazy import and documented requirement.
🏁 Script executed:
#!/bin/bash
# 1) Find deserialization calls and see which library they use.
rg -n -C2 -g '!**/site-packages/**' -e '\bloads\s*\(' -e 'cloudpickle|dill' pylammpsmpi/mpi pylammpsmpi/wrapper || true
# 2) Check for references to set_fix_external_callback on the MPI side.
rg -n -C3 'set_fix_external_callback' pylammpsmpi || true
# 3) Confirm cloudpickle is declared as a dependency somewhere.
rg -n --hidden -C2 '(dependencies|install_requires)' pyproject.toml setup.cfg setup.py || true
rg -n 'cloudpickle' pyproject.toml setup.cfg setup.py || trueLength of output: 7832
Add cloudpickle as a dependency and correct serializer documentation
- In pylammpsmpi/wrapper/concurrent.py (around lines 414–418), the docstring still says “we serialize them using
dill,” but the code imports and usescloudpickle.dumps. Update the text to reference cloudpickle instead of dill. - In pyproject.toml, under project.dependencies, add "cloudpickle" so that both the wrapper and MPI layers can import it unconditionally.
With these changes, serialization/deserialization remain consistent across layers and the package metadata accurately reflects the required dependency.
🧰 Tools
🪛 Ruff (0.12.2)
420-421: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🤖 Prompt for AI Agents
In pylammpsmpi/wrapper/concurrent.py around lines 414–433, update the docstring
that currently says “we serialize them using dill” to reference cloudpickle
instead (e.g., “we serialize them using cloudpickle”) so documentation matches
the actual cloudpickle.dumps usage; then add "cloudpickle" to
project.dependencies in pyproject.toml so cloudpickle is installed for both
wrapper and MPI layers, ensuring imports succeed unconditionally.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pylammpsmpi/wrapper/concurrent.py (2)
11-11: Avoid top-level serializer import; import lazily inside the method (prevents ModuleNotFoundError at import time).Top-level
from cloudpickle import dumpswill break environments/CI wherecloudpickleisn’t installed. Import it lazily insideset_fix_external_callback(or add it as a hard dependency). Given prior CI failures for a similar top-level dill import, let’s avoid repeating that class of issue.Apply this diff to remove the top-level import (the method patch below will add a local import):
-from cloudpickle import dumps
411-434: Make signature explicit, fix docstring, add lazy import + input validation, and keep payload shape stable.
- Use explicit signature
(fix_id, callback, caller=None)to match the LAMMPS API and the PR objective.- Docstring currently references dill and has typos; update to cloudpickle and correct wording.
- Lazy-import
cloudpickleinside the method to avoid breaking module import if optional.- Validate
callbackandcallertypes; guard against strings/iterables that aren’t list/tuple.- Keep the wire format stable: send
[fix_id, serialized_callback]and append a third element only ifcalleris provided (avoids changing the receiver’s parsing logic).Apply this diff:
- def set_fix_external_callback(self, *args): - """ - Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). - The `caller` arguemnt is a list of inputs to the callback function, including class instances and, importantly, the LammpsLibrary instance itself. - Since LAMMPS's set_fix_external_callback allows passing a self-reference, we want to keep this feature. - To transfer the callback function and its associated caller list to the layer containing the actual LAMMPS instance, we serialize them using `dill`. - Because the LAMMPS instance is not available at this stage, we insert a placeholder in its position, which will later be replaced with the actual LAMMPS instance during deserialization. - """ - data = [args[0], dumps(args[1])] - if len(args) == 3 and args[2] is not None: - data.append( - [ - ( - dumps("lammps") - if hasattr(arg, "__class__") - and arg.__class__.__name__ == "LammpsLibrary" - else dumps(arg) - ) - for arg in args[2] - ] - ) - return self._send_and_receive_dict( - command="set_fix_external_callback", data=data - ) + def set_fix_external_callback(self, fix_id, callback, caller=None): + """ + Follows the LAMMPS signature: set_fix_external_callback(fix_id, callback, caller=None). + The `caller` argument is a sequence of inputs to the callback and may include a + self-reference to the LAMMPS instance (LammpsLibrary). That element is replaced with + the placeholder string "lammps" before serialization; on the MPI side it is restored + to the actual LAMMPS instance prior to invoking LAMMPS's set_fix_external_callback. + Serialization uses cloudpickle. + """ + try: + from cloudpickle import dumps as _dumps # lazy import to avoid hard dep at import time + except ModuleNotFoundError as e: + raise ModuleNotFoundError( + "Optional dependency 'cloudpickle' is required for set_fix_external_callback. " + "Install it via `pip install cloudpickle`." + ) from e + + if not callable(callback): + raise TypeError("callback must be callable") + + serialized_caller = None + if caller is not None: + if not isinstance(caller, (list, tuple)): + raise TypeError("caller must be a list or tuple, or None") + processed = [ + ( + "lammps" + if getattr(arg, "__class__", None) + and arg.__class__.__name__ == "LammpsLibrary" + else arg + ) + for arg in caller + ] + serialized_caller = [_dumps(cl) for cl in processed] + + data = [fix_id, _dumps(callback)] + if serialized_caller is not None: + data.append(serialized_caller) + return self._send_and_receive_dict(command="set_fix_external_callback", data=data)
🧹 Nitpick comments (3)
pylammpsmpi/wrapper/concurrent.py (3)
412-418: Docstring fixes: serializer mismatch and typos.
- Replace mentions of “dill” with “cloudpickle” to match the actual implementation.
- Fix “arguemnt” → “argument”.
- Prefer consistent “LAMMPS” spelling.
424-429: Optional: use a collision-resistant placeholder string instead of plain "lammps".Using the common string "lammps" risks accidental replacement if a legitimate caller element equals that string. Consider a unique sentinel such as "PYLAMMPSMPI_LAMMPS_PLACEHOLDER" and update the MPI side accordingly.
If you opt in, apply this minimal change (requires corresponding MPI-side update to look for the new sentinel after loads()):
- "lammps" + "__PYLAMMPSMPI_LAMMPS_PLACEHOLDER__"
411-434: Add a lightweight unit test to validate serialization shape and placeholder substitution.We can subclass
LammpsConcurrentin tests to capture thedatawe enqueue and assert:
- two-element payload when
caller=None- three-element payload when
callerprovided- placeholder present for
LammpsLibraryinstancescallbackis actually serializedI can draft this test if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pylammpsmpi/wrapper/concurrent.py(2 hunks)
| def set_fix_external_callback(self, *args): | ||
| """ | ||
| Follows the signature of LAMMPS's set_fix_external_callback(fix_id, callback, caller=None). | ||
| The `caller` arguemnt is a list of inputs to the callback function, including class instances and, importantly, the LammpsLibrary instance itself. | ||
| Since LAMMPS's set_fix_external_callback allows passing a self-reference, we want to keep this feature. | ||
| To transfer the callback function and its associated caller list to the layer containing the actual LAMMPS instance, we serialize them using `dill`. | ||
| Because the LAMMPS instance is not available at this stage, we insert a placeholder in its position, which will later be replaced with the actual LAMMPS instance during deserialization. | ||
| """ | ||
| data = [args[0], dumps(args[1])] | ||
| if len(args) == 3 and args[2] is not None: | ||
| data.append( | ||
| [ | ||
| ( | ||
| dumps("lammps") | ||
| if hasattr(arg, "__class__") | ||
| and arg.__class__.__name__ == "LammpsLibrary" | ||
| else dumps(arg) | ||
| ) | ||
| for arg in args[2] | ||
| ] | ||
| ) | ||
| return self._send_and_receive_dict( | ||
| command="set_fix_external_callback", data=list(args) | ||
| command="set_fix_external_callback", data=data | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify serializer alignment with MPI layer and packaging; ensure API shape matches receiver.
- Confirm the MPI receiver uses the same serializer (
cloudpickle.loads) and expects the payload as constructed here:[fix_id, dumps(callback)]plus an optional third element for caller. - Decide whether
cloudpickleis a hard dependency (add to packaging) or optional (keep lazy import and document). Right now, top-level removal plus lazy import means callers need it at runtime.
Run this script from repo root:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check MPI side uses cloudpickle and matching API =="
rg -n -C2 'set_fix_external_callback' pylammpsmpi/mpi || true
rg -n -C2 '\blo(a)?ds\s*\(' pylammpsmpi/mpi || true
rg -n 'cloudpickle|dill' pylammpsmpi/mpi || true
echo -e "\n== Check packaging for cloudpickle =="
rg -n --hidden -C2 '(dependencies|install_requires)' pyproject.toml setup.cfg setup.py requirements*.txt || true
rg -n 'cloudpickle' pyproject.toml setup.cfg setup.py requirements*.txt || true
echo -e "\n== Find stale references to dill =="
rg -n 'dill' pylammpsmpi || true
echo -e "\n== Verify LammpsLibrary class name to ensure placeholder detection is correct =="
rg -nP 'class\s+LammpsLibrary\b' pylammpsmpi || trueLength of output: 3758
Standardize serialization and declare serializer dependencies
- In
pylammpsmpi/wrapper/concurrent.py, switch from usingdill.dumpstocloudpickle.dumps(to match the MPI layer) or, alternatively, update the MPI receiver inpylammpsmpi/mpi/lmpmpi.py(lines 299–303) to calldill.loadsinstead ofcloudpickle.loads. Whichever serializer you choose, ensure both sides use the samedumps/loadspair so callbacks and caller-lists deserialize correctly. - Verify that the payload shape remains exactly:
- fix ID
- serializer.dumps(callback)
- (optional) list of serializer.dumps(each caller arg)
The MPI side then does:
data = [funct_args[0], loads(funct_args[1])] if len(funct_args) == 3: data.append([loads(f) for f in funct_args[2]]) data[2] = [job if d == "lammps" else d for d in data[2]] job.set_fix_external_callback(*data)
- Update
pyproject.tomlunder[project.dependencies]to addcloudpickle(anddillif you continue to use it) so that both are installed at runtime, or else document and guard their lazy imports if you intend to keep them optional.
|
@raynol-dsouza I marked the pull request as draft, can you add a unit test for using fix external? |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…mmpsmpi into fix_exernal_callback
for more information, see https://pre-commit.ci
…mmpsmpi into fix_exernal_callback
|
@jan-janssen I wrote a unit test for |
(Possible) fix for #354:
self.set_fix_external_callbackinLammpsConcurrentnow expects 3 arguments:fix_id,callback, andcaller, same as regular the LAMMPS python API.Self-reference treatment:
Step 1: Check if the
callerlist has a reference to itself. If yes, replace it with a placeholder string"lammps"(could replace it by a protected variable). Picklecallbackand thecallerlist usingcloudpickle.dumps, and send them tolmpmpi.Step 2: In
lmpmpi, unpickle the arguments usingcloudpickle.loads. If one of the elements is"lammps", replace it with the actual LAMMPS instance, and pass it to LAMMPS'sset_fix_external_callback.Summary by CodeRabbit