Skip to content

Fix license metadata without Hub dependency - #1134

Merged
bruAristimunha merged 3 commits into
braindecode:masterfrom
bruAristimunha:codex/pr-1132-license-prereq-20260825
Aug 26, 2026
Merged

bruAristimunha merged 3 commits into
braindecode:masterfrom
bruAristimunha:codex/pr-1132-license-prereq-20260825

Conversation

@bruAristimunha

Copy link
Copy Markdown
Collaborator

This extracts the only generic prerequisite from #1132 / #1129 into a two-file change.

  • Consume the model license keyword before the no-Hub fallback metaclass receives subclass keywords.
  • Add a subprocess regression test that imports Braindecode with huggingface_hub unavailable and declares a licensed model.
  • Keep all pose models, registrations, NOTICE changes, and noncommercial code out of scope.

Validation:

  • focused regression: 1 passed
  • model base and Hub suites: 226 passed, 20 skipped
  • changed-file pre-commit hooks: passed
  • independent review: approve

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 _BraindecodeDocstringMeta metaclass to accept subclass keyword arguments (**kwargs) during class creation.
  • Pop license early in EEGModuleMixin.__init_subclass__ so it doesn’t leak into no-Hub paths.
  • Add a subprocess-based regression test that imports Braindecode with huggingface_hub blocked 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.

Comment thread test/unit_tests/models/test_huggingface.py Outdated
Copilot AI review requested due to automatic review settings August 25, 2026 19:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread test/unit_tests/models/test_base.py
Copilot AI review requested due to automatic review settings August 25, 2026 19:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@bruAristimunha
bruAristimunha merged commit 1113d45 into braindecode:master Aug 26, 2026
13 checks passed
@bruAristimunha
bruAristimunha deleted the codex/pr-1132-license-prereq-20260825 branch August 26, 2026 10:28
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants