Skip to content

Add agent-friendly EEGPrep CLI - #176

Merged
arnodelorme merged 85 commits into
developfrom
feature/eegprep-agent-cli
Jun 11, 2026
Merged

Add agent-friendly EEGPrep CLI#176
arnodelorme merged 85 commits into
developfrom
feature/eegprep-agent-cli

Conversation

@suraj-ranganath

@suraj-ranganath suraj-ranganath commented Jun 8, 2026

Copy link
Copy Markdown
Member

Adds the headless eegprep CLI for agent and batch workflows with JSON discovery, schemas, bundled skill guide, inspect/validate, transforms, pipelines, batch, QC/report, BIDS, and EEGLAB migration helpers.

Stacking note: this PR is intentionally pointed at develop, but it is stacked on #174 (feature/eeglab-full-standalone-completion) and should not be merged before #174 lands. Reviewers should expect the diff to include parent-stack changes until #174 is merged or this branch is rebased.

Fixes #175

Complete clean_rawdata and FIRFilt parity
…tics

[ICLabel] Complete viewprops diagnostics parity
…ompletion' into phase/160-dipfit-source-parity

# Conflicts:
#	.notes/implementation-notes.html
Add standalone DIPFIT source-localization parity
…ompletion' into phase/161-study-pac-limo

# Conflicts:
#	.notes/implementation-notes.html
Implement STUDY PAC and LIMO design workflows
…ompletion' into phase/162-large-dataset-storage

# Conflicts:
#	.notes/implementation-notes.html
#	src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py
[Storage] Implement large-dataset storedisk semantics
Build standalone EEGPrep Sphinx manual and tutorials
…-hardening

[Parity] Close final standalone evidence
@claude

claude Bot commented Jun 8, 2026

Copy link
Copy Markdown

Claude finished @suraj-ranganath's task in 7m 32s —— View job


Code review

  • Overall assessment: Needs changes (a few important consistency/parity fixes; no merge-blocking crashes found)
  • Highest-risk area: Pipeline clean step and CLI/pipeline transform parity
  • Merge recommendation: Needs changes

Reviewed all 12 new CLI modules and the 4 new test files. Cross-checked every referenced pop_*/plugin signature and return shape (pop_eegfilt, pop_eegfiltnew, pop_resample, pop_reref, pop_epoch, pop_runica, pop_clean_rawdata, clean_artifacts, pop_importbids/pop_exportbids). Indexing conversions (1-based↔0-based) are correct, return_com unpacking matches each callee, YAML loading is safe_load, no eval/exec/pickle/shell-out, and HTML report output is escaped. No syntax/import/reference errors found.

Blocking

None.

Important

  1. Pipeline clean bypasses the pop_clean_rawdata epoched-data guardsrc/eegprep/cli/commands/pipeline.py:561 (_apply_clean). Calls clean_artifacts(...) directly instead of pop_clean_rawdata(...). Since pipelines don't order epoch vs clean, an epoch→clean config runs ASR on (nbchan, pnts, trials) data and produces garbage rather than the EEGLAB error "Input data must be continuous. This data seems epoched." (plugins/clean_rawdata/pop_clean_rawdata.py:70). The hand-built EEG = clean_artifacts(EEG, ...) history is also not EEGLAB-style. Fix: route through pop_clean_rawdata(EEG, gui=False, return_com=True, **clean_kwargs) like transforms._clean does. (See inline comment.) Fix this →

  2. transforms ica --deterministic can't be disabledsrc/eegprep/cli/commands/transforms.py:518. action="store_true", default=True with no --no-deterministic, so it's always True and for runica without --seed it always forces rndreset="off", making EEGLAB's default non-deterministic behavior unreachable. Mirror the --reorder/--no-reorder pattern two lines below. (See inline comment.)

  3. filter uses a different backend in transforms vs pipelinetransforms.py:244,266 use pop_eegfilt while pipeline.py:513,525 use pop_eegfiltnew. Same user-facing "filter" concept, different algorithms/defaults, so eegprep filter and a pipeline filter step give different results. For an agent-facing CLI this surprise is worth eliminating; pick one backend (likely pop_eegfiltnew) for both, or document the difference explicitly.

Nits

  • Dead command registration on arrivalpipeline.py:163 register()/:188 handle_registered() and qc.py register()/handle_registered() are never used. main.py wires these commands through its own _register_pipeline/_register_qc (main.py:185,215) dispatching on args.handler, while these register() functions set func= (a key the dispatcher never reads). AGENTS.md: "Remove … functions … that your change made unused." Delete them, or route main.py through them. (report.py correctly sets both and is wired in, so it's fine.)
  • Parallel re-implementations of core.py helperstransforms.py reimplements _sha256/_json_default/_utc_now/_manifest_payload/_write_json/_error_payload (and a CliTransformError subclass), and qc.py:415 reimplements _json_safe, all of which exist in core.py. AGENTS.md flags "Parallel implementations of existing helpers." Notably transforms.py's manifest doesn't go through core.build_manifest, so manifest shape can drift from pipeline/qc/report. Prefer importing the core helpers.
  • pipeline.py:119 # noqa: BLE001 is inert — ruff only selects E4/E7/E9/F (pyproject.toml), so BLE001 isn't enabled.

Test gaps

  • Pipeline non-destructive guard untestedpipeline.py:448 _preflight_writes/OUTPUT_EXISTS is a core safety contract but no test runs a pipeline twice (or against a pre-existing output) to assert it returns OUTPUT_EXISTS and leaves the existing file untouched. Same for bids export into a non-empty root (bids.py:205).
  • rereference --method channels 1-based→0-based conversion untested — this is exactly the MATLAB/Python index boundary AGENTS.md calls out. Add a case asserting --channels 1 2 reaches pop_reref as [0, 1] (via manifest/history) and that omitting --channels errors with CONFIG_SCHEMA_ERROR.
  • ica determinism toggle — once Refactor #2 is fixed, add a test that --no-deterministic does not set rndreset="off".

EEGLAB parity notes

@suraj-ranganath

Copy link
Copy Markdown
Member Author

🤖 Specification for issue #175

Problem: EEGPrep had GUI and interactive console entry points, but no stable headless CLI contract for agents or batch workflows. Agents needed machine-readable discovery, JSON results, stable error codes, safe output handling, manifests, and a version-matched usage guide instead of scraping Python APIs or GUI behavior.

Approach: Add the installed eegprep command in src/eegprep/cli/ while leaving eegprep-gui and eegprep-console unchanged. The CLI is organized around researcher workflows: capabilities/schema/examples/skills, inspect, validate, resample, rereference, filter, clean, epoch, ica, pipeline, batch, qc, report, bids, and eeglab migration helpers. Shared CLI primitives live in core.py, dataset inspection in dataset.py, and report rendering in reporting.py; command modules call existing EEGPrep pop_* and plugin functions rather than duplicating preprocessing math.

Key code: main.py builds the top-level command grammar and agent help section. discovery.py exposes capabilities, command schemas, examples, and bundled skills. transforms.py handles safe dataset-writing transforms with manifests and stdout/stderr separation. pipeline.py validates, plans, and runs YAML pipelines. batch.py applies pipelines across explicit inputs or BIDS-discovered subjects. qc.py and report.py provide machine-readable QC plus HTML reports. bids.py and eeglab.py cover BIDS import/export/validate and EEGLAB history/compare/convert helpers.

Tests: Added CLI subprocess and unit tests for discovery completeness, bundled skill resources, JSON parse errors, inspect/validate, transform output safety and manifests, pipeline validate/plan/run, batch relative output handling, QC 1-based public indices, report writing, BIDS read-only validation and manifest overwrite safety, EEGLAB history mapping, nonfinite-aware dataset compare, and public entry point/package-data coverage. Verification run locally: uv run --no-sync ruff check ., uv run --no-sync ruff format --check ., uv run --no-sync ty check, ./pre-commit.py --changed-from origin/feature/eeglab-full-standalone-completion, focused CLI pytest, and EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest -m "not slow" --tb=short with 1912 passed, 209 skipped, 12 deselected.

Stacking note: This PR targets feature/eeglab-full-standalone-completion and should wait for that parent branch before merge to develop.

Comment thread src/eegprep/cli/commands/pipeline.py Outdated
Comment on lines +561 to +566
EEG, *_ = clean_artifacts(EEG, **clean_kwargs)
history = "EEG = clean_artifacts(EEG"
if clean_kwargs:
history += ", " + ", ".join(f"{key}={value!r}" for key, value in clean_kwargs.items())
history += ");"
return EEG, history

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important (EEGLAB parity / correctness): _apply_clean calls clean_artifacts(...) directly instead of pop_clean_rawdata(...). The transforms path (transforms.py:299) correctly routes through pop_clean_rawdata, which guards against epoched input:

if int(EEG.get("trials", 1) or 1) > 1 or np.asarray(EEG.get("data")).ndim == 3:
    raise ValueError("Input data must be continuous. This data seems epoched.")

(plugins/clean_rawdata/pop_clean_rawdata.py:70-71)

Pipelines impose no ordering between epoch and clean, so a config that epochs then cleans will run ASR on (nbchan, pnts, trials) data and produce garbage instead of the clear EEGLAB-style error. The hand-built history EEG = clean_artifacts(EEG, ...) is also not EEGLAB-style; everything else uses pop_* return_com history.

Suggested fix (drop the manual history string too):

Suggested change
EEG, *_ = clean_artifacts(EEG, **clean_kwargs)
history = "EEG = clean_artifacts(EEG"
if clean_kwargs:
history += ", " + ", ".join(f"{key}={value!r}" for key, value in clean_kwargs.items())
history += ");"
return EEG, history
EEG, history = pop_clean_rawdata(EEG, gui=False, return_com=True, **clean_kwargs)
return EEG, history

This restores the continuous-data guard and the EEGLAB pop_clean_rawdata(...) history. Requires importing pop_clean_rawdata instead of clean_artifacts.

Comment thread src/eegprep/cli/commands/transforms.py Outdated
_add_common_arguments(parser, include_common_flags=include_common_flags)
parser.add_argument("--method", choices=("runica", "picard", "amica", "runamica15"), default="runica")
parser.add_argument("--seed", type=int, help="Random seed for backends that support one.")
parser.add_argument("--deterministic", action="store_true", default=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important: --deterministic is declared action="store_true", default=True, so args.deterministic is always True and there is no way to turn it off. For runica without --seed this unconditionally forces options["rndreset"] = "off" (_ica_options, line 411-412), making EEGLAB's default non-deterministic random-reset behavior unreachable from the CLI. Compare --reorder/--no-reorder two lines below, which correctly provide the paired flag.

Suggested change
parser.add_argument("--deterministic", action="store_true", default=True)
parser.add_argument("--deterministic", dest="deterministic", action="store_true", default=True)
parser.add_argument("--no-deterministic", dest="deterministic", action="store_false")

@suraj-ranganath
suraj-ranganath changed the base branch from feature/eeglab-full-standalone-completion to develop June 8, 2026 02:09
@arnodelorme
arnodelorme merged commit 09d5e5b into develop Jun 11, 2026
9 checks passed
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.

Epic: Add an agent-friendly EEGPrep CLI

2 participants