Repository navigation
test(evaluators): use stepped perf_counter in evaluator duration test - #17816
RogerHYang merged 3 commits into
Conversation
|
The factory recommends ✅ Approve for 4ec1bc1 and labeled this pull request cc @RogerHYang |
There was a problem hiding this comment.
✅ Factory review of 4ec1bc1: Approve
The diff does what it sets out to do, its checks and tests pass, and there are no findings.
| Fit | Checks | Tests | Blocking | Non-blocking | Risk | Depth |
|---|---|---|---|---|---|---|
| ✅ Pass | ✅ Pass | ✅ Pass | ✅ 0 | ✅ 0 | ✅ None | ✅ Focused |
Details
Intent: Stop test_evaluator_run_time_is_observed_whatever_its_result from failing at random on Windows (#17807). The fix imports perf_counter directly into online_eval/executor.py and patches it with a stepped fake clock, so the test no longer sleeps on real asyncio timers. This is the fix a maintainer chose in the issue.
Fit: The executor now uses from time import perf_counter at its two call sites in evaluate_and_annotate, and makes no other change. The test advances the fake clock inside _SlowEvaluator, inside _SlowRaisingEvaluator, and before it cancels the hanging run. The >= assertion is now an exact pytest.approx match. The diff makes no unrelated changes.
Verified: ruff format --check, ruff check and mypy pass on both changed files. tests/unit/server/online_eval/test_consumer.py with --db all gives 139 passed and 2 failed. The changed test passes on SQLite and PostgreSQL. The 2 failures are test_hydration_savepoint_isolates_a_unit_database_error[sqlite] and test_shared_hydration_failure_releases_claims_without_attempts[sqlite], and they also fail on base commit 6584924, so this PR did not cause them. With the base executor.py restored, the changed test fails with AttributeError: ... has no attribute 'perf_counter'. In a scratch worktree, the server reviewer made four changes to the executor, one at a time. The test failed for each one: started_at taken after the evaluator returns, duration observed only on success, the observe moved out of the finally, and the observe skipped on cancellation.
Not verified: The test was not run on Windows, so the fix for the Windows failure is inferred, not observed. The inference is sound because the test no longer depends on wall-clock timers.
Findings without a diff line: none
Resolved threads: none
Risk: side effects none, performance none, backwards compatibility none, from the area reviewers
Graded against the review criteria.
Avoids Windows asyncio.sleep vs time.perf_counter clock tick resolution drift in test_evaluator_run_time_is_observed_whatever_its_result by importing perf_counter in executor.py and stepping a monkeypatched clock inside each test evaluator run. Fixes Arize-ai#17807
024f81f to
266203f
Compare
Fixes #17807
On Windows,
asyncio.sleep(0.05)can wake up slightly early (~48.7–49.96ms) as measured byperf_counter()due to Windows timer/wait quantization, causingtest_evaluator_run_time_is_observed_whatever_its_resultto intermittently failseconds >= 0.05.This imports
from time import perf_counterinsrc/phoenix/server/online_eval/executor.py(matchingsrc/phoenix/db/bulk_inserter.py) and monkeypatchesexecutor_module.perf_counterintest_evaluator_run_time_is_observed_whatever_its_resultwith a stepped clock:1.0,2.0, and3.0inside_SteppedEvaluator.evaluate,_SteppedRaisingEvaluator.evaluate, and while_HangingEvaluatoris running beforecut_off.cancel(), asserting[call(1.0), call(2.0), call(3.0)]without sleeping on wall-clock timers.evaluate()is included inONLINE_EVAL_EVALUATOR_DURATION_SECONDS.