HistoryEncoder interface is incomplete, so callers isinstance-check AttentionHistory #35

New Issue

2026-08-13T15:07:03+02:00

lars commented

2026-08-13 15:07:03 +02:00

Stage2Autoregressive.init_history_cache and .history_step
(giant/model/models.py:419-440) both branch on
isinstance(self.history_encoder, AttentionHistory). The base class
(giant/model/history.py:8-22) declares only forward, and its docstring
explains that init_cache/step are conditionally-implemented extras.

So a third history type must be known to Stage2Autoregressive to work,
which is the definition of a non-pluggable plug point. history is also
validated as a two-value enum in three separate places (config.py:1276,
models.py:320, and the from_dict default).

Proposal: move init_cache/step onto HistoryEncoder with the O(1)
defaults MarkovHistory already satisfies (init_cache → None,
step(f, h, c) → (self(f, h), c)), add HISTORY_REGISTRY + register_history,
and delete both isinstance checks. Validation then becomes "is it in the
registry", in one place.

Payoff: history = "gru", "mean_prefix", or "none" (a real ablation:
does the AR decoder's history earn its parameters?) each become a single
registered class. Small, self-contained, no checkpoint impact — good first
issue.

Migrated from issues.md (v0.3.0 branch review, 2026-08-13), Issue 10.

`Stage2Autoregressive.init_history_cache` and `.history_step` (`giant/model/models.py:419-440`) both branch on `isinstance(self.history_encoder, AttentionHistory)`. The base class (`giant/model/history.py:8-22`) declares only `forward`, and its docstring explains that `init_cache`/`step` are conditionally-implemented extras. So a third history type must be *known to* `Stage2Autoregressive` to work, which is the definition of a non-pluggable plug point. `history` is also validated as a two-value enum in three separate places (`config.py:1276`, `models.py:320`, and the `from_dict` default). **Proposal:** move `init_cache`/`step` onto `HistoryEncoder` with the O(1) defaults `MarkovHistory` already satisfies (`init_cache → None`, `step(f, h, c) → (self(f, h), c)`), add `HISTORY_REGISTRY` + `register_history`, and delete both `isinstance` checks. Validation then becomes "is it in the registry", in one place. **Payoff:** `history = "gru"`, `"mean_prefix"`, or `"none"` (a real ablation: does the AR decoder's history earn its parameters?) each become a single registered class. Small, self-contained, no checkpoint impact — good first issue. --- Migrated from `issues.md` (v0.3.0 branch review, 2026-08-13), Issue 10.

lars added the modularity label 2026-08-13 15:07:03 +02:00

lars referenced this issue

2026-08-13 15:08:04 +02:00

Add "none" variants for every plug point (router, history, trunk) #45

lars added a new dependency 2026-08-13 15:23:30 +02:00

#45 Add "none" variants for every plug point (router, history, trunk)

Sign in to join this conversation.