Epic: make EEGPrep browser-ready with Pyodide and ONNX ICLabel - #386
Open
neuromechanist wants to merge 26 commits into
Open
neuromechanist wants to merge 26 commits into
neuromechanist wants to merge 26 commits into
Conversation
scipy 1.15.3's _spropack extension fails to dlopen on Darwin 27
("zero-fill section type, but offset field is not zero"), which breaks
scipy.io and therefore every import that reaches it, including picard
and the ICLabel network loader. 1.16.3 ships fixed wheels and stays
inside the existing <1.17 constraint, so no constraint change is needed.
Tested: scipy.io and picard import cleanly after uv sync --group dev.
oct2py drags in octave-kernel/ipykernel/pyzmq/debugpy/tornado, and psutil has no WebAssembly build; both now live behind new [eeglab] and [sys] extras instead of [project].dependencies, and both are included in [all]. pyedflib has zero imports under src/; nothing but a test fixture writer in tests/test_bids_load_frombids_helpers.py needs it, so it moves to the dev dependency group instead of becoming an unused extra. Not tested beyond `uv lock`/`uv sync` succeeding; behavior verified by later commits.
max_mem=None previously called psutil.virtual_memory() twice (once for the initial default, once as a retry when the block-size check failed), which broke unhelpfully now that psutil is optional. Resolve it once, at the top of asr_process, to a fixed DEFAULT_MAX_MEM_MB=64, matching the maxmem=64 default already used by asr_calibrate() and clean_asr() elsewhere in this plugin. The now-pointless retry (re-fetching the same value again) is removed in favor of failing fast. Updated test_utils_asr.py: dropped the psutil mocks in test_memory_management/test_memory_error_handling since asr_process no longer imports psutil, and added test_default_max_mem_works_without_psutil, which blocks the real psutil import via sys.modules to prove max_mem=None does not need it. Tested with `uv run pytest tests/test_utils_asr.py`.
get_eeglab('OCT') now catches the bare oct2py ImportError and re-raises
naming 'eegprep[eeglab]'. num_jobs_from_reservation's psutil ImportError
previously named a bare 'uv pip install psutil'; it now names
'eegprep[sys]' too. Verified manually with the real import blocked (and,
for the oct2py path, a stub EEGLAB checkout so get_eeglab reaches the
oct2py import at all): both raise the updated message.
Phase gate for #374: walks eegprep's base dependency closure in uv.lock (evaluating dependency markers against a synthetic Pyodide/ Emscripten environment, so e.g. greenlet correctly self-excludes), then checks each package against the official Pyodide package lock and, failing that, for a pure-Python (abi=none, platform=any) wheel on PyPI that micropip could install directly. One pre-existing gap is tracked as non-blocking: docopt (pulled in via pybids -> num2words) has no PyPI wheel at all. Fixing it would mean changing pybids's or num2words's own dependency tree, which is out of scope for this phase (no numerical/behavioral change, and pybids/mne/neo are explicitly not to be touched here); flagged in the script output and in the PR description for a maintainer decision. Tested by running the script directly: 0 failing, 1 known gap, exit 0.
Add install.rst sections for the MATLAB/Octave parity helpers (eeglab extra) and system-aware parallel job sizing (sys extra), and mention them in the all-extras examples in install.rst and README. Docs-only change, not separately tested beyond reading the rendered text.
The iclabel extra provides onnxruntime, the new runtime backend for ICLabel classification. torch now only needs to install onnx alongside it, since it is used to regenerate iclabel.onnx from netICL.mat, not to run classification. Package data ships plugins/**/*.onnx instead of plugins/**/*.mat, and netICL.mat is explicitly excluded from the wheel now that it is a dev-only export source rather than a runtime resource. Not tested standalone; covered by the packaging and test suite runs in later commits.
iclabel.onnx is a ~11 MB protobuf artifact; without this it fails the 5 MB large-file check that .mat files are already exempt from. Tested: ./pre-commit.py passes with iclabel.onnx staged.
tools/iclabel/export_iclabel_onnx.py exports ICLabelNet (built from netICL.mat) to ONNX at a pinned opset (17: supported by onnxruntime since 1.14, and comfortably within the ops the network actually uses - Conv2d/LeakyReLU/Softmax/Concat/Reshape, all available since opset 7-9). The script verifies the export against torch on random inputs before succeeding. iclabel.onnx (11.1 MB) is the regenerated artifact, committed so the package ships it in place of netICL.mat (10.3 MB). iclabel_net_load_py_measures.py now resolves netICL.mat from the source tree instead of importlib.resources, since it is a dev-only export source and no longer a packaged runtime resource. Tested: uv run python tools/iclabel/export_iclabel_onnx.py regenerates iclabel.onnx deterministically and reports torch-vs-onnxruntime max abs diff 1.00e-05 on random inputs (opset export self-check).
iclabel_net_onnx.py loads the packaged iclabel.onnx and runs it through onnxruntime. iclabel.py's engine=None path now calls it instead of building ICLabelNet from netICL.mat with torch; torch no longer appears anywhere in the runtime classification path. Feature extraction, the 4-way augmentation, and the post-network averaging are unchanged. The lite/beta NotImplementedError is unchanged in behavior; only the artifact name in its message was updated. Tested: iclabel(EEG, engine=None) on sample_data with torch fully uninstalled (separate venv, eegprep[iclabel] only) reproduces the same classification as before this change; see the ONNX-vs-torch test added in a later commit for the numeric comparison.
test_iclabel.py: TestICLabelEngines.test_basic now requires onnxruntime (what engine=None actually needs) instead of torch. Added TestICLabelOnnxExport, which cross-checks the packaged iclabel.onnx against the torch network it was exported from, on sample_data. Measured max abs diff 1.43e-06 / max rel diff 2.59e-05, the same order of magnitude as the existing Python-vs-MATLAB parity gap this file already tolerates at rtol=1e-4/atol=1e-5 (float32 op-ordering noise between eager torch and onnxruntime's fused CPU kernels), so the test reuses that established tolerance rather than introducing a new one. conftest.py: narrowed the matlab-file classification for test_iclabel.py from the whole file to TestICLabelEngines:: only, since the new TestICLabelOnnxExport class does not need MATLAB. test_public_api_examples.py: packaging assertions and comments now expect plugins/ICLabel/iclabel.onnx and the iclabel extra instead of netICL.mat and the torch extra. Tested: EEGPREP_SKIP_MATLAB=1 uv run pytest tests/test_iclabel.py tests/test_public_api_examples.py -v (18 passed, 1 skipped for missing MATLAB).
Updates every user-facing and developer-facing mention of netICL.mat and the torch requirement for ICLabel classification: installation guide (new ICLabel extra section), ICA/component and plugin user guide pages, the pop_iclabel help resource, the public-API example comment, the changelog, the final parity matrix rationale, and AGENTS.md's repo map. No behavior change; docs and comments only.
The release verification gate asserted netICL.mat shipped in the wheel; it now asserts iclabel.onnx ships and netICL.mat does not. Tested: uv build locally and confirmed the wheel contains plugins/ICLabel/iclabel.onnx and not netICL.mat (the same check this workflow step performs).
The extras split changes what a plain install provides and the ASR default changes numerical output for direct asr_process(max_mem=None) callers, so both need a user-facing note.
Phase 1: Move oct2py, psutil, and pyedflib out of the base install
Resolves three conflicts from the phase 1 extras split: - changelog.rst and installation.rst: both phases appended to the same anchors, so both entries are kept. The extras example now names the iclabel extra rather than torch, since torch became export-only here. - uv.lock: regenerated with uv lock against the merged pyproject.toml rather than hand-merged. pyproject.toml auto-merged; the all extra is a superset of torch, iclabel, gui, console, docs, eeglab, and sys.
Export ICLabel to ONNX and add the onnxruntime backend
* Add Pyodide harness and ICA benchmark gate * Pin Node.js in Pyodide CI job * Report native ICA iteration counts * Publish native and Pyodide matmul timings * Clarify Picard benchmark parameters * Keep Node-specific test in Pyodide CI
* Add async ICLabel browser execution * Fix async Pyodide parity harness * Await Pyodide parity scripts explicitly * Fix async GUI menu scheduling * Use the active async event loop
* Add ICLabel quantization tests * Freeze ICLabel evaluation provenance * Add ICLabel int8 evaluation tooling * Record ICLabel evaluation generation contract * Record ICLabel artifact checksums * Add OpenNeuro fallback for ICLabel data * Retry ICLabel evaluation downloads * Correct sub-015 evaluation checksum * Freeze subject-disjoint ICLabel features * Quantize ICLabel with frozen parity gate * Make ICLabel gate test platform stable * Harden ICLabel parity provenance * Explain float32 parity boundary
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
An organization admin can add extra usage credits at claude.ai/admin-settings/usage.
Once credits are available, reopen this pull request to trigger a review.
Member
Author
|
🤖 Full project:pr-review-toolkit integration review completed for the epic branch across code, tests, errors, comments/docs, types, and simplification. Findings
Validation
Residual risk
@arnodelorme This epic PR is ready for maintainer review and merge. I have not merged it into |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Member
Author
|
🤖 Correction after the requested independent review panel: the earlier inline “no actionable findings” assessment is superseded. Five fresh-context reviewers covering code, tests, errors, docs/types, and simplification found actionable blockers. Findings
Verification
The epic PR is not merge-ready and remains intentionally unmerged to develop/main. The Codex review dispatch gap is tracked separately in research-skills #97: neuromechanist/research-skills#97. @arnodelorme |
* Fix epic review blockers * Harden async dataset freshness * Guard all async console entry points * Address final epic review findings * Harden async freshness and release gates * Harden async freshness and release validation * Harden async freshness and mutable data tracking
The epic branch was 45 commits behind develop and building on it meant building on a stale base. Seven files conflicted. pyproject.toml: took develop's Pyodide-bounded floors, but kept this branch's extras split. Develop still carries oct2py in the base install and taking its side wholesale would have undone phase/374. docs.yml and release.yml: kept this branch's ICLabel jobs, took develop's 3.12 pins. Four more 3.11 pins came in from this side without conflicting, in the sdist smoke test and the Pyodide harness job, and are bumped too. storage.py: both sides added to the same class. This branch's mutation tracking and develop's _BackingFile, _validate_backing_file and empty() are all kept. changelog.rst and ica_and_components.rst: both sides appended; kept both, and develop's newer API entries. uv.lock: regenerated rather than hand-merged. Two bugs the merge exposed, neither findable on this branch alone: MemmapData.__array_function__ returned NotImplemented for everything except copyto and put. Defining that method opts the type into numpy's dispatch protocol, where NotImplemented raises rather than falling back, so every other numpy function failed on a MemmapData. np.size(data) in pop_reref is one caller of many. Develop's #351 test covers it and this branch never had that test. It now delegates to the mapped array, unwrapping nested handles so np.concatenate([a, b]) does not dispatch straight back into itself. The tomli backport in check_pyodide_base_resolution.py is dead on a 3.12 floor and ty flagged its ignore directive. Tested: 3196 passed, 277 skipped, 0 failed with MATLAB skipped. Ruff check and format clean, ty clean with no diagnostics, pre-commit OK over 1221 files.
Merge develop into the browser epic
Picks up #402, which landed on develop after the previous forward merge branched. Its content is already here: the same correction was applied while resolving that merge, so this records the ancestry rather than changing any file, and keeps the next develop merge from re-presenting it.
Merge develop into the browser epic (ancestry only)
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final integration PR for #324. The epic branch consolidates the six phase PRs:
runicahot products through SciPydgemmunder Emscripten.onnxruntimebackend.Integration review
🤖 Full
project:pr-review-toolkitreview completed across code, tests, errors, comments/docs, types, and simplification. No actionable findings.Validation
develop../pre-commit.py --changed-from origin/developpasses.ruff check .andruff format --check .pass.Explicit browser boundary
Pyodide ICA is single-threaded. Web Workers are the UI/independent-job boundary, not an intra-ICA MPI implementation. Native runica retains NumPy matmul; Emscripten uses float64
dgemm. The universal all-dtypes BLAS gate remains false by design.This PR is intentionally left open for maintainer review and merge. It has not been merged into
developormainby this action.