refactor(accuracy): re-home results processor as accuracy accumulator#1117
refactor(accuracy): re-home results processor as accuracy accumulator#1117ajcasagrande wants to merge 1 commit into
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@171e52a264351502a85d990f53f4450fc40db378Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@171e52a264351502a85d990f53f4450fc40db378Last updated for commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
26d2ebe to
171e52a
Compare
WalkthroughThe accuracy post-processor class ChangesAccuracyAccumulator rename and re-registration
Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/accuracy/test_accuracy_record_processor.py (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFile name no longer matches its content.
This file exclusively tests
AccuracyAccumulatornow, but is still namedtest_accuracy_record_processor.py, while a distinctAccuracyRecordProcessorclass exists elsewhere in the codebase. This naming collision risks confusing future contributors about which class is under test.♻️ Suggested rename
git mv tests/unit/accuracy/test_accuracy_record_processor.py tests/unit/accuracy/test_accuracy_accumulator.pyAlso applies to: 48-49, 247-248, 277-278
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/accuracy/test_accuracy_record_processor.py` around lines 8 - 9, The test module name no longer matches what it covers: it now tests AccuracyAccumulator only, but is still named as if it targets AccuracyRecordProcessor. Rename the test file to match its actual subject and update the imports/usages in the test module so AccuracyAccumulator is the only referenced class; keep AccuracyRecordProcessor references out of this file since that class is tested elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/accuracy/test_accuracy_record_processor.py`:
- Around line 8-9: The test module name no longer matches what it covers: it now
tests AccuracyAccumulator only, but is still named as if it targets
AccuracyRecordProcessor. Rename the test file to match its actual subject and
update the imports/usages in the test module so AccuracyAccumulator is the only
referenced class; keep AccuracyRecordProcessor references out of this file since
that class is tested elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 89c3aa76-5b98-40be-a2e8-a03c5f5b428a
📒 Files selected for processing (9)
docs/accuracy/accuracy-benchmarking.mddocs/accuracy/accuracy_stubs.mdsrc/aiperf/accuracy/accuracy_accumulator.pysrc/aiperf/dataset/loader/accuracy_dataset_loader.pysrc/aiperf/plugin/enums.pysrc/aiperf/plugin/plugins.yamltests/unit/accuracy/test_accuracy_accumulator_protocol_path.pytests/unit/accuracy/test_accuracy_accumulator_summarize.pytests/unit/accuracy/test_accuracy_record_processor.py
ajcasagrande
left a comment
There was a problem hiding this comment.
I found one correctness issue that should be fixed before merge: the new AccuracyAccumulator is wired into the accumulator pipeline but still behaves like the old all-run results processor, so warmup records are included in final profiling accuracy totals whenever warmup is enabled. The source path confirms the accumulator receives all metric records and stores only global counters, and the runtime reproduction with the mock server produced accuracy_results.csv totals of 2 for a run with one warmup request and one profiling request.
Everything else in the re-home looks directionally sound: the plugin registration moved to accumulator:accuracy_results, the record-type metadata routes it through the metric-record accumulator path, and disabled-accuracy construction still raises PostProcessorDisabled as expected. Fix order: first make accuracy summaries phase/window-aware and add a warmup+profiling regression test through the accumulator/RecordsManager path; then rerun the targeted accuracy tests and CLI repro.
|
|
||
| Conforms to the accumulator summarize contract (``ctx`` is accepted for | ||
| protocol compatibility and unused; this accumulator owns its own state). | ||
|
|
There was a problem hiding this comment.
This loses the phase scoping that the accumulator pipeline relies on. After moving accuracy aggregation from the legacy results-processor path into accumulator:accuracy_results, every warmup and profiling MetricRecordsData is now sent through process_record, but the accumulator keeps only one set of counters and explicitly ignores the SummaryContext. The final RecordsManager path asks for profiling and warmup summaries separately, so an accuracy run with --warmup-request-count 1 --request-count 1 exports Total 2 in accuracy_results.csv for the profiling result. I reproduced that with the real CLI and in-repo mock server, and a direct accumulator repro with one wrong warmup record plus one correct profiling record returns the same mixed count=2/current=0.5 for both SummaryContext(PROFILING) and SummaryContext(WARMUP). Please store enough per-record phase/window data and honor the summary/export context (and make sure the RecordsManager path passes it here) before emitting accuracy MetricResults.
Stacked on #1102 (
ajc/metrics-accumulator-engine) — retargets tomainwhen that merges.What moved and why
AccuracyResultsProcessoris re-homed asAccuracyAccumulator(src/aiperf/accuracy/accuracy_results_processor.py->src/aiperf/accuracy/accuracy_accumulator.py, git rename, 81% similar) and its registration moves fromresults_processor:accuracy_resultstoaccumulator:accuracy_resultswithrecord_types: [metric_records].With #1102's accumulator engine, legacy
results_processorplugins no longer feedProfileResults.records— RecordsManager summarizes only the registered accumulators (_summarize_metric_record_accumulators), so accuracy summaries were dropped from results on the engine path. This slice plugs accuracy into the engine:process_result->process_record(engine per-record dispatch via_send_record_to_accumulators)summarize()gains an optionalctx: SummaryContext | None = Nonefor protocol compatibility (engine's realtime + final paths callsummarize()with no args)on_dataset_configuredkeeps working — RecordsManager already duck-calls it on accumulatorsaccuracy.overall,accuracy.task.*,accuracy.unparsed*), and the self-disable (PostProcessorDisabledwhen accuracy mode is off) are unchangedAlso carried: docstring renames in
accuracy_dataset_loader.py, doc reference updates indocs/accuracy/, and regenerated plugin enum docstrings (src/aiperf/plugin/enums.py).Tests
test_accuracy_results_processor_summarize.py->test_accuracy_accumulator_summarize.py(mechanical)test_accuracy_record_processor.pyfor the new namestest_accuracy_accumulator_protocol_path.py: drives the accumulator through the registered plugin path — registry class resolution, the exactload_accumulatorsconstructor kwargs,record_typesmetadata routing, andsummarize()viagenerate_realtime_metrics(the engine helper) — so seam drift fails tests instead of only crashing at runtimeVerification
uv run pytest -n auto tests/unit/accuracy/ tests/unit/records/ tests/unit/metrics/— 1270 passed, 11 skippeduv run pytest --collect-only -q tests/unit/— 14288 collected, no errorsmake check-ruff-baselined(119 baselined, 0 new),make check-ergonomics(61 baselined, 0 new)make validate-plugin-schemas(235 plugins OK),make generate-all-plugin-files— zero drift🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests