Skip to content

fix(perf): improve metric accuracy, validation, and shutdown - #1654

Merged
Yunnglin merged 1 commit into
modelscope:mainfrom
git-jxj:fix/perf-accuracy-validation
Aug 28, 2026
Merged

fix(perf): improve metric accuracy, validation, and shutdown#1654
Yunnglin merged 1 commit into
modelscope:mainfrom
git-jxj:fix/perf-accuracy-validation

Conversation

@git-jxj

@git-jxj git-jxj commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes several correctness, validation, and lifecycle issues in the perf benchmark path:

  • Percentiles were biased one rank high for every p1-p99 result.
  • Failed requests could either pin wall time to the clock epoch or omit their real elapsed time, producing invalid QPS and throughput.
  • Unsupported or invalid argument combinations were accepted until they failed opaquely or produced an empty run.
  • SIGINT/SIGTERM stopped the event loop mid-flight or leaked CancelledError tracebacks after cleanup.
  • Runs without a visualizer still built and dispatched a metrics snapshot for every request.

The changes are covered by deterministic unit, local-server, process-level CLI, and regression tests.

Root Cause

The percentile helper used int(n * p / 100) as a zero-based index instead of nearest-rank's one-based ceil(n * p / 100) - 1. The initial correction also removed the legacy None-to-NaN fallback, which could break reports produced from incomplete metric data.

Failure records either kept the dataclass defaults (start_time=completed_time=0) or captured both timestamps only after an exception had already occurred. Some API plugins catch their own failures and return an incomplete BenchmarkData, so fixing only the outer exception path was insufficient.

The perf argument model did not reject combinations that the dispatch strategies cannot execute safely. Signal handling called loop.stop() directly; switching only to task cancellation allowed cleanup to run but still propagated CancelledError through the CLI.

Changes

  • Use nearest-rank percentile indices while preserving missing values as NaN.
  • Capture failed requests from the beginning of request preparation through completion.
  • Normalize incomplete failure records returned directly by API plugins and ignore malformed timing intervals defensively.
  • Reject open-loop multi-turn runs, non-positive closed-loop parallelism, and non-positive log intervals up front.
  • Cancel pending tasks on SIGINT/SIGTERM, allow cleanup to finish, preserve unrelated internal cancellation, and convert only signal-triggered cancellation to conventional CLI exit codes (130/143).
  • Skip per-request visualizer snapshot work when no visualizer is configured.
  • Add regression coverage for metrics, arguments, lifecycle, CLI exit behavior, and the metrics consumer hot path.

Reproduction

On the unmodified base:

percentiles for range(100)             -> {50: 50, 99: 99}
slow failed request + fast success      -> failure duration 0; wall time only covers the success
plugin-returned failed BenchmarkData    -> completed_time remains 0
SIGINT after benchmark cleanup          -> CancelledError traceback, exit code 1
--open-loop with --multi-turn            -> completes with zero requests
--parallel 0 / --log-every-n-query 0     -> late asyncio or division failure
calculate_percentiles([None], ...)       -> TypeError after nearest-rank change

After this change, the percentile result is {50: 49, 99: 98}, failed request lifecycles contribute to the real wall-time window, missing percentile values remain NaN, invalid arguments fail immediately, and Ctrl-C exits quietly with code 130 after cleanup.

Validation

Targeted regression tests:

python -m pytest \
  tests/perf/test_percentile_metrics.py \
  tests/perf/test_wall_time_failures.py \
  tests/perf/test_arguments_validation.py \
  tests/perf/test_async_lifecycle.py \
  tests/perf/test_metrics_consumer.py -q

Result:

56 passed

Adjacent perf regression suite (unit and local mock-server tests only):

239 passed, 2 skipped

CI smoke test:

python -m pytest tests/cli/test_all.py::TestRun::test_ci_lite -q -p no:warnings

Result:

1 passed

test_multi_parallel_sweep remains skipped by its existing environment gate. All current Ruff and repository pre-commit checks passed.

Scope

The changes are limited to perf metrics, validation, request lifecycle handling, signal-to-CLI propagation, and their tests. They do not change request payloads, model generation behavior, benchmark datasets, or non-perf evaluation semantics. The branch is rebased onto the current main and contains one commit.

Use nearest-rank percentiles, validate incompatible perf arguments, and avoid unnecessary visualizer snapshots. Record the full lifecycle of failed requests so wall-time and throughput remain accurate. Cancel pending work on signals, finish cleanup, and convert signal cancellations to conventional CLI exit codes without hiding internal cancellation.
@git-jxj
git-jxj marked this pull request as ready for review August 27, 2026 10:39

@Yunnglin Yunnglin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@Yunnglin
Yunnglin merged commit 3a74173 into modelscope:main Aug 28, 2026
3 checks passed
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.

2 participants