feat: add detector benchmark script#60
Conversation
adaamko
left a comment
There was a problem hiding this comment.
Thanks — the harness itself is sound (warmup, perf_counter, mean/median/p95, JSON output, no new deps), but as submitted it would auto-close #58 while delivering only part of it:
- No peak-memory reporting — explicitly in the issue's task list.
torch.cuda.max_memory_allocated()on GPU (reset before the measured loop) andresource.getrusage(RUSAGE_SELF).ru_maxrsson CPU, added to the result record, would cover it. - No context-length dimension — the built-in case is one short sentence, and there's no built-in way to produce the ~512/2k/8k sweep the issue asks for. Generating synthetic cases at target token lengths (or shipping case files) would keep it reproducible without hand-built JSONL.
- No device in the output — results aren't interpretable across the CPU-vs-GPU axis without recording which device ran.
If you'd rather land this as a first step, change "Closes #58" to "Partially addresses #58" and I'm open to that too — but then the issue stays open.
Nits, non-blocking: with the default --repeats 5, p95 is just the max — consider a higher default or a note in --help; and the script-loading tests might live better in their own tests/test_benchmark_detectors.py.
|
Heads-up: the repo now requires a contribution-rights attestation on PRs (new required CI check). When you push updates, please add this line to your PR description and tick it:
|
610c8bd to
60e4848
Compare
|
Thanks, addressed the requested benchmark coverage:
Verified locally with:
|
adaamko
left a comment
There was a problem hiding this comment.
Thanks for the thorough second round — all three blockers are properly fixed: the CUDA peak-memory reset/read placement is correct, the Darwin/Linux ru_maxrss unit difference is handled, device is resolved and recorded, and the synthetic 512/2k/8k sweep with per-case stats covers the issue.
One small ask before merge: the plain (non---json) output prints only the aggregate summary line, but the per-case latency-vs-context-length numbers are the story #58 is after — please print one line per case too (the "small table" from the issue).
Optional nit, non-blocking: a note in --help that context lengths are counted in whitespace tokens, so the 8192 case exceeds ModernBERT's 4k subword window and partly measures truncation. With the per-case output in, this is good to go.
60e4848 to
6242c64
Compare
|
Added the requested per-case table to the plain-text output and documented that synthetic lengths use whitespace tokens and may measure model truncation. Verified:
|
Summary
Testing
python3 -m pytest tests/test_inference_pytest.py tests/test_benchmark_detectors_pytest.py -v -k "not TestAnswerStartToken"python3 -m ruff format --check --diff lettucedetect/ tests/ scripts/benchmark_detectors.pypython3 -m ruff check lettucedetect/ tests/ scripts/benchmark_detectors.py --extend-exclude lettucedetect/integrations/python3 scripts/benchmark_detectors.py --helpI certify that I have the right to submit this code and that it may be distributed under the repository's MIT license
Closes #58