autoregressive.order accepts any value and silently ignores it #30

New Issue

2026-08-13T15:05:33+02:00

lars commented

2026-08-13 15:05:33 +02:00

Severity: low.

giant/config.py:437-439 documents order as "single-valued for now; the key
exists so an alternative ordering is not a config break" — a reasonable
placeholder. But validate_config checks its two siblings (history,
teacher_forcing, giant/config.py:1274-1284) and not order, so
order = "energy_asc" is accepted, saved, and silently trains
descending-energy.

Fix: one-line check alongside the neighbouring two.

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

**Severity: low.** `giant/config.py:437-439` documents `order` as "single-valued for now; the key exists so an alternative ordering is not a config break" — a reasonable placeholder. But `validate_config` checks its two siblings (`history`, `teacher_forcing`, `giant/config.py:1274-1284`) and not `order`, so `order = "energy_asc"` is accepted, saved, and silently trains descending-energy. **Fix:** one-line check alongside the neighbouring two. --- Migrated from `issues.md` (v0.3.0 branch review, 2026-08-13), Issue 4.

lars added the bug label 2026-08-13 15:05:33 +02:00

lars commented

2026-08-13 15:40:57 +02:00

Fixed in 899ca3a on v0.3.0-stage2-autoregressive.

Added a check for stage2_model.autoregressive.order in validate_config (giant/config.py), alongside the existing history/teacher_forcing checks, gated the same way (only under stage2_model.decoder = 'autoregressive'): any value other than 'energy_desc' (the only implemented ordering) now raises ValueError instead of being silently accepted. Added tests for the pass/fail cases and extended the existing one-shot-skips-AR-checks test to cover order too. Also updated the reason string on the pre-existing _KNOWN_UNUSED allow-list entry for this key in tests/test_config_consumed_keys.py, since it previously said validate_config never checked order — that's no longer accurate, though the key stays allow-listed since validate_config itself isn't in that test's build/train/rollout consumer whitelist (order is still single-valued, so nothing branches on its value yet). No follow-up filed — implementing an actual alternative ordering is out of scope and wasn't asked for.

Fixed in 899ca3a on v0.3.0-stage2-autoregressive. Added a check for `stage2_model.autoregressive.order` in `validate_config` (giant/config.py), alongside the existing `history`/`teacher_forcing` checks, gated the same way (only under `stage2_model.decoder = 'autoregressive'`): any value other than `'energy_desc'` (the only implemented ordering) now raises `ValueError` instead of being silently accepted. Added tests for the pass/fail cases and extended the existing one-shot-skips-AR-checks test to cover `order` too. Also updated the reason string on the pre-existing `_KNOWN_UNUSED` allow-list entry for this key in tests/test_config_consumed_keys.py, since it previously said validate_config never checked order — that's no longer accurate, though the key stays allow-listed since validate_config itself isn't in that test's build/train/rollout consumer whitelist (order is still single-valued, so nothing branches on its value yet). No follow-up filed — implementing an actual alternative ordering is out of scope and wasn't asked for.

lars closed this issue

2026-08-13 15:40:57 +02:00

lars referenced this issue from a commit

2026-08-13 16:21:01 +02:00

Validate stage2_model.autoregressive.order in validate_config (gitea #30)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lars/giant#30