diff --git a/every_eval_ever/cron/README.md b/every_eval_ever/cron/README.md index bd9e7e1ec..6eee1ca2a 100644 --- a/every_eval_ever/cron/README.md +++ b/every_eval_ever/cron/README.md @@ -240,6 +240,15 @@ the pull request for the adapter. A refresh the Hub refuses is reported in the run report and the step summary rather than failing the run, because by then the records are published. +A submission that lands completely ends by posting `/eee validate changed` as +a fresh comment on the pull request, because the datastore's validator runs on +that command rather than on push — records nobody comments on are records +nobody validated. It is posted last, after the records and the description, so +what it validates is the finished submission, and never after a partial one: a +retry that completes the submission asks instead. A comment the Hub refuses is +reported like a refused description rewrite, with a note asking a human to +post the command by hand. + ## Setup 1. Nothing, if the token in the next step can create datasets: the first run diff --git a/every_eval_ever/cron/__main__.py b/every_eval_ever/cron/__main__.py index 5ad330de7..c2705b911 100644 --- a/every_eval_ever/cron/__main__.py +++ b/every_eval_ever/cron/__main__.py @@ -609,9 +609,13 @@ def _finish( if submission is not None: pull_request = submission.pull_request committed_paths = submission.committed_paths - if submission.description_note: - notes.append(submission.description_note) - outcome.messages.append(submission.description_note) + for note in ( + submission.description_note, + submission.validation_note, + ): + if note: + notes.append(note) + outcome.messages.append(note) report = outcome.to_manifest() report['raw_reference'] = raw_reference diff --git a/every_eval_ever/cron/submit.py b/every_eval_ever/cron/submit.py index c9c80d6ce..cef568323 100644 --- a/every_eval_ever/cron/submit.py +++ b/every_eval_ever/cron/submit.py @@ -32,6 +32,11 @@ #: How an instance-level sidecar is named after its aggregate. The two are #: one record and are committed together. SAMPLES_SUFFIX = '_samples.jsonl' +#: The comment that asks the datastore's validation bot to check what a pull +#: request now carries. Validation on the Hub side runs on request, not on +#: push: a pull request nobody comments on is a pull request nobody +#: validated (see evaleval/EEE_datastore discussion 168). +VALIDATION_COMMAND = '/eee validate changed' class SubmissionError(RuntimeError): @@ -113,6 +118,11 @@ class Submission: #: Why the pull request body still describes an earlier run, when it does. #: Not a failure: the records are published either way. description_note: str | None = None + #: Why the datastore's validator was not asked to check this pull + #: request, when it was not. Not a failure either, but worth a human + #: reading: an unvalidated pull request sits unreviewed until somebody + #: posts the command by hand. + validation_note: str | None = None def _discussion_number(discussion: Any) -> int | None: @@ -328,6 +338,23 @@ def update_description( f'{type(exc).__name__}: {exc}' ) from exc + def request_validation(self, pull_request: PullRequest) -> None: + """Post :data:`VALIDATION_COMMAND` on a pull request, as a new + comment, which is what makes the datastore validate it.""" + try: + self.api.comment_discussion( + repo_id=self.repo_id, + repo_type='dataset', + discussion_num=pull_request.number, + comment=VALIDATION_COMMAND, + ) + except Exception as exc: # noqa: BLE001 - re-raised with context + raise SubmissionError( + f'could not request validation on pull request ' + f'{pull_request.number} on {self.repo_id}: ' + f'{type(exc).__name__}: {exc}' + ) from exc + def pull_request_status(self, number: int) -> str: """Return ``open``, ``merged`` or ``closed`` for a pull request. @@ -560,14 +587,16 @@ def _upload_batches( ) except Exception as exc: # noqa: BLE001 - re-raised with context landed = self._paths_on_ref(pull_request, batch) - unresolved: list[str] = [] if landed: + # The Hub accepted the commit and only the reply was + # lost, so this batch is on the pull request and the + # upload carries on. Stopping here instead would end a + # run whose every batch landed as a failure nothing + # retries, because its records are all accounted for. committed.extend(landed) - hint = ( - ' The failing batch itself reached the pull request ' - 'despite the error and is counted as committed.' - ) - elif landed is None: + continue + unresolved: list[str] = [] + if landed is None: unresolved = [operation.path_in_repo for operation in batch] hint = ( ' Whether the failing batch landed could not be ' @@ -654,6 +683,10 @@ def publish( body a reviewer reads describes the run that last added to it rather than whichever run opened it. + A submission that lands completely ends by requesting validation + (see :meth:`request_validation`); a partial one leaves that to the + retry that completes it. + An opening commit that errored after landing is adopted rather than repeated, and what it left on the ref decides whether its batch counts as published. @@ -719,10 +752,25 @@ def publish( # The records are in. A stale body is worth reporting and not # worth failing a run that published everything it meant to. note = f'{exc}; the body still describes an earlier run' + validation_note = None + # Last, once the pull request holds everything this run meant to + # publish, so the validator reads the finished submission. A partial + # submission never reaches here, which is deliberate: asking for + # validation of half an upload wastes the reviewer the command + # summons. + try: + self.request_validation(pull_request) + except SubmissionError as exc: + # The records are in, same bargain as the description: report + # that nobody asked for validation rather than fail the run. + validation_note = ( + f'{exc}; post `{VALIDATION_COMMAND}` on it manually' + ) return Submission( pull_request=pull_request, committed_paths=tuple(committed), description_note=note, + validation_note=validation_note, ) @@ -841,6 +889,7 @@ def pull_request_description( 'Submission', 'SubmissionError', 'SAMPLES_SUFFIX', + 'VALIDATION_COMMAND', 'marker', 'pull_request_description', 'pull_request_title', diff --git a/tests/test_cron_store_and_submit.py b/tests/test_cron_store_and_submit.py index 991294265..98c3183bb 100644 --- a/tests/test_cron_store_and_submit.py +++ b/tests/test_cron_store_and_submit.py @@ -61,6 +61,8 @@ def __init__( self.details_error: Exception | None = None self.edit_comment_error: Exception | None = None self.edited_comments: list[tuple[int, str]] = [] + self.comment_error: Exception | None = None + self.posted_comments: list[tuple[int, str]] = [] self.list_files_error: Exception | None = None self.whoami_error: Exception | None = None self.repo_info_error: Exception | None = None @@ -152,6 +154,12 @@ def edit_discussion_comment( return type('Comment', (), {'content': new_content})() raise EntryNotFoundError(f'discussion {discussion_num} not found') + def comment_discussion(self, *, discussion_num, comment, **kwargs): + if self.comment_error is not None: + raise self.comment_error + self.posted_comments.append((discussion_num, comment)) + return type('Comment', (), {'content': comment})() + def list_repo_files(self, repo_id=None, **kwargs): if self.list_files_error is not None: raise self.list_files_error @@ -1106,15 +1114,17 @@ def fail_on_second(**kwargs): ) -def test_a_batch_that_landed_despite_the_error_is_counted_committed( +def test_a_batch_that_landed_despite_the_error_does_not_stop_the_upload( tmp_path, ) -> None: """The ambiguous timeout: the Hub accepted the commit, the client saw an - error. Reporting only the earlier batches would make the caller's ledger - forget this one, and the retry would republish it under fresh UUID paths. - The pull request ref is the arbiter of what actually landed.""" + error. The pull request ref is the arbiter of what actually landed, and a + batch that is on it is a success, so the upload carries on. Stopping + instead turned a run whose final batch landed this way into a failure + with every record accounted for, which no retry ever completed and so + nothing ever validated.""" tree = _upload_tree(tmp_path, 7) - hub = FakeHub() + hub = FakeHub(discussions=[cron_pr(12)]) sub = submit.DatastoreSubmitter(hub, batch_size=3) pull_request = submit.PullRequest(12, 'https://x/12', 'refs/pr/12', 'x') @@ -1128,20 +1138,19 @@ def land_then_time_out(**kwargs): hub.create_commit = land_then_time_out - with pytest.raises(submit.PartialSubmissionError) as caught: - sub.publish( - 'hle', - pull_request=pull_request, - operations=submit.upload_operations(tree), - description='', - message='hle 2026-08-10', - ) + submission = sub.publish( + 'hle', + pull_request=pull_request, + operations=submit.upload_operations(tree), + description='body', + message='hle 2026-08-10', + ) - # Both the clean first batch and the ambiguous second one are reported; - # only the never-attempted third is left for the retry. - assert len(caught.value.committed_paths) == 6 - assert caught.value.unresolved_paths == () - assert 'reached the pull request despite the error' in str(caught.value) + # The clean first batch and the two ambiguous ones all count; nothing is + # left for a retry, and the finished submission is validated. + assert len(submission.committed_paths) == 7 + assert hub.posted_comments == [(12, submit.VALIDATION_COMMAND)] + assert submission.validation_note is None def test_an_unanswerable_reconciliation_claims_nothing(tmp_path) -> None: @@ -1373,6 +1382,94 @@ def test_a_description_that_cannot_be_refreshed_does_not_fail_the_run( assert 'describes an earlier run' in submission.description_note +# --- validation is asked for once everything is in ------------------------ + + +def test_a_full_submission_into_a_reused_pull_request_asks_for_validation( + tmp_path, +) -> None: + """The datastore validates on request, so a run that published records + has to post the command or nothing checks them.""" + tree = _upload_tree(tmp_path, 2) + hub = FakeHub(discussions=[cron_pr(12)]) + sub = submit.DatastoreSubmitter(hub) + pull_request = submit.PullRequest(12, 'https://x/12', 'refs/pr/12', 'x') + + submission = sub.publish( + 'hle', + pull_request=pull_request, + operations=submit.upload_operations(tree), + description='body', + message='hle 2026-08-10', + ) + + assert hub.posted_comments == [(12, submit.VALIDATION_COMMAND)] + assert submission.validation_note is None + + +def test_a_newly_opened_pull_request_asks_for_validation(tmp_path) -> None: + tree = _upload_tree(tmp_path, 2) + hub = FakeHub() + sub = submit.DatastoreSubmitter(hub) + + submission = sub.publish( + 'hle', + pull_request=None, + operations=submit.upload_operations(tree), + description='body', + message='hle 2026-08-10', + ) + + number = submission.pull_request.number + assert hub.posted_comments == [(number, submit.VALIDATION_COMMAND)] + assert submission.validation_note is None + + +def test_a_validation_request_that_fails_does_not_fail_the_run( + tmp_path, +) -> None: + """The records are published either way; the note tells a human to post + the command by hand.""" + tree = _upload_tree(tmp_path, 2) + hub = FakeHub(discussions=[cron_pr(12)]) + hub.comment_error = RuntimeError('403 Forbidden') + sub = submit.DatastoreSubmitter(hub) + pull_request = submit.PullRequest(12, 'https://x/12', 'refs/pr/12', 'x') + + submission = sub.publish( + 'hle', + pull_request=pull_request, + operations=submit.upload_operations(tree), + description='body', + message='hle 2026-08-10', + ) + + assert len(submission.committed_paths) == 2 + assert '403 Forbidden' in submission.validation_note + assert submit.VALIDATION_COMMAND in submission.validation_note + + +def test_a_partial_submission_asks_for_no_validation(tmp_path) -> None: + """Validation of half an upload wastes the reviewer it summons; the + retry that completes the submission asks instead.""" + tree = _upload_tree(tmp_path, 2) + hub = FakeHub(discussions=[cron_pr(12)]) + hub.commit_error = RuntimeError('504 Gateway Timeout') + sub = submit.DatastoreSubmitter(hub) + pull_request = submit.PullRequest(12, 'https://x/12', 'refs/pr/12', 'x') + + with pytest.raises(submit.PartialSubmissionError): + sub.publish( + 'hle', + pull_request=pull_request, + operations=submit.upload_operations(tree), + description='body', + message='hle 2026-08-10', + ) + + assert hub.posted_comments == [] + + # --- what happened to the last pull request -------------------------------