Fix CedRawIO with sonpy >= 1.9.12 and re-enable the CED tests - #1891
Fix CedRawIO with sonpy >= 1.9.12 and re-enable the CED tests#1891AxelNoun wants to merge 5 commits into
Conversation
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
left a comment
There was a problem hiding this comment.
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.
| try: | ||
| candidates.append(importlib.import_module("sonpy.sonpy")) | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good call, thanks, pushed in a follow-up commit. Two details worth flagging:
import importlibdoesn't pull inimportlib.util, so the import is now explicit.find_spec("sonpy.sonpy")imports the parent and raisesModuleNotFoundErrorifsonpyhas no__path__. Guarding on that first keeps the failure on the explicitImportErrorbelow rather than surfacing fromfind_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>
Thanks, that context helps and it reframes part of my description. I read the 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:
|
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>
Closes #1890.
sonpy 1.9.12 reorganised its package layout and dropped the
libnamespace thatcedrawio.pyrelies on, so everysonpy.lib.*access raisesAttributeError: 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 namespaceA cached
_get_sonpy_namespace()helper probes three candidates in order:sonpy.lib<= 1.9.5, the old per-platform dispatchsonpy>= 1.9.12on Windows and macOS (from .sonpy import *)sonpy.sonpy>= 1.9.12on Linux, whose wheel ships an empty__init__.pyThe third is not redundant: the
cp314-manylinux_2_39_x86_64wheel has a 0-byte__init__.py, so neithersonpy.libnorsonpy.SonFileresolves there. If none of thethree exposes
SonFile, anImportErrornaming all three is raised, rather than letting anAttributeErrorsurface 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.pyreplicated the old per-platform dispatch(
import sonpy.linux.sonpy, …), which no longer exists →HAVE_SONPY = False→ skipped.neo/test/rawiotest/test_cedrawio.pyused a bareimport sonpy, which succeeds with1.9.12 →
HAVE_SONPY = True→ the test would run and fail with theAttributeError.Both now delegate to
_get_sonpy_namespace(), so "sonpy is usable" has one definition.3.
pyproject.toml— thetestextra never installed sonpy"sonpy;python_version<'3.10'",with
requires-python = ">=3.10". The marker cannot be satisfied, so sonpy is absent fromevery 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:
1.9.12 ships
win_amd64wheels for cp39–cp314, butmanylinuxandmacosxonly for cp314.The marker is deliberately not just
"sonpy": on Linux 3.10–3.13 pip would fall back to thesource 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 nowinstall 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 -vFAILED—AttributeError: module 'sonpy' has no attribute 'lib'atcedrawio.py:72PASSED(1.07 s) —test_read_allacross all three spike2 entitiesThat covers both
.smrxand the two.smrentities, so the reader is exercised end to endand not just at import time.
Additionally:
sonpy.libabsent on all five; the compiledlibrary reads
spike2/m365_1sec.smrxcorrectly through the new namespace(
GetOpenError() == 0,MaxChannels() == 101), confirming an import-path problem ratherthan a functional regression in sonpy.
sonpy.sonpy;SonFile,DataType.Adc,DataType.AdcMarkandMarkerFilterare all reachable, andCedRawIO.parse_header()runs to completion instead of raising.HAVE_SONPYisTruewith sonpy 1.9.12 presentand
Falsewhen sonpy cannot be imported.Trueexactly where a usable wheel exists, across the sixplatform/version combinations in Neo's support range.
black --line-length 120 --checkclean on all changed files.pytest neo/test/rawiotest/test_cedrawio.py neo/test/iotest/test_cedio.py -v→ 8 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
platform_system=='Windows' or python_version>='3.14'acceptable, or would you ratherkeep the
testextra minimal and add sonpy only to the CI workflow?cedextra (line 101, currently a bare"sonpy") get the same marker forconsistency? I left it alone to keep the diff focused.