Repository navigation
Fix license metadata without Hub dependency - #1134
Merged
bruAristimunha merged 3 commits intoAug 26, 2026
Merged
bruAristimunha merged 3 commits into
bruAristimunha merged 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts Braindecode’s model base-class machinery so model license metadata passed as a class keyword (e.g., license="...") is consumed safely even when the optional huggingface_hub dependency is unavailable, and adds a regression test for that scenario.
Changes:
- Update the
_BraindecodeDocstringMetametaclass to accept subclass keyword arguments (**kwargs) during class creation. - Pop
licenseearly inEEGModuleMixin.__init_subclass__so it doesn’t leak into no-Hub paths. - Add a subprocess-based regression test that imports Braindecode with
huggingface_hubblocked and declares a licensed model.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
braindecode/models/base.py |
Accepts/consumes class keywords so license=... won’t break subclass creation when Hub isn’t available. |
test/unit_tests/models/test_huggingface.py |
Adds a subprocess regression test for class keyword license when Hub import is blocked. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
adammounir
added a commit
that referenced
this pull request
Sep 18, 2026
`EEGModuleMixin.__init_subclass__` defaults `license` to `bsd-3-clause` when a model does not declare one, and that value is what a `push_to_hub` writes onto the model card. The upstream BrainBERT repository ships no LICENSE file, so BSD-3 is a claim nobody can back. The class now declares `license="unknown"`, which is what the re-hosted weights at `braindecode/brainbert-pretrained` already say, and a test pins it so the default cannot creep back in. This mechanism only reached master in #1134, after this branch was opened, which is why the omission was invisible until now. Towards #1097
bruAristimunha
added a commit
that referenced
this pull request
Sep 25, 2026
…1104) * Add BrainBERT building blocks (STFT front-end + encoder modules) Standalone braindecode modules ported from the upstream BrainBERT reference: the in-model STFT spectrogram front-end (reproducing the scipy front-end), the input embedding with sinusoidal positional encoding, the spectrogram-prediction head (kept for weight parity) and the classification head. * Add BrainBERT model BrainBERT(EEGModuleMixin, nn.Module): self-supervised foundation model for intracranial (sEEG/iEEG) signals (Wang et al., ICLR 2023). The STFT is computed inside forward so the model keeps the standard (batch, n_chans, n_times) input signature. Modest ready-to-run defaults, with the released large config documented; supports from_pretrained, reset_head and return_features. * Register BrainBERT in the models registry and summary table * Add BrainBERT tests and upstream parity gate Contract tests plus a parity gate: the in-model STFT is checked against the scipy reference, and the input encoding + Transformer are checked bit-exact against the upstream MaskedTFModel when BRAINBERT_SRC is available (skipped otherwise so CI stays green without the external dependency). * Document BrainBERT in the changelog * Skip BrainBERT in torch.jit.script integration test BrainBERT.forward returns a Dict (features) or a Tensor (logits) depending on return_features, and torch.jit.script rejects this polymorphic return type. Add BrainBERT to not_working_models, matching Brant/EEGDINO/MVPFormer which are excluded for the same reason. * Match the upstream configuration behind the published BrainBERT numbers Four changes, all answering the same objection: the port ran a plausible variant of BrainBERT rather than the one the released checkpoint was evaluated with. STFT recipe. Upstream ships two, and they are not equivalent. `preprocessors/stft.py` z-scores first and trims 10 frames per side; `notebooks/demo.ipynb` trims 5 first and then z-scores. It is the former that `conf/preprocessor/stft_pretrained.yaml` reaches through `preprocessors/spec_pretrained.py`, so it is the former that produced the published downstream numbers. The port implemented the notebook one. The default is now the checkpoint's recipe, the other stays reachable through `stft_clip` and `stft_zscore_before_clip`, and the docstring no longer claims an equivalence that does not hold: on filtered noise the two correlate at 0.999 but differ by up to 0.48 z-unit per bin and by 10 frames of sequence length. NaN handling. Upstream zeroes NaNs that survive the statistics and replaces a fully constant window by ones; without either, one bad sample poisons the whole attention window. Both are ported, branch-free so `torch.export` still traces the forward. Pooling. Upstream averages the 10 encoder frames centred on the window of a single electrode (`outputs[:, middle-5:middle+5].mean`); averaging every frame appears in that same file only as a commented-out alternative. The port averaged all frames and all channels. It now pools the centre frames and then the channels, which is the identity at n_chans=1, so a single-channel model reproduces the upstream feature exactly. A window too short for the centre window is refused at construction rather than pooled differently in silence. Probe. `models/linear_wav_baseline.py` is `nn.Linear(input_dim, 1)` and nothing else, so the head's LayerNorm is gone. Also: `freq_cutoff` is renamed `idx_freq_cutoff`, since it is a bin index and not a frequency (40 bins reach about 200 Hz, not 40); the positional encoding refuses a sequence longer than its table instead of truncating it; `normalizing` rejects unknown values; and the summary row moves into alphabetical order with a 1 s window at 2048 Hz, the window the checkpoint expects. Towards #1097 * Declare the BrainBERT licence as unknown instead of inheriting BSD-3 `EEGModuleMixin.__init_subclass__` defaults `license` to `bsd-3-clause` when a model does not declare one, and that value is what a `push_to_hub` writes onto the model card. The upstream BrainBERT repository ships no LICENSE file, so BSD-3 is a claim nobody can back. The class now declares `license="unknown"`, which is what the re-hosted weights at `braindecode/brainbert-pretrained` already say, and a test pins it so the default cannot creep back in. This mechanism only reached master in #1134, after this branch was opened, which is why the omission was invisible until now. Towards #1097 * Test both upstream STFT recipes, the centre-frame pooling and the licence The previous STFT test reproduced the demo-notebook recipe and asserted the port matched it, which is exactly the claim that was wrong: it validated the port against the recipe the released checkpoint does not use. Both recipes are now written out from the upstream sources and checked, and a third test pins that they are *not* interchangeable, so a future simplification cannot quietly collapse them. The tolerance drops from 1e-4 to 5e-6. The residual is the float32 Hann window (scipy builds it in float64) and measures 7.1e-7; at 1e-4 the test could not have distinguished the two recipes it now separates. New coverage: the pooled feature equals upstream's `outputs[:, middle-5:middle+5].mean` on the same encoder output and differs from the all-frames mean; the head carries no LayerNorm; a flat channel stays finite; `normalizing="none"`, `clip=0` and an unknown `normalizing` all behave; the declared licence is `unknown`. Parity gate: `sys.path` is restored whatever happens, and the gate skips rather than passing vacuously if `models` resolves anywhere other than BRAINBERT_SRC. Verified green against a fresh clone of github.com/czlwang/BrainBERT, not only skipped. Towards #1097 * Add a BrainBERT tutorial on intracranial ECoG The three settings that silently change what the pretrained encoder sees are the sampling rate (an STFT bin is a fraction of sfreq, so the same 40 bins reach 205 Hz at 2048 Hz and 100 Hz at 1000 Hz), the normalisation order (upstream ships two recipes) and the window length (the protocol pools 10 centred frames). The example works through all three on BCI Competition IV dataset 4, then reproduces the published downstream protocol: frozen encoder, bare linear probe. It also probes an untrained copy of the same architecture as a control. The untrained encoder reaches 0.580 ROC-AUC against 0.629 for the pretrained one and 0.500 for chance, so most of the headroom above chance comes from the architecture rather than from the pretraining -- reporting the pretrained number alone would have overclaimed. * Accept every torch activation spelling in BrainBERT `activation` was typed `type[nn.Module]` and instantiated with `activation()`. That is braindecode's house spelling (BIOT, LaBraM, CBraMod all use it) and it stays the default, but `activation()` raises a TypeError on the other forms `nn.TransformerEncoderLayer` itself accepts: a string such as "gelu", a ready-made module, or a bare callable like `torch.nn.functional.gelu`. Fold the four forms in `_as_transformer_activation` instead: a class is instantiated, everything else is forwarded untouched, and a class that is not an nn.Module subclass is refused with a clear message rather than failing later inside the encoder layer. * Refuse non-positive pooling and head sizes instead of returning NaN pool_n_frames=0 passed the "enough frames to pool" guard, then forward sliced middle:middle and averaged an empty tensor: the model returned NaN features rather than failing. Negative values built too. Both are refused at construction now. reset_head assigned _n_outputs directly, which accepted 0 and left the init kwargs and the Hub config reporting the previous count, so a model re-serialized after reset_head advertised a head it no longer had. It goes through EEGModuleMixin._set_n_outputs, the helper added in 1.8 for exactly this, which validates and keeps both configs in step. * Keep the activation annotation serializable Widening the annotation to a union starting with str broke the shared model config round-trip: pydantic matched the str branch first, so a serialized "torch.nn.modules.activation.GELU" came back as that string instead of the class, and test_config's json-serialization check failed on BrainBERT alone. The annotation goes back to type[nn.Module], which is what every other braindecode constructor declares and what the config coder knows how to decode. The runtime normalisation stays, so a string, a module instance or a bare callable is still accepted -- it just is not the documented, serializable form. A test pins the annotation, because test_config skips when pydantic is absent and that is how this got through locally. * Pin the re-hosted checkpoint to an immutable commit from_pretrained("braindecode/brainbert-pretrained") resolves to whatever main points at, and a Hub branch is mutable. Upstream distributes the weights from a Google Drive folder that is mutable too, so nothing in the chain was immutable: a later push would silently change what the model loads, and with it any number published against it, without a single line of this repository changing. BRAINBERT_WEIGHTS_REVISION pins the commit, BRAINBERT_WEIGHTS_SHA256 records the digests of both weight files so the bytes can be checked without trusting the Hub to have served the right commit, and every documented call site passes the revision. Two tests guard it: one pins the constants so a change has to be deliberate, the other walks every from_pretrained call in the module and fails on one without a revision -- an unpinned copy-pasteable snippet is how an unpinned load spreads. * fix: load the official BrainBERT checkpoint directly and guard inputs - Add a key mapping from the official checkpoint's input_encoding.* to input_embedding.*. stft_large_pretrained.pth (sha256 d5ba03df) now loads with only final_layer.* missing and the rebuilt pe table unexpected, and every loaded tensor is identical to the official file. - reset_head keeps the old head's device and dtype. - An odd hidden_dim raises a clear ValueError instead of a shape error in the sinusoidal encoding. - forward refuses inputs that give fewer spectrogram frames than the pooled centre frames, instead of silently pooling fewer or returning NaN. - Fix the reset_head config test, which read a non-existent _init_kwargs attribute (_braindecode_init_kwargs). * refactor: simplify the BrainBERT port - final_layer is a bare nn.Linear; the unused pretraining head is dropped (the re-hosted checkpoint loads unchanged, strict=False skips its keys) - the private STFT and input-embedding helpers live below the model class; braindecode/modules/brainbert_modules.py is removed - sfreq is no longer required and the unused normalizing option is gone - forward returns the logits under torch.jit.script, so BrainBERT leaves the TorchScript skip list and scripts directly - the bespoke test file is replaced by one STFT-vs-scipy check in test_models.py; BrainBERT joins the shared return_features suite, which now takes n_times from models_mandatory_parameters - BrainBERT is listed in docs/api.rst; the tutorial is removed Claude-Session: https://claude.ai/code/session_016335Fx1o9ZdJnwKbzsYKgX * refactor: einops layers, figure and docstring for BrainBERT - reshapes and pooling are einops Rearrange/Reduce layers built in __init__, so forward is a sequence of named layers with shape comments - the BRAINBERT_WEIGHTS_* constants are gone; the docstring shows the Hub repo id as a literal, like the other models - the class docstring links the paper's architecture figure and states how to obtain one output per electrode as upstream does - outputs, scripting and export are unchanged Claude-Session: https://claude.ai/code/session_016335Fx1o9ZdJnwKbzsYKgX * fix: validate BrainBERT inputs and settings, mark the Bandit false positive - forward raises on a channel-count mismatch (it silently re-batched) and on non-floating inputs; the constructor names the minimum n_times and rejects invalid STFT settings; the positional table overflow is a ValueError; sfreq != 2048 Hz warns as in BIOT - enable_nested_tensor=False removes the activation warning (no mask is ever passed); `# nosec B105` marks the cls_token=None Bandit false positive as in ZUNA - docstring: which from_pretrained overrides work, the .pth load recipe, NaN and dead-channel behaviour; versionadded 1.9 - tests: test_models.py carries only the BrainBERT block (STFT parity at 2048 and 6000 samples incl. a flat signal, mapping targets); the return_features suite reads n_times via _get_signal_params; BrainBERT is in the Hub integration registry - normalizing stays an instance attribute for pipelines that apply their own statistics; features are unchanged versus the previous head Claude-Session: https://claude.ai/code/session_01G2YTx6U99AkGoHswQ5ZhHX --------- Co-authored-by: Bruno Aristimunha <[email protected]> Co-authored-by: Bru <[email protected]>
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
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.
This extracts the only generic prerequisite from #1132 / #1129 into a two-file change.
Validation: