Skip to content

fix(detection): honor config concurrency when --concurrent is omitted - #207

Open
rudycelekli wants to merge 1 commit into
uber:mainfrom
rudycelekli:fix/detection-benchmark-concurrent-config
Open

rudycelekli wants to merge 1 commit into
uber:mainfrom
rudycelekli:fix/detection-benchmark-concurrent-config

Conversation

@rudycelekli

Copy link
Copy Markdown

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Related issue: N/A (found by reading the code; reproduced on main at 6b33dff)

What changed?
Default the flag to None and only override the config when it is passed (same pattern main_detector.py uses for its --concurrent).

Why?
Main_benchmark.py main() declares --concurrent with default=10 and always calls config.update_concurrent_tasks(args.concurrent) (main_benchmark.py:1333,1342), so concurrency.max_concurrent_tasks in config_benchmark.yaml is never used. Setting it to 3 and running python main_benchmark.py still runs 10 tasks at once.

How did you test it?

  • Reproduction: new TestMainConcurrency in tests/test_main_benchmark.py (config file with max_concurrent_tasks: 3, BenchmarkRunner stubbed). main: test_config_concurrency_used_without_flag fails assert 10 == 3; branch: both pass.
  • Test suite: pytest tests/ -q -> 258 passed (main 256 + 2 new).
  • Not tested: a live benchmark run. Note: a custom config with no concurrency section now falls back to Config.max_concurrent_tasks's default (3) instead of the old hard-coded 10.

Potential risks
The change only affects the input described above; other paths go through the same code as before, and the component's existing tests pass.

AI assistance: Claude Code (model Claude Opus 5.5) found this bug, wrote the fix and test, and drafted this description; submitted by @rudycelekli.

🤖 Generated with Claude Code

main_benchmark.py gave --concurrent a default of 10 and always wrote it
into the config, so concurrency.max_concurrent_tasks in
config_benchmark.yaml was never used. Default the flag to None and only
override the config when the flag is passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

This branch has not been deployed

No deployments
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