Fix silent truncation when -s/-t/-r line counts differ - #278
Open
shaurya416 wants to merge 1 commit into
Open
shaurya416 wants to merge 1 commit into
shaurya416 wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #277
Cause
comet-score(comet/cli/score.py) andcomet-compare(comet/cli/compare.py) read-s,-tand-ras separate line lists and pair them by index withzip(), 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-comparepasses mismatched lists toscore()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: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 oftests/unit). It runsscore_command()/compare_command()on small temp files, withload_from_checkpointpatched to a fake model that records the samples it is asked to score. Forcomet-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_cfgandcompare_command(compare.py) andsplit_sequence_into_sublists(models/utils.py) withast.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 withpython3 -m unittest -v(Python 3.12.4).test_score_translation_shorter_than_sourcetest_score_reference_shorter_than_sourcetest_score_short_system_with_multiple_gpus--gpus 2)test_compare_source_shorter_than_translationstest_compare_translation_shorter_than_sourcetest_score_alignedtest_score_two_aligned_systems(--gpus 1and2)test_score_reference_free_alignedtest_score_counts_lines_as_read(no trailing newline, U+2028 in a segment)test_compare_alignedtest_compare_reference_free_alignedtest_score_missing_references_still_rejected(existing error, exits 2 both times)test_compare_single_system_still_rejected(existing assert, raises both times)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: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:
len(sources)withlen(translations), which is the number of systems:test_score_aligned,test_score_two_aligned_systems,test_score_reference_free_alignedandtest_score_counts_lines_as_readfail.str.splitlines():test_score_counts_lines_as_readfails.test_score_reference_free_alignederrors.Stand-ins used in the stub packages (all hand-written):
jsonargparse.ArgumentParser: a plainargparse.ArgumentParsersubclass. 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: astrsubclass that rejects missing files and hasrel_path,__call__()returning the absolute path and__str__returningrel_path, matching 3.13.1.numpy:array(a list withtolist()) andarray_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 patchload_from_checkpoint, and--modelpoints to an existing.ckptfile, sodownload_modelis never reached.comet.cli.compare.score: a placeholder. Every compare test patches it.Formatting:
black24.8.0 andisort5.13.2 (--profile black) are clean on the new test file. Neither tool is pinned in the repo. Onscore.pyandcompare.py, black reports the same changes to existing lines before and after the patch, and none to the added lines.Not run:
comet-score/comet-compareend to end with a real model, on GPU (including--gpus > 1on real GPUs), or with-d(SacreBLEU)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.