Skip to content

Pin fetchable EEGLAB reference#225

Merged
arnodelorme merged 1 commit into
developfrom
fix/dependency-graph-eeglab-submodule
Jun 17, 2026
Merged

Pin fetchable EEGLAB reference#225
arnodelorme merged 1 commit into
developfrom
fix/dependency-graph-eeglab-submodule

Conversation

@suraj-ranganath

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

Copy link
Copy Markdown
Member

Pin the vendored EEGLAB reference to the latest nearby upstream commit whose nested plugin submodules can be fetched recursively. This fixes GitHub Dependency Graph cloning, which currently fails because upstream EEGLAB develop points plugins/EEG-BIDS at an unavailable commit. The vendored tree remains a development parity oracle and can be advanced again after upstream repairs its submodule graph; follow-up tracked in #227.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @suraj-ranganath's task in 2m 17s —— View job


Review — PR #225: Pin fetchable EEGLAB reference

  • Gather context (diff, submodule config, PR claims)
  • Assess runtime/test impact of the bump
  • Assess how the fix is (and isn't) validated by CI
  • Post review feedback

Summary of the change

A single submodule gitlink bump for the vendored EEGLAB reference:

src/eegprep/eeglab  4207693 → 329056e

.gitmodules still points at https://github.com/sccn/eeglab.git; only the pinned commit changes.

Verdict: 👍 Low-risk and well-scoped. The rationale is sound and the blast radius is contained. One caveat worth calling out below.


What's good

  • Correctly scoped to the parity oracle. Per AGENTS.md, src/eegprep/eeglab/ is a development/parity reference only, and runtime package code must not depend on it — a contract that's actively enforced by tests/test_runtime_eeglab_independence.py (it greps package code for src/eegprep/eeglab, eegprep.eeglab imports, etc.). So moving the pin cannot affect installed/runtime behavior. ✅
  • Minimal diff (+1/-1), matching the repo's "smallest change that solves the request" guidance.
  • The commit message and PR body clearly document why the pin is being moved backward and that it's a temporary measure pending upstream EEGLAB repairing its plugins/EEG-BIDS submodule graph. 👍

Caveat / thing to verify

  • The existing CI does not actually exercise the thing this PR fixes. Both jobs in .github/workflows/test.yml check out with submodules: true (lines 50 and 100), which is non-recursive — by design, with a comment explaining nested EEGLAB plugin submodules are intentionally not fetched. The bug this PR targets is GitHub's Dependency Graph recursive clone failing on the unfetchable plugins/EEG-BIDS pin. That recursive resolution isn't reproduced by CI, so a green CI run on this PR will not confirm the fix. The real validation is the dependency-graph submodule resolver succeeding after merge.

    Recommend confirming out-of-band that 329056e resolves recursively (i.e. its nested plugins/EEG-BIDS and other plugin submodules all point at fetchable commits) before/after merge, since the test matrix won't catch a regression here. I was unable to fetch the upstream commit's nested .gitmodules from this environment (network/gh api calls were not permitted), so I could not independently verify the recursive fetchability claim — please double-check it.

Minor suggestions (non-blocking)

  • Consider opening/linking a tracking issue so the pin gets advanced again once upstream EEGLAB repairs its submodule graph; otherwise the parity oracle silently drifts further behind develop over time.
  • If the parity/matlab test suites read reference files from the vendored tree, a quick local -m parity/-m matlab run against the new pin would confirm no reference files shifted in a way that moves parity tolerances. Since the new commit is "nearby," this is expected to be a no-op, but worth a sanity check.
    · branch fix/dependency-graph-eeglab-submodule

@suraj-ranganath

Copy link
Copy Markdown
Member Author

🤖 Validation note: I reproduced the original failure locally with the old gitlink using a recursive submodule update; it failed on plugins/EEG-BIDS at ea4df87b18634128ba6c315d9d54c85a3dac5dfb. After pinning the vendored EEGLAB reference to 329056e268005df6ea5700a1b6f575e652ad3910, I ran a fresh recursive clone simulation with --recurse-submodules --shallow-submodules and it completed successfully through EEG-BIDS, ICLabel/viewprops/matconvnet, clean_rawdata/manopt, dipfit, firfilt, and tutorial_scripts. I also ran both parity matrix validators and the matrix tests: 24 passed.

@arnodelorme arnodelorme merged commit b39ea53 into develop Jun 17, 2026
10 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.

2 participants