Skip to content

Conversation

@AsadShahid04
Copy link

@AsadShahid04 AsadShahid04 commented Nov 11, 2025

Summary

This PR fixes a bug in benchmarks/llm/perf.sh where the script was overwriting benchmark results instead of creating separate subdirectories for each concurrency level, preventing plot_pareto.py from generating Pareto plots.

Problem

When running perf.sh with multiple concurrency levels (e.g., --concurrency 1,2,4,8), the script used the same artifact_dir for all concurrency levels, causing AIPerf to overwrite results. The plot_pareto.py script expects subdirectories named -concurrency<number>/ to parse concurrency levels from file paths.

Solution

Modified the script to:

  1. Create a unique subdirectory for each concurrency level: -concurrency1/, -concurrency2/, etc.
  2. Pass this subdirectory to AIPerf's --artifact-dir flag instead of the base artifact_dir

Changes

  • Added code to create concurrency_dir="${artifact_dir}/-concurrency${concurrency}" for each iteration
  • Changed --artifact-dir ${artifact_dir} to --artifact-dir ${concurrency_dir}

Testing

The fix ensures:

  • Each concurrency level gets its own subdirectory
  • plot_pareto.py can correctly find and parse results from all concurrency levels
  • The documented workflow in benchmarks/llm/README.md works end-to-end

Directory Structure

Before (broken):

artifacts_0/
  └── profile_export_aiperf.json  (overwritten for each concurrency)

After (fixed):

artifacts_0/
  ├── -concurrency1/
  │   └── profile_export_aiperf.json
  ├── -concurrency2/
  │   └── profile_export_aiperf.json
  ├── -concurrency4/
  │   └── profile_export_aiperf.json
  └── -concurrency8/
      └── profile_export_aiperf.json

Fixes #4233

@hhzhang16 @athreesh

Summary by CodeRabbit

  • Chores
    • Enhanced organization of benchmark artifacts by implementing concurrency-specific subdirectories, enabling improved management and isolation of performance test outputs across different concurrency configurations.

- Create unique subdirectory for each concurrency level (-concurrency1/, -concurrency2/, etc.)
- This prevents AIPerf from overwriting results when running multiple concurrency levels
- Enables plot_pareto.py to correctly parse and generate Pareto plots
- Fixes issue ai-dynamo#4233
@copy-pr-bot
Copy link

copy-pr-bot bot commented Nov 11, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions
Copy link

👋 Hi AsadShahid04! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions bot added the external-contribution Pull request is from an external contributor label Nov 11, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 11, 2025

Walkthrough

The benchmarks/llm/perf.sh script is modified to create per-concurrency subdirectories (named -concurrency<number>) for each benchmark run and pass these directories to aiperf instead of reusing a shared directory, enabling plot_pareto.py to parse concurrency levels and generate Pareto plots.

Changes

Cohort / File(s) Change Summary
Artifact Directory Organization
benchmarks/llm/perf.sh
Introduces per-concurrency subdirectory creation within the main artifact directory. For each concurrency iteration, creates a subdirectory at artifact_dir/-concurrency<concurrency> and passes this directory to the aiperf invocation via --artifact-dir instead of the shared artifact directory.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5–10 minutes

  • Single file with localized, straightforward changes
  • Adds directory creation and path manipulation with no complex logic
  • Follows an established pattern consistent with other scripts in the codebase (e.g., benchmarks/utils/aiperf.py)

Poem

A rabbit hops through benchmark runs,
Creating directories—one for each concurrency sun! ☀️
No more overwrites, results stand proud,
In -concurrency folders, organized and loud. 📁✨
Now Pareto plots bloom and soar,
Performance metrics organized to the core! 🐰📊

Pre-merge checks

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the bug being fixed and references the issue number, accurately summarizing the main change to create per-concurrency subdirectories.
Linked Issues check ✅ Passed The PR addresses all key coding requirements from issue #4233: creating per-concurrency subdirectories with proper naming convention and passing them to AIPerf's --artifact-dir flag.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the specific bug in perf.sh by adding per-concurrency directory creation logic; no extraneous modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description comprehensively covers all required template sections: overview of the bug fix, detailed problem statement, solution explanation, specific changes made, testing validation, and directory structure comparison.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@AsadShahid04
Copy link
Author

pareto_plot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: perf.sh doesn't create per-concurrency subdirectories required by plot_pareto.py

1 participant