allenai / allenai/open-instruct

Numpy SFT cache is not invalidated by changes to label derivation

Offen
#1,815 1 Kommentar 0 Reaktionen 1 zugewiesene Person Beansprucht von @abhishekraok Auf GitHub ansehen
Vorherrschende Sprache
Python
Sterne
3.9k
Forks
585
Ø Merge
5 T. 17 Std.
Gemergte PRs (30 T.)
16

Beschreibung

The numpy SFT cache key describes the *configuration* that produced the labels, but not the *code*:

```python
cache_hash = compute_config_hash(dcs, tc) # chat template, transform fns, tokenizer
seed_suffix = _seed_cache_suffix(seed, max_seq_length)
numpy_dir = f"{local_cache_dir}/{_NUMPY_SFT_SUBDIR}/{cache_hash}-{seed_suffix}"
```
(`olmo_core_finetune.py:135-137`, plus `DATASET_CACHE_VERSION` inside `compute_config_hash`.)

So a change to assistant-label derivation does not invalidate anything. `_numpy_dir_is_populated(numpy_dir)` returns True and the run reuses `labels_mask_part_*.npy` produced by the older code, with no warning.

## Why this matters now

#1806 changes which tokens are trainable. Anyone who lands it and re-runs against an existing cache trains on the **old** labels and sees no indication. The failure is silent and the artifacts look normal — the numpy files are present and correctly shaped, they just encode the previous masking.

Concretely: a validation run for #1806 launched against the full `tulu-3-sft-olmo-2-mixture` completed in ~40 s with 0 rows processed and no dataset statistics, because the key matched a cache built before the fix. Reported as "0 dropped rows" it looks like an improvement over the previous 15. It was zero work. Forcing a different `--seed` was the only way to get a real measurement.

## Relation to #1801

#1801 fixes the mirror case: on a cache **miss**, the printed remediation command omitted cache-key args, so following it re-tokenized under a different key and failed identically. Same root cause — the key is subtle and under-documented — but the failure modes differ in severity. A false miss fails loudly. A false hit silently serves stale labels.

## Options

1. **Mix the code version into the key.** Hash the source of the transform functions, or bump `DATASET_CACHE_VERSION` whenever label derivation changes. The latter is already the convention, just easy to forget — nothing enforces it.
2. **Record provenance and warn.** Write the git commit into the cache directory alongside `dataset_statistics.json`, and log loudly when reusing a cache built by a different commit. Cheaper, does not force re-tokenization, and would have made the situation above obvious.
3. **Document it.** At minimum note in `AGENTS.md` that changing tokenization requires bumping `DATASET_CACHE_VERSION` or using a fresh `--seed`.

(2) plus (3) seems like the best return: no forced re-tokenization on unrelated changes, but a stale cache stops being invisible.

Worth noting `DATASET_CACHE_VERSION` was not bumped in #1806 — deliberately, since the mixture-level output is unchanged for the overwhelming majority of rows, but that is exactly the judgement call this issue is about.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.