Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions .ci_support/mpi4pywrapper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
try:
from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
except ImportError:
pass
Comment on lines +1 to +4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Silent import failure prevents clear error diagnosis.

The bare pass in the except clause silently suppresses import errors, which will lead to cryptic NameError exceptions later when MPI constants are referenced (lines 25, 28, 32, 35, etc.). This makes it difficult to diagnose missing dependencies.

Consider one of these approaches:

Option 1: Fail fast if mpi4py is unavailable:

 try:
     from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
 except ImportError:
-    pass
+    raise ImportError("mpi4py is required but not installed")

Option 2: Define stub constants if testing without mpi4py is intentional:

 try:
     from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
 except ImportError:
-    pass
+    # Define stubs for environments without mpi4py
+    Request = SUM = MAX = MIN = IN_PLACE = IDENT = CONGRUENT = SIMILAR = UNEQUAL = None
📝 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.

Suggested change
try:
from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
except ImportError:
pass
try:
from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
except ImportError:
raise ImportError("mpi4py is required but not installed")
Suggested change
try:
from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
except ImportError:
pass
try:
from mpi4py.MPI import Request, SUM, MAX, MIN, IN_PLACE, IDENT, CONGRUENT, SIMILAR, UNEQUAL
except ImportError:
# Define stubs for environments without mpi4py
Request = SUM = MAX = MIN = IN_PLACE = IDENT = CONGRUENT = SIMILAR = UNEQUAL = None
🤖 Prompt for AI Agents
In .ci_support/mpi4pywrapper.py lines 1-4, the except block currently silences
ImportError which causes later NameError when MPI names are referenced; update
this to either (a) fail fast by re-raising a clear ImportError with a helpful
message that mpi4py is required (e.g. raise ImportError("mpi4py is required for
tests; install mpi4py or run without MPI support") from err), or (b) if running
without mpi4py is intentional, provide explicit stub definitions for Request and
the MPI constants (e.g. set Request = None and define SUM, MAX, MIN, IN_PLACE,
IDENT, CONGRUENT, SIMILAR, UNEQUAL to safe sentinel values) plus a module-level
flag like MPI_AVAILABLE = False so callers can check; implement one of these
fixes and remove the bare pass.


import numpy as np


class MPI4PYWrapper:
def __init__(self, comm, parent=None):
self.comm = comm
self.size = comm.size
self.rank = comm.rank
self.parent = parent # XXX check C-object against comm.parent?

def new_communicator(self, ranks):
comm = self.comm.Create(self.comm.group.Incl(ranks))
if self.comm.rank in ranks:
return MPI4PYWrapper(comm, parent=self)
else:
# This cpu is not in the new communicator:
return None

def max_scalar(self, a, root=-1):
return self.sum_scalar(a, root=-1, _op=MAX)

def min_scalar(self, a, root=-1):
return self.sum_scalar(a, root=-1, _op=MIN)
Comment on lines +24 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Unused root parameter causes incorrect reduction behavior.

Both max_scalar and min_scalar accept a root parameter but ignore it, always passing root=-1 to sum_scalar. This means these methods will always perform an all-reduce operation, even when the caller specifies a specific root rank.

Forward the root parameter correctly:

 def max_scalar(self, a, root=-1):
-    return self.sum_scalar(a, root=-1, _op=MAX)
+    return self.sum_scalar(a, root=root, _op=MAX)

 def min_scalar(self, a, root=-1):
-    return self.sum_scalar(a, root=-1, _op=MIN)
+    return self.sum_scalar(a, root=root, _op=MIN)
📝 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.

Suggested change
def max_scalar(self, a, root=-1):
return self.sum_scalar(a, root=-1, _op=MAX)
def min_scalar(self, a, root=-1):
return self.sum_scalar(a, root=-1, _op=MIN)
def max_scalar(self, a, root=-1):
return self.sum_scalar(a, root=root, _op=MAX)
def min_scalar(self, a, root=-1):
return self.sum_scalar(a, root=root, _op=MIN)
🧰 Tools
🪛 Ruff (0.13.3)

24-24: Unused method argument: root

(ARG002)


27-27: Unused method argument: root

(ARG002)

🤖 Prompt for AI Agents
In .ci_support/mpi4pywrapper.py around lines 24 to 28, both max_scalar and
min_scalar ignore the passed root parameter and hardcode root=-1 when calling
sum_scalar; update each method to forward the root argument (use the local
variable root) to sum_scalar while still defaulting to -1 so the
caller-specified root is honored.


def sum_scalar(self, a, root=-1, _op=None):
if _op is None:
_op = SUM
assert isinstance(a, (int, float, complex))
if root == -1:
return self.comm.allreduce(a, op=_op)
else:
return self.comm.reduce(a, root=root, op=_op)

def sum(self, a, root=-1):
if root == -1:
self.comm.Allreduce(IN_PLACE, a, op=SUM)
else:
if root == self.rank:
self.comm.Reduce(IN_PLACE, a, root=root, op=SUM)
else:
self.comm.Reduce(a, None, root=root, op=SUM)

def scatter(self, a, b, root):
self.comm.Scatter(a, b, root)

def alltoallv(self, sbuffer, scounts, sdispls, rbuffer, rcounts, rdispls):
self.comm.Alltoallv((sbuffer, (scounts, sdispls), sbuffer.dtype.char),
(rbuffer, (rcounts, rdispls), rbuffer.dtype.char))

def all_gather(self, a, b):
self.comm.Allgather(a, b)

def gather(self, a, root, b=None):
self.comm.Gather(a, b, root)

def broadcast(self, a, root):
self.comm.Bcast(a, root)

def sendreceive(self, a, dest, b, src, sendtag=123, recvtag=123):
return self.comm.Sendrecv(a, dest, sendtag, b, src, recvtag)

def send(self, a, dest, tag=123, block=True):
if block:
self.comm.Send(a, dest, tag)
else:
return self.comm.Isend(a, dest, tag)

def ssend(self, a, dest, tag=123):
return self.comm.Ssend(a, dest, tag)

def receive(self, a, src, tag=123, block=True):
if block:
self.comm.Recv(a, src, tag)
else:
return self.comm.Irecv(a, src, tag)

def test(self, request):
return request.test()

def testall(self, requests):
return Request.testall(requests)

def wait(self, request):
request.wait()

def waitall(self, requests):
Request.waitall(requests)

def name(self):
return self.comm.Get_name()

def barrier(self):
self.comm.barrier()

def abort(self, errcode):
self.comm.Abort(errcode)

def compare(self, othercomm):
code = self.comm.Compare(othercomm.comm)
if code == IDENT:
return "ident"
elif code == CONGRUENT:
return "congruent"
elif code == SIMILAR:
return "similar"
elif code == UNEQUAL:
return "unequal"
else:
raise ValueError(f"Unknown compare code {code}")

def translate_ranks(self, other, ranks):
return np.array(self.comm.Get_group().Translate_ranks(ranks, other.comm.Get_group()))

def get_members(self):
return self.translate_ranks(self.parent, np.arange(self.size))

def get_c_object(self):
return self.comm
6 changes: 3 additions & 3 deletions .github/workflows/pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,13 @@ jobs:
channels: conda-forge
conda-remove-defaults: "true"
environment-file: .ci_support/environment-integration.yml
- name: Install
shell: bash -l {0}
run: pip install . --no-deps --no-build-isolation
- name: Notebooks
shell: bash -l {0}
timeout-minutes: 20
run: |
pip install . --no-deps --no-build-isolation
cp .ci_support/mpi4pywrapper.py /home/runner/miniconda3/envs/test/lib/python3.12/site-packages/gpaw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Hardcoded Python version in path is brittle.

The path explicitly includes python3.12, which will break if the Python version specified in line 177 is updated without also updating this path.

Consider using a version-agnostic approach:

-          cp .ci_support/mpi4pywrapper.py /home/runner/miniconda3/envs/test/lib/python3.12/site-packages/gpaw
+          cp .ci_support/mpi4pywrapper.py $(python -c "import sysconfig; print(sysconfig.get_path('purelib'))")/gpaw
📝 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.

Suggested change
cp .ci_support/mpi4pywrapper.py /home/runner/miniconda3/envs/test/lib/python3.12/site-packages/gpaw
cp .ci_support/mpi4pywrapper.py $(python -c "import sysconfig; print(sysconfig.get_path('purelib'))")/gpaw
🤖 Prompt for AI Agents
.github/workflows/pipeline.yml around line 189: the cp command hardcodes a
python3.12 path which will break when the runner Python/conda env version
changes; replace the hardcoded path with a runtime-resolved site-packages
destination—either derive site-packages from the active conda environment prefix
(CONDA_PREFIX) or invoke Python at runtime to print
sysconfig.get_paths()[“purelib”] and use that value as the target for cp—so the
workflow computes the correct site-packages path instead of embedding a specific
Python version.

flux start flux resource list
flux start papermill notebooks/5-1-gpaw.ipynb notebooks/5-1-gpaw-out.ipynb -k python3
flux start papermill notebooks/5-2-quantum-espresso.ipynb notebooks/5-2-quantum-espresso-out.ipynb -k python3

Expand Down
Loading
Loading