Skip to content

FIX: stop scorer-metrics registry rewrites from deleting entries they could not read - #2797

Open
fei (feiiiiii5) wants to merge 8 commits into
microsoft:mainfrom
feiiiiii5:fix/scorer-registry-rewrite-safety
Open

fei (feiiiiii5) wants to merge 8 commits into
microsoft:mainfrom
feiiiiii5:fix/scorer-registry-rewrite-safety

Conversation

@feiiiiii5

Copy link
Copy Markdown
Contributor

Description

Fixes #2796

replace_evaluation_results() rebuilt the JSONL metrics registry from _load_jsonl(), which is a lookup helper: it drops lines it cannot parse, and when the read itself fails it returns the partial list it managed to read. Used as the source of truth for a truncating rewrite, both behaviours delete data — a torn line disappeared permanently, and one undecodable byte left a registry file holding only the new entry while the call returned normally. Those files ship in the package (pyrit/datasets/scorer_evals/) and hold scores that cost hours of model calls; #2796 has the measured output.

The rewrite now reads through a private _read_registry_lines(), which returns (raw line, parsed) pairs: unparseable lines are preserved verbatim, and a read error propagates before the file is touched. The new contents move into place with a temp file plus os.replace(), which is what the docstring's "atomic operation" claim needed anyway. _load_jsonl() is untouched, so lookups stay as lenient as they are today (test_load_jsonl_skips_invalid_json still pins that behaviour).

Two consequences to weigh, since neither is invisible:

  • A registry that cannot be read now raises out of replace_evaluation_results() instead of rewriting from a partial read. _write_metrics_to_registry() already wraps this call in except Exception with a warning (pyrit/score/scorer_evaluation/scorer_evaluator.py:619), so an evaluation run still completes; it just keeps the old registry instead of truncating it.
  • Preserving unparseable lines means a rewrite no longer re-normalises JSON it did not touch (previously every line was re-json.dumps-ed). Entries keep their exact bytes, including unknown fields and float formatting. If the registry is meant to stay in canonical form instead, keeping only the failed lines verbatim and re-serialising the rest is a small change to the same function — say which you prefer.

Tests and Documentation

  • Two regression tests in tests/unit/score/test_scorer_metrics_io.py: test_replace_evaluation_results_keeps_unparseable_lines and test_replace_evaluation_results_leaves_registry_intact_after_a_failed_read. On fc692226 both fail — the first with the rewrite deleted a line it could not read, the second with DID NOT RAISE UnicodeDecodeError — and that file then reads 2 failed, 29 passed. On this branch: 31 passed.
  • pytest -q tests/unit/score/ (2330 tests, offline, no API keys): passed on this branch, with the two new tests included.
  • ruff check and ruff format --check on both changed files: clean.
  • Not run: ty type checking (not available in this environment) and JupyText (no documentation files changed; the only docstrings added are on the two new private helpers).

replace_evaluation_results() rebuilt the JSONL registry from _load_jsonl(),
which is a lookup helper that drops lines it cannot parse and returns a short
list when the read fails. Every rewrite therefore deleted those entries for
good, and a decode error could reduce a registry of pre-computed metrics to
the single new entry while the run reported success.

Read the file as raw lines for the rewrite instead: unparseable lines are
preserved verbatim, read errors propagate before the file is touched, and the
new contents move into place with os.replace() as the docstring already
claimed.
Comment thread pyrit/score/scorer_evaluation/scorer_metrics_io.py Outdated
Comment thread pyrit/score/scorer_evaluation/scorer_metrics_io.py Outdated
Comment thread pyrit/score/scorer_evaluation/scorer_metrics_io.py Outdated
@feiiiiii5

Copy link
Copy Markdown
Contributor Author

Good catch, and thanks for the three concrete checks. I addressed them in c2c94cda:

  • The staging file is now created exclusively and closed before os.replace(), so independent writers cannot share a path.
  • An existing registry's permission bits are copied to the staging file before publication.
  • Reads use newline="" and rewrites retain the original raw line content and line endings; tests cover whitespace, CRLF, a missing final newline, permissions, and distinct staging names.

Test: pytest -q tests/unit/score/test_scorer_metrics_io.py — 34 passed. pytest -q tests/unit/score/ — 2333 passed. ruff check and ruff format --check passed. I did not run ty because it is not installed locally.

Comment thread pyrit/score/scorer_evaluation/scorer_metrics_io.py Outdated
@feiiiiii5

Copy link
Copy Markdown
Contributor Author

Thanks for the two additional checks. I addressed them in 9be09ba5:

  • New staging files are created with O_EXCL and mode 0666, so the OS applies the normal process umask instead of always producing a private temporary file. Existing registries still have their permission bits copied before publication.
  • Staging cleanup now tolerates Windows read-only files by making only the disposable staging file writable, retrying deletion, and logging cleanup failures without masking the original os.replace exception.

Added regression tests for new-file umask permissions and for preserving the original replace error while removing a read-only staging file.

Validation: pytest -q tests/unit/score/test_scorer_metrics_io.py — 36 passed; pytest -q tests/unit/score/ — 2335 passed; ruff check and ruff format --check passed. I could not execute the Windows-specific filesystem path on macOS, so that behavior is covered by a platform-independent mocked cleanup test.

Comment thread pyrit/score/scorer_evaluation/scorer_metrics_io.py Outdated

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.

replace_evaluation_results() deletes registry lines it could not read

2 participants