Skip to content

Fix CedRawIO with sonpy >= 1.9.12 and re-enable the CED tests - #1891

Open
AxelNoun wants to merge 6 commits into
NeuralEnsemble:masterfrom
AxelNoun:fix/cedrawio-sonpy-namespace
Open

Fix CedRawIO with sonpy >= 1.9.12 and re-enable the CED tests#1891
AxelNoun wants to merge 6 commits into
NeuralEnsemble:masterfrom
AxelNoun:fix/cedrawio-sonpy-namespace

Conversation

@AxelNoun

Copy link
Copy Markdown

Closes #1890.

sonpy 1.9.12 reorganised its package layout and dropped the lib namespace that
cedrawio.py relies on, so every sonpy.lib.* access raises
AttributeError: module 'sonpy' has no attribute 'lib'.

While preparing the fix I found that the CED tests could not have caught this, for two
independent reasons — so this PR fixes the reader and restores the test coverage that would
have flagged it.

1. neo/rawio/cedrawio.py — resolve the namespace

A cached _get_sonpy_namespace() helper probes three candidates in order:

candidate covers
sonpy.lib <= 1.9.5, the old per-platform dispatch
sonpy >= 1.9.12 on Windows and macOS (from .sonpy import *)
sonpy.sonpy >= 1.9.12 on Linux, whose wheel ships an empty __init__.py

The third is not redundant: the cp314-manylinux_2_39_x86_64 wheel has a 0-byte
__init__.py, so neither sonpy.lib nor sonpy.SonFile resolves there. If none of the
three exposes SonFile, an ImportError naming all three is raised, rather than letting an
AttributeError surface from the middle of _parse_header.

2. Both CED test guards

There are two, and each fails differently with 1.9.12:

  • neo/test/iotest/test_cedio.py replicated the old per-platform dispatch
    (import sonpy.linux.sonpy, …), which no longer exists → HAVE_SONPY = False → skipped.
  • neo/test/rawiotest/test_cedrawio.py used a bare import sonpy, which succeeds with
    1.9.12 → HAVE_SONPY = True → the test would run and fail with the AttributeError.

Both now delegate to _get_sonpy_namespace(), so "sonpy is usable" has one definition.

3. pyproject.toml — the test extra never installed sonpy

"sonpy;python_version<'3.10'",

with requires-python = ">=3.10". The marker cannot be satisfied, so sonpy is absent from
every CI run and both guards were moot regardless of how they were written. This is why the
breakage went unnoticed.

Changed to match where sonpy actually publishes usable wheels:

"sonpy; platform_system=='Windows' or python_version>='3.14'",

1.9.12 ships win_amd64 wheels for cp39–cp314, but manylinux and macosx only for cp314.
The marker is deliberately not just "sonpy": on Linux 3.10–3.13 pip would fall back to the
source distribution, which ships a Windows .pyd (verified: PE32+ executable (DLL) […] for MS Windows) and produces an unusable install.

The practical effect is that the automatic ubuntu-latest / Python 3.14 CI job will now
install sonpy and actually exercise CedRawIO.

Verification

The existing test suite goes from failing to passing. Windows, Python 3.12, sonpy 1.9.12,
against master (35cbce7):

pytest neo/test/rawiotest/test_cedrawio.py -v
unpatched FAILEDAttributeError: module 'sonpy' has no attribute 'lib' at cedrawio.py:72
patched PASSED (1.07 s) — test_read_all across all three spike2 entities

That covers both .smrx and the two .smr entities, so the reader is exercised end to end
and not just at import time.

Additionally:

  • Windows, Python 3.10–3.14, sonpy 1.9.12: sonpy.lib absent on all five; the compiled
    library reads spike2/m365_1sec.smrx correctly through the new namespace
    (GetOpenError() == 0, MaxChannels() == 101), confirming an import-path problem rather
    than a functional regression in sonpy.
  • Linux x86_64, Python 3.14, sonpy 1.9.12: the helper resolves to sonpy.sonpy; SonFile,
    DataType.Adc, DataType.AdcMark and MarkerFilter are all reachable, and
    CedRawIO.parse_header() runs to completion instead of raising.
  • Both guards checked in both directions: HAVE_SONPY is True with sonpy 1.9.12 present
    and False when sonpy cannot be imported.
  • The proposed marker evaluates True exactly where a usable wheel exists, across the six
    platform/version combinations in Neo's support range.
  • black --line-length 120 --check clean on all changed files.
  • After the three commits: pytest neo/test/rawiotest/test_cedrawio.py neo/test/iotest/test_cedio.py -v8 passed.

Not covered

macOS is untested. It is the one platform where the 1.9.12 wheel exists only for cp314, so
3.10–3.13 are excluded by the marker there. Happy to run it if someone has a machine.

Open questions

  1. Is platform_system=='Windows' or python_version>='3.14' acceptable, or would you rather
    keep the test extra minimal and add sonpy only to the CI workflow?
  2. Should the ced extra (line 101, currently a bare "sonpy") get the same marker for
    consistency? I left it alone to keep the diff focused.

AxelNoun and others added 3 commits July 30, 2026 18:52
sonpy 1.9.12 dropped the 'lib' namespace that cedrawio.py used, so every
sonpy.lib.* access raised AttributeError. Resolve the namespace once,
probing sonpy.lib, sonpy and sonpy.sonpy in turn: the last is needed on
Linux, where the 1.9.12 wheel ships an empty __init__.py.

Refs NeuralEnsemble#1890

Co-authored-by: Cursor <cursoragent@cursor.com>
test_cedio.py replicated the old per-platform sonpy dispatch and skipped
with 1.9.12; test_cedrawio.py guarded on a bare 'import sonpy', which
succeeds with 1.9.12 so the test would run and fail. Both now delegate to
_get_sonpy_namespace().

Refs NeuralEnsemble#1890

Co-authored-by: Cursor <cursoragent@cursor.com>
The test extra declared sonpy;python_version<'3.10' while the project
requires >=3.10, so sonpy was never installed and the CED tests never ran.
Target the platform/version combinations sonpy actually publishes wheels
for; the sdist ships a Windows .pyd and is not usable elsewhere.

Refs NeuralEnsemble#1890

Co-authored-by: Cursor <cursoragent@cursor.com>

@zm711 zm711 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this overall, though I would prefer a tiny rewrite in the logic of the new function you've created. Your other concerns about the testing were on purpose from our perspective. When we no longer have active contributors (or if one of us doesn't have the familiarity/time) for an io we do a slow deprecation process where we slowly reduce testing (but keep it accessible for people) until it completely dies. Your work appears to revive thisio which means that we are happy to fully test things again within your constraints.

Comment thread neo/rawio/cedrawio.py Outdated
Comment on lines +55 to +58
try:
candidates.append(importlib.import_module("sonpy.sonpy"))
except ImportError:
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this try-except is necessary. you could use importlib.util.find_spec to verify submodules without importing and without the try except (you just have to be careful of the logic to prevent errors before you've got to the deepest nesting. Then you can just do the import at the end with import_module.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, thanks, pushed in a follow-up commit. Two details worth flagging:

  • import importlib doesn't pull in importlib.util, so the import is now explicit.
  • find_spec("sonpy.sonpy") imports the parent and raises ModuleNotFoundError if sonpy has no __path__. Guarding on that first keeps the failure on the explicit ImportError below rather than surfacing from find_spec.

Reordering also means the submodule is only imported when the first two candidates come up empty, i.e. the Linux >= 1.9.12 case, instead of eagerly on every platform. Verified against four fake sonpy layouts: right namespace on the three real ones, explicit ImportError on a package exposing nothing.

Per review: find_spec verifies sonpy.sonpy without importing it, so the
try/except goes away. Two guards are needed for that to hold: importlib.util
must be imported explicitly, and find_spec raises ModuleNotFoundError if
sonpy is not a package, so check __path__ first. Probing after the first two
candidates also means the submodule is only imported on Linux >= 1.9.12.

Refs NeuralEnsemble#1890

Co-authored-by: Cursor <cursoragent@cursor.com>
@AxelNoun

AxelNoun commented Aug 3, 2026

Copy link
Copy Markdown
Author

I like this overall, though I would prefer a tiny rewrite in the logic of the new function you've created. Your other concerns about the testing were on purpose from our perspective. When we no longer have active contributors (or if one of us doesn't have the familiarity/time) for an io we do a slow deprecation process where we slowly reduce testing (but keep it accessible for people) until it completely dies. Your work appears to revive thisio which means that we are happy to fully test things again within your constraints.

Thanks, that context helps and it reframes part of my description. I read the test extra marker and the two divergent guards as accidents; if the reduced testing was deliberate, then "this is why the breakage went unnoticed" is the wrong framing on my side. Happy to reword that section so it doesn't read as an oversight report.

On the revival: I'm happy to take CedRawIO on going forward. Ping me on anything CED and I'll pick it up.

Taking "within your constraints" as a green light on the two open questions:

  1. I'll keep sonpy; platform_system=='Windows' or python_version>='3.14' in the test extra. The practical effect is that the existing ubuntu-latest / 3.14 job installs sonpy and exercises the reader end to end, so coverage comes back without a new matrix entry.

  2. The ced extra is worth the same marker, I think. Today pip install neo[ced] on Linux 3.10 to 3.13 falls back to the sdist and installs a Windows .pyd, so users get an install that looks fine and fails later. With the marker they get nothing installed plus the explicit ImportError from _get_sonpy_namespace() at first use, which at least names the constraint. Say the word and I'll add it here, otherwise I'll keep this diff focused and open a separate PR.

sonpy only ships wheels for Windows, and for Linux and macOS from 3.14 on,
so neo[ced] silently resolves to nothing elsewhere. Say so, and point users
at Spike2RawIO for .smr files, which needs no sonpy.

Refs NeuralEnsemble#1890

Co-authored-by: Cursor <cursoragent@cursor.com>
@zm711

zm711 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I've approved our work flows so let's make sure everything passes but this looks good to me. I'll do a final read through once tests pass.

@zm711

zm711 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Taking "within your constraints" as a green light on the two open questions:

I'll keep sonpy; platform_system=='Windows' or python_version>='3.14' in the test extra. The practical effect is that the existing ubuntu-latest / 3.14 job installs sonpy and exercises the reader end to end, so coverage comes back without a new matrix entry.

The ced extra is worth the same marker, I think. Today pip install neo[ced] on Linux 3.10 to 3.13 falls back to the sdist and installs a Windows .pyd, so users get an install that looks fine and fails later. With the marker they get nothing installed plus the explicit ImportError from _get_sonpy_namespace() at first use, which at least names the constraint. Say the word and I'll add it here, otherwise I'll keep this diff focused and open a separate PR.

I would say separate PR. A lot of of the team is away on vacation this month and I'm only intermittently able to respond. So multiple small PRs will be easier on us for now. Thanks for updating this reader.

find_spec() cannot see sonpy.lib in <=1.9.5: __init__.py binds the
extension module via 'import sonpy.<platform>.sonpy as lib', so there is
no sonpy/lib.py for find_spec to find. Use getattr for that candidate and
keep find_spec for the top-level guard and for the nested sonpy.sonpy of
the 1.9.12 Linux wheel.

Co-authored-by: Cursor <cursoragent@cursor.com>
@AxelNoun

AxelNoun commented Aug 6, 2026

Copy link
Copy Markdown
Author

Applied, with one adjustment I want to flag rather than slip in.

find_spec works for two of the three candidates but not for sonpy.lib. In
1.9.5 there is no sonpy/lib.py - __init__.py does platform dispatch and binds
the extension module as an attribute (import sonpy.linux.sonpy as lib), so
find_spec("sonpy.lib") returns None, the candidate is skipped, and since
SonFile isn't on the top-level namespace in 1.9.5 either, resolution falls
through to ImportError. I caught this building fixtures from the published
wheels rather than from assumption - my first synthetic fixture invented a
sonpy/lib.py and passed happily.

So: find_spec for the top-level guard and for sonpy.sonpy, getattr for
lib, where find_spec is structurally blind.

Layouts, from unpacking the wheels:

Distribution __init__.py SonFile
1.9.5 py3-none-any 331 B, platform dispatch sonpy.lib (attribute)
1.9.12 manylinux cp314 0 B sonpy.sonpy
1.9.12 macosx cp314 21 B, from .sonpy import * sonpy
1.9.12 win_amd64 cp39-314 103 B, from .sonpy import * sonpy

One thing worth deciding rather than me deciding it: the sonpy.lib branch is
dead code for us.
Every pre-1.9.12 release pins Requires-Python to a single
minor (1.9.5 is >=3.9, <3.10), so on anything from 3.10 up the only installable
sonpy is 1.9.12. I've kept the branch as defensive handling - it costs one
getattr and protects against sonpy repackaging again - but I'm equally happy to
drop it and simplify to the two live layouts. Your call.

Same metadata also explains the extras marker, though not the way I first
assumed. Below 3.14 on Linux/macOS there's no 1.9.12 wheel and no older
installable version, so pip falls to the sdist - and the sdist succeeds. It
declares no extension to compile, ships sonpy/sonpy.pyd (a Windows binary) as
its only package data, and pip happily builds
sonpy-1.9.12-cp312-cp312-linux_x86_64.whl from it. The install exits 0;
import sonpy then raises ModuleNotFoundError: No module named 'sonpy.sonpy'
from the from .sonpy import * in __init__.py. Verified on Linux / Python
3.12. So the python_version >= '3.14' gate isn't guarding against a build
failure, it's guarding against a silently broken install.

Worth noting what that does to the resolver: find_spec("sonpy") returns a valid
spec there, and import_module("sonpy") raises. It surfaces as an ImportError so
callers still behave, but the message points at sonpy.sonpy when the failure is
really in __init__.py. No amount of find_spec can catch that - the package
exists, it's just unusable. Happy to wrap the top-level import for a clearer
message if you think it's worth it, or leave it.

Local verification (accurate scope):

  • layout matrix: 5/5 PASS against fixtures built from the real wheels (resolver
    extracted from neo/rawio/cedrawio.py)
  • real install: Windows 11 / Python 3.14.2 / sonpy 1.9.12 -> resolves to sonpy
  • CED pytest suite: not run (no working git-annex locally)
  • neither the Linux nested path nor the 1.9.5 attribute path was exercised
    against a real install here - fixtures only; and 1.9.5 cannot install on 3.10+

On the red IO job - it isn't this PR. ubuntu-latest / 3.14 / 2.3.3 fails with
95 errors spread across alphaomega, axon, blackrock, edf, spikeglx, tdt and ced
alike, all the same IncompleteResultsError behind a 403 on
gin.g-node.org/NeuralEnsemble/ephy_testing_data. sonpy 1.9.12 installs fine and
there is no sonpy-related ImportError, AttributeError or ModuleNotFoundError
anywhere in the log. datalad 1.6.0 marked their own test_gin_cloning as xfail on
TimeoutError and IncompleteResultsError - the same exception we're seeing
here - citing HTTP 403 from GitHub-hosted IPs, TCP timeouts and slow responses
(PR #7876).

Worth flagging separately: the dataset-hash step stays green through this.
echo "dataset_hash=$(git ls-remote ...)" >> $GITHUB_OUTPUT takes its exit status
from echo, so the 403 yields an empty hash and the cache key silently degrades
to Linux-datasets-. Happy to open a separate issue for that.

That said, it does mean the CED tests haven't yet gone green in CI - which is the
one claim in this PR I can't currently back with a check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CedRawIO is broken with sonpy >= 1.9.12: the lib namespace no longer exists

2 participants