Skip to content

Update concurrency handling in benchmark execution - #136

Merged
barisozbas merged 1 commit into
uber:mainfrom
MaddipatlaChetan24:patch-2
Sep 23, 2026
Merged

barisozbas merged 1 commit into
uber:mainfrom
MaddipatlaChetan24:patch-2

Conversation

@MaddipatlaChetan24

Copy link
Copy Markdown
Contributor

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

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

Related issue: N/A

What changed?

_generate_summary computed concurrency_efficiency and reported max_concurrent_tasks using self.config.max_concurrent_tasks directly. _execute_tasks_concurrently forces max_concurrent = 1 for AgentDojo runs regardless of config, so the summary was reading a value that didn't match what was actually used. _execute_tasks_concurrently now returns (results, max_concurrent); run_benchmark threads that through; _generate_summary takes an actual_max_concurrent param and uses it instead of re-reading config.

Why?

For every AgentDojo run, concurrency_efficiency was computed as actual_speedup / config_value (e.g. /10) instead of actual_speedup / 1, understating efficiency by roughly the configured concurrency factor. The reported max_concurrent_tasks field was also just wrong for those runs.

How did you test it?

Traced the value through both call paths (ADR-bench vs. AgentDojo) by reading the code; not run against a live benchmark. No unit test added — this file has no existing test scaffolding to attach one to.

Potential risks

_execute_tasks_concurrently's return signature changed from List[Dict[str, Any]] to Tuple[List[Dict[str, Any]], int]. Any other caller of that method (none visible in this file) would break. _generate_summary gained a new optional parameter with a safe fallback (self.config.max_concurrent_tasks when None), so existing callers not passing it still work, just with the old (wrong-for-AgentDojo) behavior.

@barisozbas barisozbas 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!

@barisozbas
barisozbas merged commit f435b7a into uber:main Sep 23, 2026
8 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