feat(extension_types): add LogicalSIRecording for SpikeInterface BaseRecording support (ITL-459) by kurodo3[bot] · Pull Request #203 · nauticalab/orcapod-python

kurodo3 · 2026-07-01T23:48:23Z

Summary

Adds LogicalSIRecording — a BaseLogicalType that stores spikeinterface.core.BaseRecording as Arrow large_string using SI's own to_dict() JSON dump (via SIJsonEncoder)
Adds SIRecordingHandler — hashes the same JSON bytes via SHA-256 for content identity, keeping hash/storage representation consistent
Adds register_spikeinterface_types(context=None) — explicit opt-in registration into the default context; not wired into v0.1.json to avoid import failure when SI is absent
Supports file-backed (zarr, binary folder) and ephemeral lazy preprocessing chains; in-memory NumpyRecording raises ValueError with save instructions
New optional extras group: pip install orcapod[spikeinterface]
10 new tests covering: import guard, in-memory error, folder/zarr/ephemeral round-trips, hash stability/change, integration via context

Test plan

uv run --with spikeinterface pytest tests/test_extension_types/test_spikeinterface_types.py -v → 10 passed
uv run pytest tests/ -q → 3918 passed, 57 skipped, 6 xfailed, 0 failed

Fixes ITL-459

🤖 Generated with Claude Code

…message (ITL-459)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…gister_spikeinterface_types (ITL-459)

…oogle-style single-backtick (ITL-459) Convert all double-backtick code references to single-backtick notation to align with Google-style docstring standards. Remove ReST double-colon literal-block syntax from module docstring and simplify to plain prose with inline code formatting. Move ITL-467 phase 2 note from class docstring to inline TODO comment on the relevant line. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…cstring (ITL-459)

…r (ITL-459)

…ral recordings (ITL-459) Also fix python_to_storage to use SIJsonEncoder (required for preprocessing chains that store numpy arrays in to_dict output) and fix storage_to_python to use si_core.load instead of the removed load_extractor API.

… load() API (ITL-459)

…ITL-459)

…ace_types (ITL-459)

…I available (ITL-459) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…459)

…x stale load_extractor docstring (ITL-459)

…(ITL-459)

Copilot

Pull request overview

Adds optional SpikeInterface integration so spikeinterface.core.BaseRecording can be stored as an Arrow extension type (JSON in large_string) and semantically hashed (SHA-256 of the same JSON), with explicit opt-in registration via register_spikeinterface_types().

Changes:

Add LogicalSIRecording, SIRecordingHandler, and register_spikeinterface_types() for SpikeInterface BaseRecording support.
Add a new optional dependency extra: orcapod[spikeinterface] and update lockfile accordingly.
Add tests and update context changelog/spec docs for the new capability.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/orcapod/extension_types/spikeinterface_types.py`	Implements Arrow/Polars logical type, JSON serialization, SHA-256 hashing handler, and explicit registration helper.
`src/orcapod/extension_types/__init__.py`	Conditionally exports SpikeInterface types when SpikeInterface is installed.
`pyproject.toml`	Adds `spikeinterface` optional-dependencies extra and includes it in `all`.
`uv.lock`	Locks SpikeInterface and transitive dependencies; adds `spikeinterface` extra metadata.
`tests/test_extension_types/test_spikeinterface_types.py`	New tests for import guard, in-memory error, round-trips, hash stability/change, and context registration.
`src/orcapod/contexts/data/v0.1.json`	Adds a changelog entry documenting the new SpikeInterface logical type/handler (explicit registration).
`superpowers/specs/2026-07-01-spikeinterface-baserecording-design.md`	Design spec for the feature (needs alignment with implementation details).
`superpowers/plans/2026-07-01-itl-459-spikeinterface-baserecording.md`	Implementation plan documentation for ITL-459.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kurodo3 · 2026-07-02T00:02:52Z

+        if saved_si_types is not None:
+            sys.modules[si_types_key] = saved_si_types
+        else:
+            sys.modules.pop(si_types_key, None)
+            importlib.import_module(si_types_key)
+


Fixed. The block now wraps the re-import in try/except ImportError: pass so cleanup does not raise when SI is genuinely absent from the environment.

kurodo3 · 2026-07-02T00:02:53Z

+# All tests below require spikeinterface — skip if not installed
+si = pytest.importorskip("spikeinterface", reason="spikeinterface not installed")
+import spikeinterface.core as si_core  # noqa: E402
+


Fixed. Removed the module-level pytest.importorskip("spikeinterface") and import spikeinterface.core as si_core. Each SI-dependent test now calls pytest.importorskip as its first statement, so only those tests are individually skipped when SI is absent. test_spikeinterface_not_installed_raises_import_error now correctly runs (and passes) in an environment without SpikeInterface installed. _make_numpy_recording() imports spikeinterface.core lazily inside the function body.

kurodo3 · 2026-07-02T00:02:54Z

+SI imports use `LazyModule` (same pattern as polars/pyarrow in the codebase) so that
+`import orcapod` does not fail when SpikeInterface is not installed.


Fixed. The spec now accurately describes the implementation: spikeinterface_types.py uses a module-level try/except ImportError that re-raises with a pip-install message when SI is absent, and extension_types/__init__.py wraps the import in its own try/except so that import orcapod never fails.

kurodo3 · 2026-07-02T00:02:56Z

+`LogicalSIRecording` is registered in `contexts/data/v0.1.json` alongside
+`LogicalFile`, `LogicalDirectory`, and `LogicalNumpyArray`. The `SIRecordingHandler`
+is registered in the `python_type_handler_registry` in the same context config.


Fixed. The Registration section now correctly describes the explicit opt-in approach: SI types are not wired into contexts/data/v0.1.json (which would break startup without SI installed); instead users call register_spikeinterface_types() once at startup.

kurodo3 · 2026-07-02T00:02:57Z

+- `python_to_storage()` serializes via `recording.to_dict(recursive=True)` → JSON string.
+  Raises a clear `ValueError` for in-memory recordings (`check_serializability("json") == False`).
+- `storage_to_python()` reconstructs via `spikeinterface.core.load_extractor(dict)`.
+- A `SIRecordingHandler` registered in the semantic hasher that hashes the JSON string


Fixed. Updated all three occurrences: Goals & Success Criteria, the storage_to_python component description, and the Error Handling table — all now correctly reference spikeinterface.core.load() rather than the removed load_extractor API.

- Move pytest.importorskip() into each SI-dependent test so that test_spikeinterface_not_installed_raises_import_error runs without SI - Fix finally block cleanup to handle ImportError when SI is genuinely absent - Update spec: LazyModule -> try/except ImportError, v0.1.json -> register_spikeinterface_types(), load_extractor -> spikeinterface.core.load throughout

kurodo3 · 2026-07-02T00:03:14Z

Review round 1 — all comments addressed

Single commit 4a4679e covers all 5 comments:

Test fixes (2 comments)

test_spikeinterface_not_installed_raises_import_error — finally block cleanup
Wrapped the unconditional importlib.import_module(si_types_key) in try/except ImportError: pass. Cleanup no longer raises when SI is genuinely not installed.

Module-level pytest.importorskip — whole-file skip
Removed the module-level si = pytest.importorskip(...) and import spikeinterface.core as si_core. Each of the 9 SI-dependent tests now calls pytest.importorskip as its first line, so they are individually skipped when SI is absent. test_spikeinterface_not_installed_raises_import_error now runs and passes in a bare environment. _make_numpy_recording() imports si_core lazily inside the function body.

Verified: without SI installed, test_spikeinterface_not_installed_raises_import_error → PASSED, all 9 others → SKIPPED.

Spec fixes (3 comments)

LazyModule reference → Updated to describe the actual try/except ImportError approach used in spikeinterface_types.py and the wrapping try/except in extension_types/__init__.py.

v0.1.json registration → Registration section now correctly states that SI types are not wired into v0.1.json (which would break startup without SI) and documents the explicit register_spikeinterface_types() opt-in instead.

load_extractor → load → All three occurrences updated (Goals & Success Criteria, storage_to_python component description, Error Handling table).

… in v0.1.json Add `_optional: true` support to `parse_objectspec` so types backed by optional extras groups (e.g. spikeinterface) can be listed in v0.1.json without breaking startup for users who haven't installed them. When `_optional` is set, ImportError on module import returns None instead of raising; both LogicalTypeRegistry and PythonTypeHandlerRegistry silently skip None entries. Wires LogicalSIRecording and SIRecordingHandler into v0.1.json with _optional: true so they auto-register when spikeinterface is installed. register_spikeinterface_types() is kept for custom contexts and made idempotent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

eywalker · 2026-07-02T00:27:31Z

        self._python_class_factories: dict[type, LogicalTypeFactoryProtocol] = {}
        for lt in (logical_types or []):
-            self.register_logical_type(lt)
+            if lt is not None:


why is this not None check suddenly necessary?

You're right to flag it — the not None check was a code smell. A registry shouldn't need to know about parse_objectspec's sentinel.

Removed. Filtering is now done at the source:

parse_objectspec list processing — when an item in a list is a dict with "_optional": true and its module fails to import, the result is None and is dropped from the list before the caller ever sees it. So LogicalTypeRegistry.__init__ receives a clean list with no None entries, and the loop is back to the plain for lt in (logical_types or []): form.

Handler pairs — for the SpikeInterface handler pair [BaseRecording, SIRecordingHandler], each element carries "_optional": true. When SI is absent, parse_objectspec filters both from the inner list, leaving []. PythonTypeHandlerRegistry.__init__ now guards with entry and len(entry) == 2 and all(x is not None for x in entry) — the entry truthiness check (falsy for []) prevents the vacuous-truth all() problem that would have caused an unpack error.

Tests for both paths are in tests/test_utils/test_object_spec.py (added in commit 434184ac).

…recording-objects

…ltering to parse_objectspec Fix two missing commas left by the merge conflict resolution in v0.1.json (SI handler entry lacked comma before numpy.ndarray; SI changelog entry lacked comma before pandas entry). Also address review feedback on the `not None` check in LogicalTypeRegistry: move the filtering of optional-missing entries to parse_objectspec (where None is produced) so registries receive clean lists. parse_objectspec now drops None results from "_optional": true dict items when processing lists. For handler pairs (inner 2-element lists), optional filtering makes the inner list empty []; PythonTypeHandlerRegistry guards with `entry and len(entry) == 2` to correctly skip both empty and incomplete pairs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

codecov · 2026-07-02T00:47:34Z

Codecov Report

❌ Patch coverage is 92.55319% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rc/orcapod/extension_types/spikeinterface_types.py	92.30%	5 Missing ⚠️
src/orcapod/extension_types/__init__.py	60.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Adds spikeinterface>=0.101 to [dependency-groups] dev so that `uv sync` installs it for all developers and spikeinterface tests run as part of the standard pytest suite rather than being skipped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…hips license outside standard metadata quantities (transitive dep of spikeinterface via neo) has a BSD-3-Clause license but stores it in licenses/doc/user/license.rst rather than a standard License: wheel metadata field, so pip-licenses reports UNKNOWN. Verified from source; exclude it from the allow-only check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

kurodo3 · 2026-07-02T00:58:03Z

Review round 2 — eywalker's comment addressed

Single commit 434184ac covers the change:

Why the `not None` check was removed from `LogicalTypeRegistry`

The check was added as a workaround for the _optional: true mechanism in v0.1.json, but a registry shouldn't need to know about parse_objectspec's sentinel value — that's the wrong layer.

Fix: filter at the source (parse_objectspec), not at the consumer

When processing a list, parse_objectspec now drops any item whose result is None and whose spec had "_optional": true. The registry receives a clean list with no None entries, so the loop stays as plain for lt in (logical_types or []):.

Handler pairs (inner lists)

The SI handler entry is [{"_type": "spikeinterface.core.BaseRecording", "_optional": true}, {"_class": "..SIRecordingHandler", "_config": {}, "_optional": true}]. When SI is absent, both elements are filtered from the inner list, leaving [] as an entry in the outer list. PythonTypeHandlerRegistry.__init__ now guards with entry and len(entry) == 2 and all(x is not None for x in entry) — the truthiness check on entry (falsy for []) prevents a vacuous-truth unpack error.

Tests in tests/test_utils/test_object_spec.py cover both paths.

Other changes in this round (prior commits)

feat(context) d2f05f84: _optional: true support added to parse_objectspec; SI types auto-register in v0.1.json
fix(context) 434184ac: v0.1.json merge-conflict missing commas repaired; not None check removed from registry
chore(dev) 161c1c34: spikeinterface added to dev dependency group so CI runs SI tests
fix(ci) 958dec04: --ignore-packages quantities added to license check (BSD-3-Clause but non-standard license path)

kurodo3 Bot and others added 16 commits July 1, 2026 22:19

docs(specs): add BaseRecording LogicalType design spec (ITL-459)

a6f360c

docs(specs): clarify ephemeral recording support and in-memory error …

a16709d

…message (ITL-459)

docs(plans): add BaseRecording implementation plan (ITL-459)

7aa422e

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

chore(deps): add spikeinterface optional extras group (ITL-459)

e05d3b2

feat(extension_types): add LogicalSIRecording, SIRecordingHandler, re…

bbc92d1

…gister_spikeinterface_types (ITL-459)

refactor(extension_types): fix remaining double-backtick in module do…

9e34ef3

…cstring (ITL-459)

test(spikeinterface_types): in-memory NumpyRecording raises ValueErro…

0b6ba24

…r (ITL-459)

docs(extension_types): update docstrings to reflect SIJsonEncoder and…

eab2a62

… load() API (ITL-459)

test(spikeinterface_types): hash stability and content-change tests (…

fa7154f

…ITL-459)

test(spikeinterface_types): integration test for register_spikeinterf…

b507096

…ace_types (ITL-459)

feat(extension_types): conditionally export LogicalSIRecording when S…

7ffe1dc

…I available (ITL-459) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs(context): record LogicalSIRecording in v0.1.json changelog (ITL-…

fb4e173

…459)

refactor(extension_types): use public register_logical_type() API; fi…

9914b5b

…x stale load_extractor docstring (ITL-459)

chore(deps): update uv.lock for spikeinterface optional dependencies …

efa13c0

…(ITL-459)

eywalker requested a review from Copilot July 1, 2026 23:53

Copilot started reviewing on behalf of eywalker July 1, 2026 23:53 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

eywalker requested changes Jul 2, 2026

View reviewed changes

eywalker and others added 2 commits July 1, 2026 17:40

Merge branch 'main' into eywalker/itl-459-support-spikeinterface-base…

9b1de7e

…recording-objects

kurodo3 Bot and others added 2 commits July 2, 2026 00:47

eywalker approved these changes Jul 2, 2026

View reviewed changes

eywalker merged commit af5d3a5 into main Jul 2, 2026
11 checks passed

eywalker deleted the eywalker/itl-459-support-spikeinterface-baserecording-objects branch July 2, 2026 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(extension_types): add LogicalSIRecording for SpikeInterface BaseRecording support (ITL-459)#203

feat(extension_types): add LogicalSIRecording for SpikeInterface BaseRecording support (ITL-459)#203
eywalker merged 22 commits into
mainfrom
eywalker/itl-459-support-spikeinterface-baserecording-objects

kurodo3 Bot commented Jul 1, 2026 •

edited by eywalker

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

kurodo3 Bot Jul 2, 2026

Uh oh!

kurodo3 Bot Jul 2, 2026

Uh oh!

kurodo3 Bot Jul 2, 2026

Uh oh!

kurodo3 Bot Jul 2, 2026

Uh oh!

kurodo3 Bot Jul 2, 2026

Uh oh!

kurodo3 Bot commented Jul 2, 2026

Uh oh!

eywalker Jul 2, 2026

Uh oh!

kurodo3 Bot Jul 2, 2026

Uh oh!

codecov Bot commented Jul 2, 2026 •

edited

Loading

Uh oh!

kurodo3 Bot commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		SI imports use `LazyModule` (same pattern as polars/pyarrow in the codebase) so that
		`import orcapod` does not fail when SpikeInterface is not installed.

Uh oh!

Conversation

kurodo3 Bot commented Jul 1, 2026 • edited by eywalker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

kurodo3 Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot commented Jul 2, 2026

Review round 1 — all comments addressed

Test fixes (2 comments)

Spec fixes (3 comments)

Uh oh!

eywalker Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kurodo3 Bot commented Jul 2, 2026

Review round 2 — eywalker's comment addressed

Why the not None check was removed from LogicalTypeRegistry

Other changes in this round (prior commits)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kurodo3 Bot commented Jul 1, 2026 •

edited by eywalker

Loading

codecov Bot commented Jul 2, 2026 •

edited

Loading

Why the `not None` check was removed from `LogicalTypeRegistry`