fix: more review

Author: jharlow-intel

dpctl/_sycl_queue.pyx

@@ -584,20 +584,14 @@ cdef DPCTLSyclEventRef _copy_impl(
      SyclQueue q,
      object dst,
      object src,
-     size_t byte_count,
+     size_t count,
      DPCTLSyclEventRef *dep_events,
      size_t dep_events_count,
-     str dtype=None
+     str dtype="u1"
 ) except *:
-    cdef size_t element_size = 0
-
-    if dtype is not None:
-        element_size = _get_dtype_size(dtype)
-        if byte_count % element_size != 0:
-            raise ValueError(
-                f"byte_count ({byte_count}) must be a multiple of dtype "
-                f"element size ({element_size} bytes for '{dtype}')"
-            )
+    # ``count`` is in elements of ``dtype`` (default "u1" => bytes).
+    cdef size_t element_size = _get_dtype_size(dtype)
+    cdef size_t byte_count = count * element_size
 
     return _copy_memcpy_impl(
         q, dst, src, byte_count, dep_events, dep_events_count,
@@ -1495,11 +1489,14 @@ cdef class SyclQueue(_SyclQueue):
 
         return SyclEvent._create(ERef)
 
-    cpdef copy(self, dest, src, size_t count, str dtype=None):
-        """Copy ``count`` bytes from ``src`` to ``dest`` and wait.
+    cpdef copy(self, dest, src, size_t count, str dtype="u1"):
+        """Copy ``count`` elements of type ``dtype`` from ``src`` to
+        ``dest`` and wait.
 
-        Internally, this dispatches ``sycl::queue::copy`` instantiated for
-        byte-sized elements (or typed elements if dtype is specified).
+        Internally, this dispatches ``sycl::queue::copy``. The number of
+        bytes transferred is ``count`` multiplied by the size of ``dtype``.
+        The default ``dtype`` of ``"u1"`` (a single byte) makes the default
+        a byte-wise copy.
 
         This is a synchronizing variant corresponding to
         :meth:`dpctl.SyclQueue.copy_async`.
@@ -1512,11 +1509,12 @@ cdef class SyclQueue(_SyclQueue):
                 Source USM object or Python object supporting buffer
                 protocol.
             count (int):
-                Number of bytes to copy.
+                Number of elements to copy.
             dtype (str, optional):
-                Data type string (e.g., 'i4', 'f8') for typed copy
-                validation. If provided, validates that count is a
-                multiple of the element size.
+                Data type string of the elements to copy. Determines the
+                element size used to convert ``count`` into a byte count.
+                Defaults to ``"u1"`` (one byte per element).
+                Supported types: i1, u1, i2, u2, i4, u4, i8, u8, f4, f8.
         """
         cdef DPCTLSyclEventRef ERef = NULL
 
@@ -1530,12 +1528,15 @@ cdef class SyclQueue(_SyclQueue):
         DPCTLEvent_Delete(ERef)
 
     cpdef SyclEvent copy_async(
-        self, dest, src, size_t count, list dEvents=None, str dtype=None
+        self, dest, src, size_t count, list dEvents=None, str dtype="u1"
     ):
-        """Copy ``count`` bytes from ``src`` to ``dest`` asynchronously.
+        """Copy ``count`` elements of type ``dtype`` from ``src`` to
+        ``dest`` asynchronously.
 
-        Internally, this dispatches ``sycl::queue::copy`` instantiated for
-        byte-sized elements (or typed elements if dtype is specified).
+        Internally, this dispatches ``sycl::queue::copy``. The number of
+        bytes transferred is ``count`` multiplied by the size of ``dtype``.
+        The default ``dtype`` of ``"u1"`` (a single byte) makes the default
+        a byte-wise copy.
 
         Args:
             dest:
@@ -1545,13 +1546,13 @@ cdef class SyclQueue(_SyclQueue):
                 Source USM object or Python object supporting buffer
                 protocol.
             count (int):
-                Number of bytes to copy.
+                Number of elements to copy.
             dEvents (List[dpctl.SyclEvent], optional):
                 Events that this copy depends on.
             dtype (str, optional):
-                Data type string (e.g., 'i4', 'f8') for typed copy
-                validation. If provided, validates that count is a
-                multiple of the element size.
+                Data type string of the elements to copy. Determines the
+                element size used to convert ``count`` into a byte count.
+                Defaults to ``"u1"`` (one byte per element).
                 Supported types: i1, u1, i2, u2, i4, u4, i8, u8, f4, f8.
 
         Returns:

dpctl/tests/test_sycl_queue_copy.py

@@ -122,8 +122,8 @@ def test_copy_with_dtype_valid(dtype, element_size, usm_type):
         host_buf = bytearray(i % 256 for i in range(nbytes))
         src.copy_from_host(host_buf)
 
-        # Copy device to device with dtype validation
-        q.copy(dst, src, nbytes, dtype=dtype)
+        # Copy device to device, count given in elements of dtype
+        q.copy(dst, src, num_elements, dtype=dtype)
 
         # Verify via host buffer
         result_buf = bytearray(nbytes)
@@ -135,8 +135,8 @@ def test_copy_with_dtype_valid(dtype, element_size, usm_type):
         for i in range(nbytes):
             src_mv[i] = i % 256
 
-        # Copy with dtype validation
-        q.copy(dst, src, nbytes, dtype=dtype)
+        # Copy, count given in elements of dtype
+        q.copy(dst, src, num_elements, dtype=dtype)
 
         # Verify
         dst_mv = memoryview(dst)
@@ -163,8 +163,8 @@ def test_copy_async_with_dtype_valid():
     for i in range(nbytes):
         src_mv[i] = i % 256
 
-    # Async copy with dtype
-    e = q.copy_async(dst, src, nbytes, dtype=dtype)
+    # Async copy with dtype, count given in elements
+    e = q.copy_async(dst, src, num_elements, dtype=dtype)
     e.wait()
 
     # Verify
@@ -173,47 +173,38 @@ def test_copy_async_with_dtype_valid():
 
 
 @pytest.mark.parametrize(
-    "dtype,element_size,bad_count",
+    "dtype,element_size",
     [
-        ("i4", 4, 7),  # 7 is not divisible by 4
-        ("f8", 8, 13),
-        ("i2", 2, 5),
-        ("u8", 8, 3),
+        ("i2", 2),
+        ("i4", 4),
+        ("f8", 8),
+        ("u8", 8),
     ],
 )
-def test_copy_with_dtype_invalid_count(dtype, element_size, bad_count):
-    """Test copy raises ValueError when count isn't a dtype multiple."""
+def test_copy_count_is_in_elements(dtype, element_size):
+    """``count`` is interpreted as a number of elements of ``dtype``."""
     try:
         q = dpctl.SyclQueue()
     except dpctl.SyclQueueCreationError:
         pytest.skip("Default constructor for SyclQueue failed")
 
-    nbytes = 64
+    num_elements = 4
+    nbytes = num_elements * element_size
+
     src = dpctl.memory.MemoryUSMShared(nbytes, queue=q)
     dst = dpctl.memory.MemoryUSMShared(nbytes, queue=q)
 
-    with pytest.raises(ValueError) as cm:
-        q.copy(dst, src, bad_count, dtype=dtype)
-    assert "multiple" in str(cm.value).lower()
-    assert str(element_size) in str(cm.value)
-
-
-def test_copy_async_with_dtype_invalid_count():
-    """Test that copy_async raises ValueError for invalid count."""
-    try:
-        q = dpctl.SyclQueue()
-    except dpctl.SyclQueueCreationError:
-        pytest.skip("Default constructor for SyclQueue failed")
+    src_mv = memoryview(src)
+    for i in range(nbytes):
+        src_mv[i] = i % 256
+    dst_mv = memoryview(dst)
 
-    dtype = "i4"
-    bad_count = 7  # Not divisible by 4
-    nbytes = 64
-    src = dpctl.memory.MemoryUSMShared(nbytes, queue=q)
-    dst = dpctl.memory.MemoryUSMShared(nbytes, queue=q)
+    # Copying half the elements transfers exactly half the bytes.
+    q.copy(dst, src, num_elements // 2, dtype=dtype)
 
-    with pytest.raises(ValueError) as cm:
-        q.copy_async(dst, src, bad_count, dtype=dtype)
-    assert "multiple" in str(cm.value).lower()
+    half_bytes = (num_elements // 2) * element_size
+    assert dst_mv[:half_bytes].tobytes() == src_mv[:half_bytes].tobytes()
+    assert dst_mv[half_bytes:nbytes].tobytes() == bytes(nbytes - half_bytes)
 
 
 def test_copy_with_invalid_dtype():
@@ -247,13 +238,11 @@ def test_copy_with_dtype_host_buffers():
 
     dtype = "f4"
     num_elements = 20
-    element_size = 4
-    nbytes = num_elements * element_size
 
     src = np.arange(num_elements, dtype=np.float32)
     dst = np.zeros(num_elements, dtype=np.float32)
 
-    q.copy(dst, src, nbytes, dtype=dtype)
+    q.copy(dst, src, num_elements, dtype=dtype)
 
     assert np.array_equal(dst, src)
 
@@ -267,18 +256,17 @@ def test_copy_with_dtype_mixed_sources():
 
     dtype = "i8"
     num_elements = 10
-    element_size = 8
-    nbytes = num_elements * element_size
+    nbytes = num_elements * 8
 
     # Host to USM
     src_host = np.arange(num_elements, dtype=np.int64)
     dst_usm = dpctl.memory.MemoryUSMShared(nbytes, queue=q)
 
-    q.copy(dst_usm, src_host, nbytes, dtype=dtype)
+    q.copy(dst_usm, src_host, num_elements, dtype=dtype)
 
     # USM to host
     dst_host = np.zeros(num_elements, dtype=np.int64)
-    q.copy(dst_host, dst_usm, nbytes, dtype=dtype)
+    q.copy(dst_host, dst_usm, num_elements, dtype=dtype)
 
     assert np.array_equal(dst_host, src_host)
 

libsyclinterface/source/dpctl_sycl_queue_interface.cpp

@@ -704,7 +704,7 @@ DPCTLQueue_CopyData(__dpctl_keep const DPCTLSyclQueueRef QRef,
     if (Q) {
         sycl::event ev;
         try {
-            // Bind queue::copy with uint8_t so Count is interpreted as bytes.
+            // Copy uint8_t elements (1 byte each), so Count is a byte count.
             ev = Q->copy(static_cast<const std::uint8_t *>(Src),
                          static_cast<std::uint8_t *>(Dest), Count);
         } catch (std::exception const &e) {
@@ -741,7 +741,7 @@ DPCTLQueue_CopyDataWithEvents(__dpctl_keep const DPCTLSyclQueueRef QRef,
                 }
             }
 
-            // Bind queue::copy with uint8_t so Count is interpreted as bytes.
+            // Copy uint8_t elements (1 byte each), so Count is a byte count.
             auto ev =
                 Q->copy(static_cast<const std::uint8_t *>(Src),
                         static_cast<std::uint8_t *>(Dest), Count, dep_events);