Python tests 2: Streaming boogaloo (#12)

* bindings created * pip install . working * python -m build working (on Windows anyway) * get it building on linux * Remove test stub (save for the next PR) * Undo an overzealous rename * Add Python wheel build job * Prepare for Python bindings * Don't export the enum values into the module base namespace. * (wip) some basic tests * Revert CMake minimum version and use cmake_policy. Using builtin `BUILD_TESTING` cmake option. * Update build.yml * Update release.yml * some simple tests * add python tests to CI * (wip): first stream test (crash!) * Remove deprecated workflows * A rectification of names * whitespace * Fix double free error * Slightly improve debug output. * Add some more streaming tests. * concurrency? * Explicit nullopt * Get it working ok on Windows * remove stub * Use PROJECT_SOURCE_DIR instead of CMAKE_SOURCE_DIR * Respond to PR comments
acquire-project · Oct 29, 2024 · 9c46763 · 9c46763
1 parent b205594
commit 9c46763
Show file tree

Hide file tree

Showing 12 changed files with 506 additions and 113 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -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:
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -11,6 +11,7 @@ env:
 
 jobs:
   windows-and-linux-build:
+    name: Build on ${{ matrix.platform }}
     strategy:
       matrix:
         platform:
@@ -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
@@ -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
@@ -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

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -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 }}
@@ -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
@@ -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
@@ -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:
@@ -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
@@ -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
-
diff --git a/pyproject.toml b/pyproject.toml
@@ -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"

diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt
@@ -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

diff --git a/python/acquire-zarr-py.cpp b/python/acquire-zarr-py.cpp
@@ -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 };
@@ -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();
@@ -340,13 +342,6 @@ class PyZarrStream
         }
     }
 
-    ~PyZarrStream()
-    {
-        if (is_active()) {
-            ZarrStream_destroy(stream_.get());
-        }
-    }
-
     void append(py::array image_data)
     {
         if (!is_active()) {
@@ -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 =
@@ -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()) {
@@ -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") +

diff --git a/python/tests/test_settings.py b/python/tests/test_settings.py
@@ -3,7 +3,6 @@
 import json
 from pathlib import Path
 
-import numpy as np
 import pytest
 
 import acquire_zarr