Skip to content

Log documents_seen / num_documents and offer it as a W&B x-axis#559

Merged
jlamypoirier merged 11 commits into
mainfrom
jlp_documents_seen
Jul 9, 2026
Merged

Log documents_seen / num_documents and offer it as a W&B x-axis#559
jlamypoirier merged 11 commits into
mainfrom
jlp_documents_seen

Conversation

@jlamypoirier

@jlamypoirier jlamypoirier commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Claude Sonnet 5, on behalf of @jlamypoirier.

Summary

Extracted from #553 as the first, independent piece of the RL diagnostics umbrella: the
documents_seen document-clock counter itself. Split out ahead of the weights_ready
document-count model version + reward/model_version staleness metrics (separate PRs) because
those both depend on this counter existing (the model version is documents_seen, and
staleness is computed as documents_seen - model_version) — this PR has no dependents-side
prerequisites and stands alone.

  • ScheduleRunner exposes the whole-step DP-summed document count (num_documents_in_batch,
    the loss-normalization divisor) as _num_documents_in_batch, also returned as part of
    run_step's return tuple.
  • The trainer accumulates it into _documents_seen every step (persisted in checkpoint
    metadata, restored on resume) and logs num_documents + documents_seen in the step metrics
    (console + W&B) on every logging step.
  • Wandb declares training/documents_seen as an alternative step_metric for training/* at
    setup, so it can be used as a cross-run x-axis. The global wandb.log(step=...) step is
    unchanged, so both axes remain available.
  • Document counting is unconditional, not an opt-in loss preprocessing flag. It used to
    activate only as a side effect of GRPO/GSPO's loss preprocessing config requesting it (for
    their own, unrelated loss-normalization divisor) — an obscure, indirect enablement path for a
    training-level metric. Document counting is a real, DP-wide all_reduce per step, so this is a
    deliberate tradeoff (sync cost accepted for every run) rather than an oversight. The now-dead
    return_document_count flag and its now-empty TokenPreprocessingConfig host class were both
    removed, along with a couple of downstream now-always-true conditionals this uncovered
    (TokenModelInput.share_batch_data's guard, the lazy W&B metric-definition dance, the
    metrics-dict presence check).

Tests

  • tests/data/test_streaming.py: 23 passed (CPU).
  • Broader regression pass after making document counting unconditional: test_preprocessing.py,
    test_streaming.py, test_random.py, test_attention.py, test_varlen.py, test_ssm.py,
    test_lm_losses.py, test_lm_head.py — 1450 passed / 35 skipped, 0 failures.
  • No dedicated unit test added for the documents_seen accumulator/checkpoint round-trip; would
    require new checkpoint-test infrastructure for GRPO/GSPO models (currently marked
    not_implemented in tests/utils/model_configs.py) — out of scope here, left as a known gap.
  • The GPU end-to-end integration test (tests/models/test_streaming.py::test_model_streaming,
    which exercises documents_seen) was not run here (no GPU) — worth running in CI before
    merge.

Reintroduce the per-step document count and cumulative documents-seen
metrics on the static-schedule path, independent of `docs_per_step`.

`ScheduleRunner` exposes the whole-step DP-summed document count
(`num_documents_in_batch`, the loss-normalization divisor) as
`_num_documents_in_batch`, set once per step in `_preprocess_data`; it is
`None` when the data carries no document counts (non-RL runs). The trainer
accumulates it into `_documents_seen` every step (persisted in checkpoint
metadata) and logs `num_documents` + `documents_seen` when available.

`Wandb` lazily declares `training/documents_seen` as an alternative
`step_metric` for `training/*` on the first log that carries it, so it can
be used as a cross-run x-axis without affecting non-RL runs. The global
`wandb.log(step=...)` step is unchanged, so both axes remain available.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jlamypoirier and others added 10 commits July 8, 2026 17:35
…rop dead check, reword comments

- Return `num_documents_in_batch` as a 4th element of `ScheduleRunner.run_step`'s tuple instead of
  having the trainer reach into the runner's private `_num_documents_in_batch` attribute — the
  runner already returns everything else it computes (`reduced_losses`, `update_successful`,
  `metrics`) this way, and the private attribute is also mutated by evaluation's `run_step` calls
  on the same shared runner later in the same iteration.
- Drop the `isinstance(values, dict)` guard in `Wandb.log_metrics`: `metrics` is always
  `dict[str, dict[str, float | int]]` per its own type signature.
- Reword comments that named "RL" as the consumer to describe the value instead.
- Dedupe the repeated `f"{PhaseType.training}/documents_seen"` literal into a local variable.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ssing flag

Document counting was only enabled indirectly, by whichever loss's preprocessing
config happened to request it (currently only GRPO/GSPO). Flip
`TokenPreprocessingConfig.return_document_count` to default `True` so
`documents_seen`/`num_documents` populate for any run, and drop the now-redundant
explicit request from the policy-gradient loss config.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Now that document counting is unconditional (previous commit), the flag itself
has no consumer that can turn it off. Drop the field and make the count
computation in TokenBatch._get_model_input unconditional; update the
preprocessing test's cross-product accordingly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tch_data

num_documents is now always set by _get_model_input (no opt-in flag left to
gate it), and no other production or test path constructs a TokenModelInput
bypassing _get_model_input, so the check was always true.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…onfig

TokenPreprocessingConfig added no fields of its own after the previous commit
removed return_document_count. Use LengthPreprocessingConfig (its only base)
directly, matching the sibling PatchPreprocessingConfig.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
documents_seen is now always present in the training metrics (document
counting is unconditional), so the lazy define-on-first-log dance and its
_defined_documents_axis flag were dead weight. Declare the custom x-axis once,
at the same point as the rest of the one-time wandb setup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Document counting is unconditional now, so the presence check was dead;
num_documents/documents_seen show up in both the console log line and W&B
on every logging step instead of only when the check happened to pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… W&B x-axis

- run_step's document count is always an int (all top-level inputs are
  TokenModelInput); drop the illusory int|None plumbing and the trainer's
  None guard.
- Drop the consumer-naming duplicate comment on _num_documents_in_batch.
- Drop the redundant class-level _documents_seen = 0 default.
- Drop the define_metric block so global step stays the default x-axis;
  documents_seen remains a logged, selectable metric.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Extract the real-document-count local in _get_model_input and reuse it
  for both num_documents and num_documents_in_sequence.
- Reword the num_documents comment to describe this rank's contribution.
- Drop the comment restating the documents_seen accumulation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jlamypoirier jlamypoirier merged commit 626db88 into main Jul 9, 2026
1 of 2 checks passed
@jlamypoirier jlamypoirier deleted the jlp_documents_seen branch July 9, 2026 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant