Skip to content
Open
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
62 changes: 32 additions & 30 deletions source/op/pd/comm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,29 @@

#if defined(GOOGLE_CUDA) || defined(TENSORFLOW_USE_ROCM)
#include "device.h"

template <typename FPTYPE>
static void copy_local_tensor_data(FPTYPE* dst,
const FPTYPE* src,
size_t count,
const paddle::Place& place) {
if (count == 0) {
return;
}

// CUDA-aware MPI describes whether MPI can consume device pointers; it does
// not describe where this particular Paddle tensor lives. Self-swaps must
// select the copy primitive from the actual tensor place so CPU tensors also
// work in CUDA/ROCm-enabled builds.
if (phi::is_gpu_place(place)) {
gpuMemcpy(dst, src, count * sizeof(FPTYPE), gpuMemcpyDeviceToDevice);
} else {
// CPU and host-pinned tensors are both host-addressable. Defaulting
// non-GPU places to memcpy also avoids treating a future host place as a
// CUDA pointer merely because the operator was built with CUDA support.
memcpy(dst, src, count * sizeof(FPTYPE));
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
#endif

#ifdef USE_MPI
Expand Down Expand Up @@ -83,13 +106,16 @@ void Border_forward_t(const paddle::Tensor& sendlist_tensor,

int tensor_size = g1.dims()[1];

// nlocal and nghost are scalar protocol values, independent of the number
// of communication swaps. In particular, nswap == 0 still needs one slot
// for each value before the host dereference below.
paddle::Tensor cpu_nlocal =
paddle::empty({nswap}, paddle::DataType::INT32, paddle::CPUPlace());
paddle::empty({1}, paddle::DataType::INT32, paddle::CPUPlace());
cpu_nlocal.copy_(nlocal_tensor, paddle::CPUPlace(), true);
int nlocal = *(cpu_nlocal.data<int>());

paddle::Tensor cpu_nghost =
paddle::empty({nswap}, paddle::DataType::INT32, paddle::CPUPlace());
paddle::empty({1}, paddle::DataType::INT32, paddle::CPUPlace());
cpu_nghost.copy_(nghost_tensor, paddle::CPUPlace(), true);
int nghost = *(cpu_nghost.data<int>());

Expand Down Expand Up @@ -175,20 +201,8 @@ void Border_forward_t(const paddle::Tensor& sendlist_tensor,
#endif

#if defined(GOOGLE_CUDA) || defined(TENSORFLOW_USE_ROCM)
#ifdef USE_MPI
if (cuda_aware == 0) {
memcpy(recv_g1, send_g1,
(unsigned long)nsend * tensor_size * sizeof(FPTYPE));
} else {
gpuMemcpy(recv_g1, send_g1,
(unsigned long)nsend * tensor_size * sizeof(FPTYPE),
gpuMemcpyDeviceToDevice);
}
#else
gpuMemcpy(recv_g1, send_g1,
(unsigned long)nsend * tensor_size * sizeof(FPTYPE),
gpuMemcpyDeviceToDevice);
#endif
copy_local_tensor_data(recv_g1, send_g1, (size_t)nsend * tensor_size,
recv_g1_tensor.place());

#else
memcpy(recv_g1, send_g1,
Expand Down Expand Up @@ -381,20 +395,8 @@ void Border_backward_t(const paddle::Tensor& sendlist_tensor,
#endif
if (nrecv) {
#if defined(GOOGLE_CUDA) || defined(TENSORFLOW_USE_ROCM)
#ifdef USE_MPI
if (cuda_aware == 0) {
memcpy(recv_g1, send_g1,
(unsigned long)nrecv * tensor_size * sizeof(FPTYPE));
} else {
gpuMemcpy(recv_g1, send_g1,
(unsigned long)nrecv * tensor_size * sizeof(FPTYPE),
gpuMemcpyDeviceToDevice);
}
#else
gpuMemcpy(recv_g1, send_g1,
(unsigned long)nrecv * tensor_size * sizeof(FPTYPE),
gpuMemcpyDeviceToDevice);
#endif
copy_local_tensor_data(recv_g1, send_g1, (size_t)nrecv * tensor_size,
d_local_g1_tensor.place());
#else
memcpy(recv_g1, send_g1,
(unsigned long)nrecv * tensor_size * sizeof(FPTYPE));
Expand Down
91 changes: 91 additions & 0 deletions source/tests/pd/test_border_op.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# SPDX-License-Identifier: LGPL-3.0-or-later
"""Regression tests for Paddle border-exchange control and data tensors."""

import numpy as np
import paddle
import pytest

deepmd_op_pd = pytest.importorskip(
"deepmd_op_pd", reason="the Paddle custom operator library is not built"
)


def _control_tensors(nswap: int) -> tuple[paddle.Tensor, ...]:
"""Create the common CPU control tensors for a border exchange."""
return (
paddle.zeros([nswap], dtype="int32"), # sendproc
paddle.zeros([nswap], dtype="int32"), # recvproc
paddle.zeros([nswap], dtype="int32"), # sendnum
paddle.zeros([nswap], dtype="int32"), # recvnum
paddle.zeros([1], dtype="int64"), # unused communicator without MPI
)


def test_border_op_accepts_no_swaps() -> None:
"""Scalar atom counts must remain readable when ``nswap == 0``."""
sendproc, recvproc, sendnum, recvnum, communicator = _control_tensors(0)
g1 = paddle.arange(6, dtype="float64").reshape([2, 3])

result = deepmd_op_pd.border_op(
paddle.zeros([0], dtype="int64"),
sendproc,
recvproc,
sendnum,
recvnum,
g1,
communicator,
paddle.to_tensor([2], dtype="int32"),
paddle.to_tensor([0], dtype="int32"),
)

np.testing.assert_array_equal(result.numpy(), g1.numpy())


def test_border_op_self_copy_uses_cpu_place() -> None:
"""A CUDA-enabled operator must not use a GPU copy for CPU tensors.

NOTE: no CI job currently builds Paddle with CUDA (``test_cuda.yml``
disables Paddle at the workflow level and ``test_python.yml`` installs the
CPU build), so ``copy_local_tensor_data`` is not compiled-and-executed by
any pipeline. This test therefore documents the intended CPU-branch
behavior rather than guarding it; it should gain real coverage once a CI
job builds Paddle with CUDA.
"""
# CUDA Paddle builds otherwise create tensors on the default GPU, which
# would leave the operator's CPU copy branch untested.
paddle.set_device("cpu")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This line does what I asked, but the job it is written for does not exist.

The reply says "the CUDA Paddle CI job will execute the operator path". There is no such job. .github/workflows/test_cuda.yml sets DP_ENABLE_PADDLE: "0" at workflow level, so both GPU jobs build without Paddle, and .github/workflows/test_python.yml installs Paddle from the cpu nightly index (.../packages/nightly/cpu/paddlepaddle/). Those are the only two places Paddle appears in CI.

Meanwhile copy_local_tensor_data is inside #if defined(GOOGLE_CUDA) || defined(TENSORFLOW_USE_ROCM). Putting those together:

  • CPU Test Python job: Paddle present, but GOOGLE_CUDA undefined, so this helper is not compiled and the test exercises the untouched #else path.
  • GPU jobs: helper would compile, but Paddle is disabled, so the custom op is not built and pytest.importorskip skips the file.

So there is no configuration in which the function this PR changes is both compiled and executed. I checked locally too -- paddle 3.3.1, is_compiled_with_cuda() false, ENABLE_CUSTOMIZED_OP false -- so it is not runnable by hand either.

I owe a correction on my own earlier comment: I wrote "on the CUDA job (the only one that compiles it)", which implied such a job existed and that the only problem was tensor placement. That was wrong. The placement fix was necessary but it cannot be sufficient while no job builds Paddle with CUDA.

Nothing here is yours to fix in this PR, and I am not asking you to. But it does mean the regression cannot fail on unpatched code anywhere today, so it documents the intended behaviour rather than protecting it. Two things that would change that, either of them separate work:

  • A CI configuration that builds Paddle with CUDA, which would give this test and the rest of source/op/pd/ real coverage.
  • Or making the CPU path compile unconditionally -- the is_gpu_place branch is only meaningful when a GPU place is possible, but the surrounding function could be built on CPU-only Paddle so the #else memcpy and the placement logic are at least exercised together.

If neither is on the cards soon, a one-line comment in the test saying it is currently unexecuted in CI would stop the next reader from assuming the green tick covers it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed on all counts — thank you for the correction about no CUDA Paddle job existing. I added a note to the test declaring that no pipeline currently compiles-and-executes copy_local_tensor_data, so the CPU-branch regression is documented rather than guarded (commit d4843dd). I also filed the CI-coverage gap as #5952, since giving source/op/pd/ real coverage needs a job that builds Paddle with CUDA (out of scope for this PR).

Coding agent: opencode
opencode version: 1.18.9
Model: ustc/deepseek-v4-flash
Reasoning effort: max

sendproc, recvproc, sendnum, recvnum, communicator = _control_tensors(1)
sendnum = paddle.ones_like(sendnum)
recvnum = paddle.ones_like(recvnum)

# The C++ operator receives the LAMMPS send lists as pointer-valued int64
# entries. Keep this NumPy owner alive through the call so the pointed-to
# int32 index remains valid.
send_indices = np.array([1], dtype=np.int32)
sendlist = paddle.to_tensor([send_indices.ctypes.data], dtype="int64")
g1_leaf = paddle.to_tensor(
Comment thread
njzjz-bot marked this conversation as resolved.
[[1.0, 2.0], [3.0, 4.0], [0.0, 0.0]], stop_gradient=False
)
# Paddle rejects an in-place custom op on an autograd leaf. This identity
# keeps a leaf for checking gradients while letting border_op update g1.
g1 = g1_leaf * 1.0

result = deepmd_op_pd.border_op(
sendlist,
sendproc,
recvproc,
sendnum,
recvnum,
g1,
communicator,
paddle.to_tensor([2], dtype="int32"),
paddle.to_tensor([1], dtype="int32"),
)

np.testing.assert_array_equal(
result.numpy(), np.array([[1.0, 2.0], [3.0, 4.0], [3.0, 4.0]])
)
# Backpropagation runs the reverse self-swap, which needs the same
# place-based CPU/GPU dispatch as the forward copy.
result.sum().backward()
np.testing.assert_array_equal(g1_leaf.grad.numpy(), np.ones([3, 2]))
Loading