Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python tests 2: Streaming boogaloo #12

Merged
merged 38 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
d6d47cf
bindings created
aliddell Oct 8, 2024
e85d185
pip install . working
aliddell Oct 9, 2024
2b648c1
python -m build working (on Windows anyway)
aliddell Oct 9, 2024
2ef675c
get it building on linux
aliddell Oct 9, 2024
9a44c42
Remove test stub (save for the next PR)
aliddell Oct 9, 2024
3978d55
Undo an overzealous rename
aliddell Oct 9, 2024
2a4c0a8
Add Python wheel build job
aliddell Oct 9, 2024
7ed144e
Prepare for Python bindings
aliddell Oct 9, 2024
68d3b0b
Merge branch 'python-bindings-prep' into python-bindings
aliddell Oct 9, 2024
c160d7c
Don't export the enum values into the module base namespace.
aliddell Oct 9, 2024
7e51977
(wip) some basic tests
aliddell Oct 9, 2024
8c76e2d
Revert CMake minimum version and use cmake_policy. Using builtin `BUI…
aliddell Oct 9, 2024
60a0bd5
Update build.yml
aliddell Oct 9, 2024
9c5c6bf
Update release.yml
aliddell Oct 9, 2024
1a9d1be
Merge branch 'python-bindings-prep' into python-bindings
aliddell Oct 9, 2024
2dc2fa6
Merge remote-tracking branch 'upstream/python-bindings-prep' into pyt…
aliddell Oct 9, 2024
f3b1e84
Merge branch 'python-bindings' into python-tests
aliddell Oct 9, 2024
40b14ad
Merge remote-tracking branch 'upstream/main' into python-bindings
aliddell Oct 9, 2024
50f889d
Merge remote-tracking branch 'upstream/python-bindings' into python-t…
aliddell Oct 9, 2024
8faabe5
some simple tests
aliddell Oct 10, 2024
7e97dd7
add python tests to CI
aliddell Oct 10, 2024
71aacd8
(wip): first stream test (crash!)
aliddell Oct 10, 2024
e219b0b
Merge remote-tracking branch 'upstream/main' into python-tests
aliddell Oct 10, 2024
f385549
Merge branch 'python-tests' into python-tests-2
aliddell Oct 10, 2024
4deaecd
Remove deprecated workflows
aliddell Oct 10, 2024
30a844c
A rectification of names
aliddell Oct 11, 2024
70f4f47
whitespace
aliddell Oct 11, 2024
b68a634
Fix double free error
aliddell Oct 11, 2024
28b28c6
Slightly improve debug output.
aliddell Oct 11, 2024
6aabdd3
Add some more streaming tests.
aliddell Oct 11, 2024
30f8ad2
Merge remote-tracking branch 'upstream/main' into python-tests-2
aliddell Oct 14, 2024
2969c6a
concurrency?
aliddell Oct 14, 2024
8e8aba8
Explicit nullopt
aliddell Oct 14, 2024
946c633
Merge remote-tracking branch 'upstream/main' into python-tests-2
aliddell Oct 24, 2024
08e95ed
Get it working ok on Windows
aliddell Oct 24, 2024
597e49c
remove stub
aliddell Oct 25, 2024
eee52c1
Use PROJECT_SOURCE_DIR instead of CMAKE_SOURCE_DIR
aliddell Oct 28, 2024
d6af9b6
Respond to PR comments
aliddell Oct 28, 2024
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
38 changes: 17 additions & 21 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@ on:
push:
branches:
- "main"
pull_request: # TODO (aliddell): remove this
branches:
- "main"

jobs:
windows-and-linux-build:
name: Build on ${{ matrix.platform }} with ${{ matrix.build_type }} configuration
strategy:
matrix:
build_type:
Expand All @@ -29,12 +27,11 @@ jobs:
permissions:
actions: write

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

steps:
- uses: actions/checkout@v3
with:
submodules: true
Expand All @@ -58,7 +55,7 @@ jobs:
cmake --build ${{github.workspace}}/build --config ${{matrix.build_type}}
cpack --config ${{github.workspace}}/build/CPackConfig.cmake -C ${{matrix.build_type}} -G ZIP

- uses: actions/upload-artifact@v3
- uses: actions/upload-artifact@v4
with:
name: ${{matrix.platform}} ${{matrix.build_type}} binaries
path: ${{github.workspace}}/*.zip
Expand All @@ -75,12 +72,11 @@ jobs:
permissions:
actions: write

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

steps:
- uses: actions/checkout@v3
with:
submodules: true
Expand Down Expand Up @@ -114,7 +110,7 @@ jobs:
run: |
cpack --config ${{github.workspace}}/build/CPackConfig.cmake -C ${{matrix.build_type}} -G ZIP

- uses: actions/upload-artifact@v3
- uses: actions/upload-artifact@v4
with:
name: macos-latest ${{matrix.build_type}} binaries
path: ${{github.workspace}}/*.zip
Expand All @@ -132,12 +128,11 @@ jobs:
permissions:
actions: write

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

steps:
- uses: actions/checkout@v3
with:
submodules: true
Expand All @@ -163,6 +158,7 @@ jobs:
run: python -m build

- name: Upload wheel
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: ${{matrix.platform}} wheel
path: ${{github.workspace}}/dist/*.whl
24 changes: 12 additions & 12 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ env:

jobs:
windows-and-linux-build:
name: Build on ${{ matrix.platform }}
strategy:
matrix:
platform:
Expand All @@ -27,12 +28,11 @@ jobs:
permissions:
actions: write

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

steps:
- uses: actions/checkout@v3
with:
submodules: true
Expand Down Expand Up @@ -61,23 +61,23 @@ jobs:
cmake --build ${{github.workspace}}/pack --config Release
cpack --config ${{github.workspace}}/pack/CPackConfig.cmake -C Release -G ZIP

- uses: actions/upload-artifact@v3
- uses: actions/upload-artifact@v4
with:
name: ${{matrix.platform}} binaries
path: ${{github.workspace}}/*.zip

mac-build:
name: Build on macos-latest
runs-on: "macos-latest"

permissions:
actions: write

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

steps:
- uses: actions/checkout@v3
with:
submodules: true
Expand Down Expand Up @@ -115,7 +115,7 @@ jobs:
run: |
cpack --config ${{github.workspace}}/build/CPackConfig.cmake -C Release -G ZIP

- uses: actions/upload-artifact@v3
- uses: actions/upload-artifact@v4
with:
name: macos-latest binaries
path: ${{github.workspace}}/*.zip
Expand Down
24 changes: 6 additions & 18 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ on:
env:
BUILD_TYPE: Release

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

jobs:
test:
name: Test on ${{ matrix.platform }}
Expand All @@ -34,11 +38,6 @@ jobs:
actions: write

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}

- uses: actions/checkout@v3
with:
submodules: true
Expand Down Expand Up @@ -78,11 +77,6 @@ jobs:
MINIO_SECRET_KEY: 12345678

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}

- uses: actions/checkout@v3
with:
submodules: true
Expand Down Expand Up @@ -135,7 +129,7 @@ jobs:
run: ctest -C ${{env.BUILD_TYPE}} -L s3 --output-on-failure

test_python:
name: Test on ${{ matrix.platform }}
name: Test Python on ${{ matrix.platform }}
runs-on: ${{ matrix.platform }}
timeout-minutes: 20
strategy:
Expand All @@ -147,11 +141,6 @@ jobs:
- "macos-latest"

steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}

- uses: actions/checkout@v3
with:
submodules: true
Expand All @@ -175,8 +164,7 @@ jobs:
run: python -m pip install -U pip "pybind11[global]" cmake build numpy pytest

- name: Build and install Python bindings
run: python -m pip install .
run: python -m pip install ".[testing]"

- name: Run tests
run: python -m pytest -v

33 changes: 30 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,13 +1,40 @@
[build-system]
requires = [
"setuptools>=42",
"wheel",
"ninja",
"cmake>=3.12",
"ninja",
"pybind11[global]",
"setuptools>=42",
"wheel",
]
build-backend = "setuptools.build_meta"

[project]
name = "acquire-zarr"
requires-python = ">=3.9"
version = "0.0.1"

[project.optional-dependencies]
testing = [
"black",
"dask",
"mypy",
"ome-zarr",
"pytest>=7",
"pytest-cov",
"python-dotenv",
"ruff",
"s3fs",
"tifffile",
"zarr",
]

[tool.black]
target-version = ['py39', 'py310', 'py311', 'py312']
line-length = 79

[tool.isort]
profile = "black"

[tool.pytest.ini_options]
minversion = "6.0"
addopts = "-ra -q --color=yes"
Expand Down
2 changes: 1 addition & 1 deletion python/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ find_package(pybind11 REQUIRED)

pybind11_add_module(acquire_zarr acquire-zarr-py.cpp)

target_include_directories(acquire_zarr PRIVATE ${CMAKE_SOURCE_DIR}/include)
target_include_directories(acquire_zarr PRIVATE ${PROJECT_SOURCE_DIR}/include)
target_link_libraries(acquire_zarr PRIVATE acquire-zarr)

set_target_properties(acquire_zarr PROPERTIES
Expand Down
61 changes: 31 additions & 30 deletions python/acquire-zarr-py.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,11 @@ class PyZarrStreamSettings

private:
std::string store_path_;
std::optional<std::string> custom_metadata_;
std::optional<PyZarrS3Settings> s3_settings_;
std::optional<PyZarrCompressionSettings> compression_settings_;
std::optional<std::string> custom_metadata_{ std::nullopt };
std::optional<PyZarrS3Settings> s3_settings_{ std::nullopt };
std::optional<PyZarrCompressionSettings> compression_settings_{
std::nullopt
};
bool multiscale_ = false;
ZarrDataType data_type_{ ZarrDataType_uint8 };
ZarrVersion version_{ ZarrVersion_2 };
Expand Down Expand Up @@ -288,11 +290,11 @@ class PyZarrStream
auto store_path = settings.store_path();
stream_settings.store_path = store_path.c_str();

auto metadata = settings.custom_metadata();
stream_settings.custom_metadata =
settings.custom_metadata().has_value()
? settings.custom_metadata()->c_str()
: nullptr;
std::string metadata;
if (settings.custom_metadata()) {
metadata = settings.custom_metadata().value();
stream_settings.custom_metadata = metadata.c_str();
}

if (settings.s3().has_value()) {
s3_settings.endpoint = settings.s3()->endpoint().c_str();
Expand Down Expand Up @@ -340,13 +342,6 @@ class PyZarrStream
}
}

~PyZarrStream()
{
if (is_active()) {
ZarrStream_destroy(stream_.get());
}
}

void append(py::array image_data)
{
if (!is_active()) {
Expand Down Expand Up @@ -534,18 +529,23 @@ PYBIND11_MODULE(acquire_zarr, m)
if (kwargs.contains("store_path"))
settings.set_store_path(kwargs["store_path"].cast<std::string>());

if (kwargs.contains("custom_metadata"))
settings.set_custom_metadata(
kwargs["custom_metadata"].cast<std::optional<std::string>>());
if (kwargs.contains("custom_metadata") &&
!kwargs["custom_metadata"].is_none()) {
auto cm = kwargs["custom_metadata"].cast<std::string>();
settings.set_custom_metadata(cm);
}

if (kwargs.contains("s3"))
settings.set_s3(
kwargs["s3"].cast<std::optional<PyZarrS3Settings>>());
if (kwargs.contains("s3") && !kwargs["s3"].is_none()) {
auto s3 = kwargs["s3"].cast<PyZarrS3Settings>();
settings.set_s3(s3);
}

if (kwargs.contains("compression"))
settings.set_compression(
kwargs["compression"]
.cast<std::optional<PyZarrCompressionSettings>>());
if (kwargs.contains("compression") &&
!kwargs["compression"].is_none()) {
auto compression =
kwargs["compression"].cast<PyZarrCompressionSettings>();
settings.set_compression(compression);
}

if (kwargs.contains("dimensions"))
settings.dimensions =
Expand All @@ -566,10 +566,10 @@ PYBIND11_MODULE(acquire_zarr, m)
.def("__repr__",
[](const PyZarrStreamSettings& self) {
std::string repr =
"StreamSettings(store_path='" + self.store_path();
"StreamSettings(store_path='" + self.store_path() + "'";
if (self.custom_metadata().has_value()) {
repr +=
", custom_metadata='" + self.custom_metadata().value();
repr += ", custom_metadata='" +
self.custom_metadata().value() + "'";
}

if (self.s3().has_value()) {
Expand All @@ -582,9 +582,10 @@ PYBIND11_MODULE(acquire_zarr, m)
for (const auto& dim : self.dimensions) {
repr += dim.repr() + ", ";
}

std::string multiscale = self.multiscale() ? "True" : "False";
repr +=
"], multiscale=" + std::to_string(self.multiscale()) +
", data_type=DataType." +
"], multiscale=" + multiscale + ", data_type=DataType." +
std::string(data_type_to_str(self.data_type())) +
", version=ZarrVersion." +
std::string(self.version() == ZarrVersion_2 ? "V2" : "V3") +
Expand Down
1 change: 0 additions & 1 deletion python/tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import json
from pathlib import Path

import numpy as np
import pytest

import acquire_zarr
Expand Down
Loading
Loading