Skip to content

test(evaluators): use stepped perf_counter in evaluator duration test - #17816

Merged
RogerHYang merged 3 commits into
Arize-ai:version-online-evalsfrom
princejain756:fix/online-eval-perf-counter-test
Nov 11, 2026
Merged

RogerHYang merged 3 commits into
Arize-ai:version-online-evalsfrom
princejain756:fix/online-eval-perf-counter-test

Conversation

@princejain756

@princejain756 princejain756 commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #17807

On Windows, asyncio.sleep(0.05) can wake up slightly early (~48.7–49.96ms) as measured by perf_counter() due to Windows timer/wait quantization, causing test_evaluator_run_time_is_observed_whatever_its_result to intermittently fail seconds >= 0.05.

This imports from time import perf_counter in src/phoenix/server/online_eval/executor.py (matching src/phoenix/db/bulk_inserter.py) and monkeypatches executor_module.perf_counter in test_evaluator_run_time_is_observed_whatever_its_result with a stepped clock:

  • Advances by 1.0, 2.0, and 3.0 inside _SteppedEvaluator.evaluate, _SteppedRaisingEvaluator.evaluate, and while _HangingEvaluator is running before cut_off.cancel(), asserting [call(1.0), call(2.0), call(3.0)] without sleeping on wall-clock timers.
  • Advances the clock on DB sessions (including trace persistence and annotation writes) to verify only evaluate() is included in ONLINE_EVAL_EVALUATOR_DURATION_SECONDS.

@princejain756
princejain756 requested a review from a team as a code owner November 9, 2026 22:38
@github-project-automation github-project-automation Bot moved this to 📘 Todo in phoenix Nov 9, 2026
@github-actions github-actions Bot added the triage issues that need triage label Nov 9, 2026
@RogerHYang RogerHYang added factory-review to be reviewed by the factory and removed triage issues that need triage labels Nov 10, 2026
@arize-phoenix-factory

arize-phoenix-factory Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

The factory recommends ✅ Approve for 4ec1bc1 and labeled this pull request factory-approved. Read the review · session sesn_01CFUsAHG4CJMRCD85zGLNyw

cc @RogerHYang

@arize-phoenix-factory arize-phoenix-factory Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@arize-phoenix-factory arize-phoenix-factory Bot added factory-approved The factory review recommends approving and removed factory-review to be reviewed by the factory labels Nov 10, 2026
@github-project-automation github-project-automation Bot moved this from 📘 Todo to 👍 Approved in phoenix Nov 11, 2026
princejain756 and others added 3 commits November 11, 2026 09:11
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
@RogerHYang
RogerHYang force-pushed the fix/online-eval-perf-counter-test branch from 024f81f to 266203f Compare November 11, 2026 17:18
@RogerHYang
RogerHYang merged commit ba50916 into Arize-ai:version-online-evals Nov 11, 2026
54 checks passed
@github-project-automation github-project-automation Bot moved this from 👍 Approved to ✅ Done in phoenix Nov 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

factory-approved The factory review recommends approving

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

[online-evals] Flaky test_evaluator_run_time_is_observed_whatever_its_result on Windows due to asyncio.sleep vs time.perf_counter clock resolution

2 participants