Skip to content

Fix silent truncation when -s/-t/-r line counts differ - #278

Open
shaurya416 wants to merge 1 commit into
Unbabel:masterfrom
shaurya416:fix-cli-line-count-check
Open

shaurya416 wants to merge 1 commit into
Unbabel:masterfrom
shaurya416:fix-cli-line-count-check

Conversation

@shaurya416

Copy link
Copy Markdown

Fixes #277

Cause

comet-score (comet/cli/score.py) and comet-compare (comet/cli/compare.py) read -s, -t and -r as separate line lists and pair them by index with zip(), which stops at the shortest input. Neither command checks the line counts. A translation file that is one line short gets scored with no error: the extra source/reference lines are dropped. With --gpus > 1, all systems are flattened before pairing, so every system after the short one is scored against the wrong source and reference lines. comet-compare passes mismatched lists to score() in the same way.

Change

Both commands now compare the line counts of every input file right after reading them, before any pairing or scoring. On a mismatch they call parser.error() with each file and its count:

comet-score: error: All input files must have the same number of lines, got: demo/src.txt: 4 lines, demo/hyp1.txt: 3 lines, demo/hyp2.txt: 3 lines, demo/ref.txt: 4 lines

The counts come from the same lists that get paired, so a line is counted exactly as the CLI reads it (for example a missing trailing newline, or a U+2028 inside a segment, does not change the count). References are only checked when they are given, so reference-free models are unaffected. The same check is added to each command. There is no shared CLI module, so I did not add a helper.

New test: tests/unit/test_cli_line_counts.py (unittest, like the rest of tests/unit). It runs score_command() / compare_command() on small temp files, with load_from_checkpoint patched to a fake model that records the samples it is asked to score. For comet-compare, score() is patched to record the systems it receives and stop.

Validation

I did not install or import the package. I extracted the verbatim text of score_command (score.py), get_cfg and compare_command (compare.py) and split_sequence_into_sublists (models/utils.py) with ast.get_source_segment, once from the pre-change files and once from the patched files. I put that text in small stub packages and ran the new test file against each copy with python3 -m unittest -v (Python 3.12.4).

Test Kind Before fix After fix
test_score_translation_shorter_than_source bug FAIL (no error, scoring ran) pass
test_score_reference_shorter_than_source bug FAIL pass
test_score_short_system_with_multiple_gpus bug (--gpus 2) FAIL pass
test_compare_source_shorter_than_translations bug FAIL pass
test_compare_translation_shorter_than_source bug FAIL pass
test_score_aligned control pass pass
test_score_two_aligned_systems (--gpus 1 and 2) control pass pass
test_score_reference_free_aligned control pass pass
test_score_counts_lines_as_read (no trailing newline, U+2028 in a segment) control, correct input pass pass
test_compare_aligned control pass pass
test_compare_reference_free_aligned control pass pass
test_score_missing_references_still_rejected (existing error, exits 2 both times) control pass pass
test_compare_single_system_still_rejected (existing assert, raises both times) control pass pass

Totals: before, 13 run and 5 failed (all five with AssertionError: None != 2, meaning no error was raised). After, 13 run, OK.

With the pre-change score_command, --gpus 2, a 4-line source/reference and two 3-line systems, the run completed without an error. The samples handed to the model were:

{'src': 'Hello.', 'mt': 'Bonjour.', 'ref': 'Bonjour.'}
{'src': 'Good morning.', 'mt': 'Bonjour !', 'ref': 'Bonjour.'}
{'src': 'Thank you.', 'mt': 'Merci.', 'ref': 'Merci.'}
{'src': 'Goodbye.', 'mt': 'Salut.', 'ref': 'Au revoir.'}
{'src': 'Hello.', 'mt': 'Bon matin.', 'ref': 'Bonjour.'}
{'src': 'Good morning.', 'mt': 'Merci bien.', 'ref': 'Bonjour.'}

System 2's first line (Salut.) is paired with the fourth source line. After the change, the same input exits with code 2 and prints the error shown above.

I also ran the tests against three simplified versions of the check in score.py, to confirm the controls catch them:

  • comparing len(sources) with len(translations), which is the number of systems: test_score_aligned, test_score_two_aligned_systems, test_score_reference_free_aligned and test_score_counts_lines_as_read fail.
  • recounting with str.splitlines(): test_score_counts_lines_as_read fails.
  • always including references: test_score_reference_free_aligned errors.

Stand-ins used in the stub packages (all hand-written):

  • jsonargparse.ArgumentParser: a plain argparse.ArgumentParser subclass. jsonargparse 3.13.1's default error handler prints the usage and exits with code 2, which is what argparse does.
  • jsonargparse.typing.Path_fr: a str subclass that rejects missing files and has rel_path, __call__() returning the absolute path and __str__ returning rel_path, matching 3.13.1.
  • numpy: array (a list with tolist()) and array_split.
  • pytorch_lightning.seed_everything: no-op.
  • sacrebleu.utils.get_source_file / get_reference_files: raise if called (not reached).
  • comet.download_model / comet.load_from_checkpoint: raise if called unpatched. The tests patch load_from_checkpoint, and --model points to an existing .ckpt file, so download_model is never reached.
  • comet.cli.compare.score: a placeholder. Every compare test patches it.

Formatting: black 24.8.0 and isort 5.13.2 (--profile black) are clean on the new test file. Neither tool is pinned in the repo. On score.py and compare.py, black reports the same changes to existing lines before and after the patch, and none to the added lines.

Not run:

  • the new test or the existing suite against the real package with its real dependencies (torch, jsonargparse 3.13.1, numpy, pytorch-lightning)
  • comet-score / comet-compare end to end with a real model, on GPU (including --gpus > 1 on real GPUs), or with -d (SacreBLEU)
  • Python 3.8 / 3.9 (the CI matrix)

Notes

The check runs after the model is loaded, because both commands read the input files after that point today. Moving the reads before model loading would fail faster but would be a larger change.

Open PR #271 reformats both CLI files (quote style), including the lines next to this change. Whichever PR is merged second will need a small rebase.

comet-score and comet-compare pair sources, translations and references
by line index with zip(), which stops at the shortest input. A file
that is one line short was scored without any error: the extra lines
were dropped, and with --gpus > 1 every later system was paired with
the wrong source and reference lines.

Compare the line counts of all input files after reading them and call
parser.error() with each file and its count when they differ.

This branch has not been deployed

No deployments
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.

comet-score / comet-compare do not check that -s, -t and -r have the same number of lines (silent truncation, cross-system mis-pairing with --gpus > 1)

1 participant