Skip to content

[fix][metrics] Reduce policy_entropy and logprob-diff metrics via sum/count - #1969

Open
ayucpp wants to merge 2 commits into
NovaSky-AI:mainfrom
ayucpp:fix/metric-reduction-sum-nnz
Open

[fix][metrics] Reduce policy_entropy and logprob-diff metrics via sum/count#1969
ayucpp wants to merge 2 commits into
NovaSky-AI:mainfrom
ayucpp:fix/metric-reduction-sum-nnz

Conversation

@ayucpp

@ayucpp ayucpp commented Aug 2, 2026

Copy link
Copy Markdown

Fixes #1822

What does this PR do?

Updates metric aggregation for policy_entropy and minibatch_rollout_logprobs_abs_diff_* to compute the exact global mean instead of a mean-of-means.

Why

Workers reported per-micro-batch masked means, averaged across micro-batches, DP ranks, and mini-batches — over-weighting shards with few unmasked tokens.

Example: Shard A = 1 token (entropy 1.0), Shard B = 100 tokens (entropy 2.0):

  • Old (mean-of-means): (1.0 + 2.0) / 2 = 1.5
  • New (exact): 201 / 101 ≈ 1.99

Token-based batching (or DP workers getting mostly failed trajectories) makes shards imbalanced, so the old value wasn't the global-batch metric.

What changes

  • Workers emit masked per-token sums (policy_entropy_sum, minibatch_rollout_logprobs_abs_diff_sum/_sq_sum) and counts (loss_mask_nnz, ..._nnz); reduce_metrics/all_reduce_metrics always sum these.
  • Final derivations (entropy = sum/count, std = sqrt(E[x²] − E[x]²)) moved to the trainer, computed once.
  • Workers still emit a final policy_entropy after the DP all-reduce, so Tinker _extract_metrics and worker-level GPU assertions are unchanged.
  • Megatron packed-entropy helpers now return the CP-global sum/count, keeping CP invariance.

Testing

  • Added imbalanced-shard exactness tests in test_worker_utils.py and test_skip_fwd_logprobs.py.
  • CPU suite: 1393 passed (-m "not vllm").
  • Pre-commit clean.

…/count

The `policy_entropy` and `minibatch_rollout_logprobs_abs_diff_*` metrics were
reduced as mean-of-means: workers emitted per-micro-batch masked means, which
were then averaged across micro-batches, DP ranks, and mini-batches. With
token-based batching (or any imbalanced sharding) that over-weights
under-sampled shards and no longer equals the metric computed on the global
mini-batch.

Emit the masked per-token sums (`_sum`, `_sq_sum`) and their counts (`_nnz`)
instead. `reduce_metrics` / `all_reduce_metrics` always sum these keys, and the
mean/std are derived once at the final step:

- policy_entropy = policy_entropy_sum / loss_mask_nnz
- logprob-diff mean = sum/nnz, std = sqrt(E[x^2] - E[x]^2)

Workers keep emitting a final `policy_entropy` after the DP all-reduce so
existing consumers (worker-level tests, Tinker `_extract_metrics`) work
unchanged; the raw keys survive so the trainer re-derives the exact global
value after mini-batch aggregation. Megatron packed entropy helpers now return
the global sum/count (all-reduced across CP), keeping CP invariance.

Signed-off-by: ayushflux <ayushflux@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the calculation of policy entropy and rollout logprob difference metrics to prevent "mean-of-means" inaccuracies when dealing with imbalanced shards. Instead of averaging means at the worker level, the code now tracks and reduces raw masked sums and non-zero counts (nnz) across micro-batches, DP ranks, and mini-batches, performing a single final division at the trainer level. A critical issue was identified in megatron_model_wrapper.py where entropy_sum and entropy_nnz are returned as PyTorch tensors in packed sequence branches but as Python floats in non-packed branches. This type mismatch can cause the metrics to be silently skipped during the reduction step, so calling .item() on the tensors is recommended to ensure they are properly logged.

Comment on lines +939 to +940
POLICY_ENTROPY_SUM_KEY: entropy_sum,
LOSS_MASK_NNZ_KEY: entropy_nnz,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In the packed sequence branches (e.g., when calling vocab_parallel_entropy_packed_sequences or from_parallel_hidden_to_entropy_packed_sequences), entropy_sum and entropy_nnz are returned as torch.Tensors on GPU. However, in the non-packed branches (using compute_masked_sum_and_count), they are returned as Python floats.\n\nIf they are torch.Tensors, they will be added to the metrics dictionary as tensors. Later, when reduce_metrics is called, it checks isinstance(x, (int, float)) for each metric value. Since torch.Tensor is not an instance of int or float, these metrics will be silently skipped and omitted from the reduced metrics, preventing policy_entropy from being finalized and logged.\n\nTo fix this, we should call .item() on entropy_sum and entropy_nnz if they are PyTorch tensors.

                POLICY_ENTROPY_SUM_KEY: entropy_sum.item() if torch.is_tensor(entropy_sum) else entropy_sum,\n                LOSS_MASK_NNZ_KEY: entropy_nnz.item() if torch.is_tensor(entropy_nnz) else entropy_nnz,

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ac1318c — I normalized to Python floats right after the branch chain (entropy_sum = float(entropy_sum); entropy_nnz = float(entropy_nnz)), which covers both the packed branches (tensors) and compute_masked_sum_and_count (already floats) in one place. float(tensor) on a scalar tensor is equivalent to .item(), so the metrics now survive the isinstance(x, (int, float)) check in reduce_metrics. Full CPU suite: 1393 passed, no regressions.

The packed entropy helpers return detached GPU tensors while
compute_masked_sum_and_count returns Python floats. reduce_metrics
skips any value that is not an int/float, so tensor-valued
policy_entropy_sum / loss_mask_nnz were silently dropped and
policy_entropy was never finalized for packed megatron runs.

Signed-off-by: ayushflux <ayushflux@gmail.com>
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.

Fix reduction for entropy and other metrics in policy workers

1 participant