allenai / allenai/open-instruct

Numpy SFT cache is not invalidated by changes to label derivation

Aperta
#1,815 1 commento 0 reazioni 1 assegnatario Rivendicata da @abhishekraok Vedi su GitHub
Lingua principale
Python
Stelle
3.9k
Fork
585
Merge medio
5g 17h
PR unite (30g)
16

Descrizione

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.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.