Skip to content

Commit

Permalink
Raise explicit errors for when PyPy won't work
Browse files Browse the repository at this point in the history
  • Loading branch information
milesgranger committed Mar 2, 2024
1 parent cc3800c commit 32f75f1
Show file tree
Hide file tree
Showing 8 changed files with 81 additions and 34 deletions.
19 changes: 10 additions & 9 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,10 @@ jobs:
- '3.12'
target: [aarch64, armv7, s390x, ppc64le]
include:
- python: pypy3.7
target: aarch64
- python: pypy3.8
target: aarch64
- python: pypy3.9
target: aarch64
- python: pypy3.10
target: aarch64
steps:
- uses: actions/checkout@v3
- uses: actions/cache@v3
Expand Down Expand Up @@ -269,9 +267,8 @@ jobs:
strategy:
matrix:
python:
- pypy-3.7
- pypy-3.8
- pypy-3.9
- pypy-3.10
steps:
- uses: actions/checkout@v3
- uses: actions/cache@v3
Expand Down Expand Up @@ -301,7 +298,8 @@ jobs:
- name: Python UnitTest - cramjam-python
run: |
pip install cramjam --no-index --find-links dist
pypy -c "import cramjam"
pip install .[dev]
python -m pytest tests -v
- name: Python cramjam-cli test
run: |
pip install cramjam-cli --no-index --find-links dist
Expand All @@ -316,7 +314,7 @@ jobs:
runs-on: macos-latest
strategy:
matrix:
python-version: [ pypy-3.7, pypy-3.8, pypy-3.9 ]
python-version: [ pypy-3.9, pypy-3.10 ]
steps:
- uses: actions/checkout@v3
- uses: actions/cache@v3
Expand Down Expand Up @@ -352,7 +350,10 @@ jobs:
pip install cramjam --no-index --find-links dist
pip install cramjam-cli --no-index --find-links dist
- name: Python Import test
run: pypy -c "import cramjam"
run: |
pip install cramjam --no-index --find-links dist
pip install .[dev]
python -m pytest tests -v
- name: Python cramjam-cli test
run: cramjam-cli --help
- name: Upload wheels
Expand Down
32 changes: 24 additions & 8 deletions cramjam-python/src/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::path::PathBuf;

pub(crate) trait AsBytes {
fn as_bytes(&self) -> &[u8];
fn as_bytes_mut(&mut self) -> &mut [u8];
fn as_bytes_mut(&mut self) -> PyResult<&mut [u8]>;
}

/// A native Rust file-like object. Reading and writing takes place
Expand Down Expand Up @@ -49,7 +49,7 @@ impl AsBytes for RustyFile {
entire file into memory; consider using cramjam.Buffer"
)
}
fn as_bytes_mut(&mut self) -> &mut [u8] {
fn as_bytes_mut(&mut self) -> PyResult<&mut [u8]> {
unimplemented!(
"Converting a File to bytes is not supported, as it'd require reading the \
entire file into memory; consider using cramjam.Buffer"
Expand Down Expand Up @@ -173,6 +173,8 @@ impl RustyFile {
pub struct PythonBuffer {
pub(crate) inner: std::pin::Pin<Box<ffi::Py_buffer>>,
pub(crate) pos: usize,
#[cfg(PyPy)]
pub(crate) owner: PyObject,
}
// PyBuffer is thread-safe: the shape of the buffer is immutable while a Py_buffer exists.
// Accessing the buffer contents is protected using the GIL.
Expand All @@ -194,21 +196,32 @@ impl PythonBuffer {
}
/// Is the Python buffer readonly
pub fn readonly(&self) -> bool {
self.inner.readonly != 0
self.inner.readonly == 1
}
/// Get the underlying buffer as a slice of bytes
pub fn as_slice(&self) -> &[u8] {
unsafe { std::slice::from_raw_parts(self.buf_ptr() as *const u8, self.len_bytes()) }
}
/// Get the underlying buffer as a mutable slice of bytes
pub fn as_slice_mut(&mut self) -> PyResult<&mut [u8]> {
// TODO: For v3 release, add self.readonly check; bytes is readonly but
// v1 and v2 releases have not treated it as such.
#[cfg(PyPy)]
{
Python::with_gil(|py| {
let is_memoryview = unsafe { ffi::PyMemoryView_Check(self.owner.as_ptr()) } == 1;
if is_memoryview || self.owner.as_ref(py).is_instance_of::<PyBytes>() {
Err(pyo3::exceptions::PyTypeError::new_err(
"With PyPy, an output of type `bytes` or `memoryview` does not work. See issue pypy/pypy#4918",
))
} else {
Ok(())
}
})?;
}
Ok(unsafe { std::slice::from_raw_parts_mut(self.buf_ptr() as *mut u8, self.len_bytes()) })
}
/// If underlying buffer is c_contiguous
pub fn is_c_contiguous(&self) -> bool {
unsafe { ffi::PyBuffer_IsContiguous(&*self.inner as *const ffi::Py_buffer, b'C' as std::os::raw::c_char) != 0 }
unsafe { ffi::PyBuffer_IsContiguous(&*self.inner as *const ffi::Py_buffer, b'C' as std::os::raw::c_char) == 1 }
}
/// Dimensions for buffer
pub fn dimensions(&self) -> usize {
Expand Down Expand Up @@ -258,6 +271,8 @@ impl<'py> TryFrom<&'py PyAny> for PythonBuffer {
let buf = Self {
inner: std::pin::Pin::from(buf),
pos: 0,
#[cfg(PyPy)]
owner: Python::with_gil(|py| obj.to_object(py)),
};
// sanity checks
if buf.inner.shape.is_null() {
Expand Down Expand Up @@ -328,8 +343,9 @@ impl AsBytes for RustyBuffer {
fn as_bytes(&self) -> &[u8] {
self.inner.get_ref().as_slice()
}
fn as_bytes_mut(&mut self) -> &mut [u8] {
self.inner.get_mut().as_mut_slice()
fn as_bytes_mut(&mut self) -> PyResult<&mut [u8]> {
let slice = self.inner.get_mut().as_mut_slice();
Ok(slice)
}
}

Expand Down
16 changes: 8 additions & 8 deletions cramjam-python/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,18 +101,18 @@ impl<'a> AsBytes for BytesType<'a> {
}
}
}
fn as_bytes_mut(&mut self) -> &mut [u8] {
fn as_bytes_mut(&mut self) -> PyResult<&mut [u8]> {
match self {
BytesType::RustyBuffer(b) => {
let mut py_ref = b.borrow_mut();
let bytes = py_ref.as_bytes_mut();
unsafe { std::slice::from_raw_parts_mut(bytes.as_mut_ptr(), bytes.len()) }
let bytes = py_ref.as_bytes_mut()?;
Ok(unsafe { std::slice::from_raw_parts_mut(bytes.as_mut_ptr(), bytes.len()) })
}
BytesType::PyBuffer(b) => b.as_slice_mut().unwrap(),
BytesType::PyBuffer(b) => b.as_slice_mut(),
BytesType::RustyFile(b) => {
let mut py_ref = b.borrow_mut();
let bytes = py_ref.as_bytes_mut();
unsafe { std::slice::from_raw_parts_mut(bytes.as_mut_ptr(), bytes.len()) }
let bytes = py_ref.as_bytes_mut()?;
Ok(unsafe { std::slice::from_raw_parts_mut(bytes.as_mut_ptr(), bytes.len()) })
}
}
}
Expand Down Expand Up @@ -212,7 +212,7 @@ macro_rules! generic {
})
},
_ => {
let bytes_out = $output.as_bytes_mut();
let bytes_out = $output.as_bytes_mut()?;
$py.allow_threads(|| {
$op(f_in, &mut Cursor::new(bytes_out) $(, $level)? )
})
Expand All @@ -237,7 +237,7 @@ macro_rules! generic {
})
},
_ => {
let bytes_out = $output.as_bytes_mut();
let bytes_out = $output.as_bytes_mut()?;
$py.allow_threads(|| {
$op(bytes_in, &mut Cursor::new(bytes_out) $(, $level)?)
})
Expand Down
4 changes: 2 additions & 2 deletions cramjam-python/src/lz4.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ pub fn compress_block(
#[pyfunction]
pub fn decompress_block_into(py: Python, input: BytesType, mut output: BytesType) -> PyResult<usize> {
let bytes = input.as_bytes();
let out_bytes = output.as_bytes_mut();
let out_bytes = output.as_bytes_mut()?;
py.allow_threads(|| libcramjam::lz4::block::decompress_into(bytes, out_bytes, Some(true)))
.map_err(DecompressionError::from_err)
.map(|v| v as _)
Expand Down Expand Up @@ -180,7 +180,7 @@ pub fn compress_block_into(
store_size: Option<bool>,
) -> PyResult<usize> {
let bytes = data.as_bytes();
let out_bytes = output.as_bytes_mut();
let out_bytes = output.as_bytes_mut()?;
py.allow_threads(|| {
libcramjam::lz4::block::compress_into(bytes, out_bytes, compression.map(|v| v as _), acceleration, store_size)
})
Expand Down
4 changes: 2 additions & 2 deletions cramjam-python/src/snappy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ pub fn decompress_into(py: Python, input: BytesType, mut output: BytesType) -> P
#[pyfunction]
pub fn compress_raw_into(py: Python, input: BytesType, mut output: BytesType) -> PyResult<usize> {
let bytes_in = input.as_bytes();
let bytes_out = output.as_bytes_mut();
let bytes_out = output.as_bytes_mut()?;
py.allow_threads(|| libcramjam::snappy::raw::compress(bytes_in, bytes_out))
.map_err(CompressionError::from_err)
}
Expand All @@ -109,7 +109,7 @@ pub fn compress_raw_into(py: Python, input: BytesType, mut output: BytesType) ->
#[pyfunction]
pub fn decompress_raw_into(py: Python, input: BytesType, mut output: BytesType) -> PyResult<usize> {
let bytes_in = input.as_bytes();
let bytes_out = output.as_bytes_mut();
let bytes_out = output.as_bytes_mut()?;
py.allow_threads(|| libcramjam::snappy::raw::decompress(bytes_in, bytes_out))
.map_err(DecompressionError::from_err)
}
Expand Down
8 changes: 8 additions & 0 deletions cramjam-python/tests/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import platform
import pytest


@pytest.fixture(scope='session')
def is_pypy():
impl = platform.python_implementation()
return impl.lower() == 'pypy'
7 changes: 6 additions & 1 deletion cramjam-python/tests/test_rust_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@


@pytest.mark.parametrize("Obj", (File, Buffer))
def test_obj_api(tmpdir, Obj):
def test_obj_api(tmpdir, Obj, is_pypy):
if isinstance(Obj, File):
buf = File(str(tmpdir.join("file.txt")))
else:
Expand All @@ -30,6 +30,11 @@ def test_obj_api(tmpdir, Obj):
):
buf.seek(0)

if isinstance(out, bytes) and is_pypy:
with pytest.raises(OSError):
buf.readinto(out)
continue

expected = b"bytes"

buf.readinto(out)
Expand Down
25 changes: 21 additions & 4 deletions cramjam-python/tests/test_variants.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def test_has_version():

@pytest.mark.parametrize("variant_str", VARIANTS)
@given(arr=st_np.arrays(st_np.scalar_dtypes(), shape=st.integers(0, int(1e4))))
def test_variants_different_dtypes(variant_str, arr):
def test_variants_different_dtypes(variant_str, arr, is_pypy):
variant = getattr(cramjam, variant_str)
compressed = variant.compress(arr)
decompressed = variant.decompress(compressed)
Expand All @@ -49,7 +49,14 @@ def test_variants_different_dtypes(variant_str, arr):
# And compress n dims > 1
if arr.shape[0] % 2 == 0:
arr = arr.reshape((2, -1))
compressed = variant.compress(arr)

if is_pypy:
try:
compressed = variant.compress(arr)
except:
pytest.xfail(reason="PyPy struggles w/ multidim buffer views depending on dtype ie datetime[64]")
else:
compressed = variant.compress(arr)
decompressed = variant.decompress(compressed)
assert same_same(bytes(decompressed), arr.tobytes())

Expand Down Expand Up @@ -89,7 +96,7 @@ def test_variants_raise_exception(variant_str):
@pytest.mark.parametrize("variant_str", VARIANTS)
@given(raw_data=st.binary())
def test_variants_compress_into(
variant_str, input_type, output_type, raw_data, tmp_path_factory
variant_str, input_type, output_type, raw_data, tmp_path_factory, is_pypy
):
variant = getattr(cramjam, variant_str)

Expand Down Expand Up @@ -124,6 +131,11 @@ def test_variants_compress_into(
else:
output = output_type(b"0" * compressed_len)

if is_pypy and isinstance(output, (bytes, memoryview)):
with pytest.raises(TypeError):
variant.compress_into(input, output)
return

n_bytes = variant.compress_into(input, output)
assert n_bytes == compressed_len

Expand All @@ -146,7 +158,7 @@ def test_variants_compress_into(
@pytest.mark.parametrize("variant_str", VARIANTS)
@given(raw_data=st.binary())
def test_variants_decompress_into(
variant_str, input_type, output_type, tmp_path_factory, raw_data
variant_str, input_type, output_type, tmp_path_factory, raw_data, is_pypy
):
variant = getattr(cramjam, variant_str)

Expand Down Expand Up @@ -180,6 +192,11 @@ def test_variants_decompress_into(
else:
output = output_type(b"0" * len(raw_data))

if is_pypy and isinstance(output, (bytes, memoryview)):
with pytest.raises(TypeError):
variant.decompress_into(input, output)
return

n_bytes = variant.decompress_into(input, output)
assert n_bytes == len(raw_data)

Expand Down

0 comments on commit 32f75f1

Please sign in to comment.