Skip to content

fix(metrics): score only sequences that carry a supervised label in seq_acc - #10247

Merged
tastelikefeet merged 1 commit into
modelscope:mainfrom
Lesereingrape:fix/seq-acc-unsupervised
Sep 26, 2026
Merged

tastelikefeet merged 1 commit into
modelscope:mainfrom
Lesereingrape:fix/seq-acc-unsupervised

Conversation

@Lesereingrape

Copy link
Copy Markdown
Contributor

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

PR information

Why

compute_acc(..., acc_strategy='seq') decides whether a sequence was answered correctly with

acc_list.append(np.all(preds[i, m] == labels[i, m]))   # acc.py:41 (padded)  / :38 (padding_free)

where m = labels[i] != -100. When a sequence carries no supervised label at all (m is all-False), the selection is empty and np.all([]) is True, so that sequence is scored as a correct prediction. Measured on c08110b3:

labels = [[-100,-100, 5, 6], [-100,-100,-100,-100]]
preds  = [[   0,   7, 9, 0], [   0,   1,   2,   3]]
compute_acc(preds, labels, acc_strategy='seq')['seq_acc']  ->  [False, True]
                                                                    ^^^^^^ the unsupervised row is a "hit"

and for a batch where nothing is supervised the reported seq_acc mean is 1.0 ({'seq_acc': [True, True]}).

That list goes straight into the trainer: swift/trainers/mixin.py:1222-1225 feeds it to MeanMetric.update(v), whose update counts len(state) — so every unsupervised row adds 1 to both the numerator and the denominator. eval/seq_acc is therefore inflated by exactly the rows that contain no information, and the more aggressively a run truncates, the higher it reads.

The same empty selection also breaks the token path: acc_list stays empty, compute_acc still returns {'token_acc': []}, and AccMetrics.compute_metrics (acc.py:55) does sum(v) / len(v) →

ZeroDivisionError: division by zero

(reproduced with --eval_metric acc on an all--100 eval set). compute_acc already returns {} for "nothing to score" at :19 and :26, so both of these are just two paths that were never connected to that existing contract.

How a row ends up without a supervised label is documented behaviour, not a corner of the code. With a non-default --truncation_strategy (Literal['delete', 'left', 'right', 'split'], swift/arguments/base_args/template_args.py:132, documented in docs/source_en/Instruction/Command-line-parameters.md):

  • right: template/base.py:_encode_truncated calls _truncate, which protects the first max_length - n_protected unprotected positions and drops the rest — the tail. Since the response is the tail, a row cut hard enough keeps no supervised label at all.
  • split: _encode_truncated slices labels[i:i + max_length] into chunks and returns every chunk, so a sample longer than max_length contributes trailing chunks that are pure padding-only context, i.e. all--100 rows that are still fed to the trainer as if they were supervised sequences.

delete (the default) drops such rows entirely, so this needs an explicit flag — but with right/split set, an all--100 row is a normal batch outcome rather than a corrupt-input edge case.

What changed

  • Skip a sequence that has no supervised label instead of scoring it: if not mask.any(): continue on the padding_free branch and if not m.any(): continue on the padded branch.
  • If nothing was scored at all, return {} like the two existing early returns, which keeps AccMetrics.compute_metrics and mixin.py:1222 on the "no metric this step" path they already handle instead of dividing by zero.

No change for any sequence that has at least one supervised label: the value is the same np.all(...) over the same mask.

Tests

tests/utils/test_acc_metrics.py (the file added by #10049) grows four cases, keeping its existing unittest.TestCase style and fixtures:

  • test_padded_seq_acc_skips_a_sequence_without_supervised_labels — the batch above now reports [False], i.e. one scored sequence instead of a hit and a miss.
  • test_padding_free_seq_acc_skips_a_sequence_without_supervised_labels — same through cu_seqlens.
  • test_seq_acc_reports_nothing_when_no_label_is_supervised and test_token_acc_reports_nothing_when_no_label_is_supervised — {} instead of the empty list that crashed the metric hook.
  • The two tests that already covered supervised rows are untouched and still pass.

Bug fix verification

Red on unmodified c08110b3 (4 of the 6 tests fail), green with the change:

$ python -m unittest tests.utils.test_acc_metrics   # before
FAILED (failures=4)
$ python -m unittest tests.utils.test_acc_metrics   # after
Ran 6 tests ... OK

Lint with the versions pinned in .pre-commit-config.yaml:

flake8 --config=setup.cfg swift/metrics/acc.py tests/utils/test_acc_metrics.py   # clean
isort --check-only --settings-path setup.cfg <same files>                         # clean
yapf --style setup.cfg --diff <same files>                                        # no diff

I did not run a training job. What was executed here is compute_acc itself — the [False, True] inflation, the 1.0 mean on an all--100 batch, the ZeroDivisionError from the AccMetrics mean, and the six unit tests; a row that keeps at least one supervised label returns byte-identical values before and after (checked on [[-100,-100,-100,7]] → {'seq_acc': [False]} in both). The two --truncation_strategy routes above are read from _truncate/_encode_truncated, not produced by an end-to-end encode.


Disclosure: this PR was prepared, tested and submitted by an AI agent working on behalf of the account owner.

…eq_acc

`np.all()` of an empty selection is True, so a sequence whose labels are all
ignored (-100) was appended to `seq_acc` as a correct prediction: a batch with
no supervised label at all reported seq_acc as 1.0, and unsupervised rows also
inflated the mean through the trainer's MeanMetric denominator. The same empty
selection made `AccMetrics.compute_metrics` divide by zero for token_acc once
nothing was supervised.

Skip sequences without a supervised label and return no metric when there is
nothing to score, which is what the existing float/shape guards already do.
@tastelikefeet
tastelikefeet merged commit 4967fd9 into modelscope:main Sep 26, 2026
3 checks passed
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.

2 participants