Refactor orchestrator runner#193
Conversation
by removing fields that are not present in both, and allowing non-ASCII characters (which was only the case in `validate_existing()`).
…_finished()` in `run()` like it's done in `validate_existing()`. In practice, this `check_conversation_finished()` was not reachable because failed records always had an entry in `rerun_history`, but that seems fragile.
in `run()` like it's done in `validate_existing()`.
in `run()` like it's done in `validate_existing()`.
Remove the early-return for incomplete conversations in `_run_and_pipeline()`. It returned before `await audio_task`, leaving the audio task running un-awaited. The pipeline then proceeded without flushing the WAV and any exception went unobserved. This mattered most for `time_limit_exceeded` records, which are later skip-gate validated and can be accepted, so their audio must be written first. Classification is unchanged: an unfinished conversation still fails the `conversation_valid_end` gate in `validate_one()` and is bucketed `not_finished`.
The worker writes `result.json` itself on the happy path, but its post-processing-exception path returns a result without writing, relying
on the runner to do it. The worker code include this comment:
# Return a failed result so result.json still gets written by the
# runner, rather than raising and leaving no result.json on disk.
But, `run()` didn't actually write it, until now.
Extract `run()`'s attempt loop into `_run()` and reuse it in `validate_existing()`. This removes duplicated code, and `validate_existing()` gains a few features that only `run()` had: - per-record pipelining: metrics fire as soon as a conversation completes instead of waiting for all conversations to complete; - time-limit acceptance: time_limit_exceeded records that pass skip-gate LLM validation are accepted; and - a progress bar. The preceding commits brought `run()` on par with `validate_existing()', so this commit simply removes the duplication.
so we can differentiate multiple runs launched at once.
59acecf to
56e5bea
Compare
raghavm243512
left a comment
There was a problem hiding this comment.
lgtm but we could just keep total attempts? we are still tracking the variable for it
| "llm_generic_error_record_ids": llm_generic_error_record_ids, | ||
| "success_rate": successful_count / total_tasks if total_tasks > 0 else 0.0, | ||
| "failure_rate": failed_count / total_tasks if total_tasks > 0 else 0.0, | ||
| "total_attempts": attempt_number, |
There was a problem hiding this comment.
we could just keep total attempts? we are still tracking the variable for it
Yes, we still have attempt_number. On the initial run, that's pretty straightforward. However, on reruns, it's not as straightforward. Before this PR, validate_existing() was dropping the total_attempts field and instead adding this:
"total_rerun_attempts": max(len(v) for v in rerun_history.values()) if rerun_history else 0,which is almost the same attempt_number, except in one edge case: if no records needed a rerun at all (pending_ids empty from the start), the for loop was still setting attempt_number = 1 before immediately breaking (runner.py:785-786), but rerun_history stayed empty, so total_rerun_attempts was 0 while attempt_number was 1. Anyway, it was not really a "total", wasn't it?
Ideally, I guess total_attempts would increment from the previous run, but:
- Simply adding
total_attemptswould be a naive "prior max + this run's max," and not a true per-record running total. - A true per-record running total would require saving per-record attempt counts from all previous runs, or listing/parsing many more files, which seems fragile.
- I'm not sure this information is worth this effort. Do you have any use for it that I don't see?
What do you think?
There was a problem hiding this comment.
Yea I guess not a big deal, I'll just merge then
Clean up (trivial)
Remove unused
finished_idsBring
run()(normal run) on par withvalidate_existing()(rerun)Sync
evaluation_summary.jsonbetweenrun()andvalidate_existing()by removing fields that are not present in both (so I guess they were not important, but I'm happy to restore any if you want), and allowing non-ASCII characters (which was only the case invalidate_existing()).Archive the final failing attempts after the last
check_conversation_finished()inrun()like it's done invalidate_existing(). In practice, thischeck_conversation_finished()was not reachable because failed records always had an entry inrerun_history, but that seemed fragile.Simplify categorizing failures in
run()like it's done invalidate_existing().Archive at the beginning of the attempt loop in
run()like it's done invalidate_existing().Always await audio for incomplete conversations
Remove the early-return for incomplete conversations in
_run_and_pipeline(). It returned beforeawait audio_task, leaving the audio task running un-awaited. The pipeline then proceeded without flushing the WAV and any exception went unobserved. This mattered most fortime_limit_exceededrecords, which are later skip-gate validated and can be accepted, so their audio must be written first.Classification is unchanged: an unfinished conversation still fails the
conversation_valid_endgate invalidate_one()and is bucketednot_finished.Write
result.jsonfallback when the worker skips itThe worker writes
result.jsonitself on the happy path, but its post-processing-exception path returns a result without writing, relying on the runner to do it. The worker code includes this comment:But,
run()didn't actually write it, until now.Share code between
run()andvalidate_existing()Extract
run()'s attempt loop into_run()and reuse it invalidate_existing(). This removes duplicated code, andvalidate_existing()gains a few features that onlyrun()had:run_validation()pass.Add run_id to progress bar description so we can differentiate multiple runs launched at once.