docs: Benchmarking guide interpreting results - #701
Conversation
kthui
added
documentation
3ae3c30 to
6fd5d23
Compare
417e355 to
fe19277
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
benchmarks/llm/perf.sh (1)
1-1:⚠️ Potential issueBroken she-bang – script may not be executed with Bash
#/bin/bashis missing the exclamation mark.
Use a portable she-bang (#!/usr/bin/env bash) to guarantee the script is run with Bash instead of/bin/sh.-#/bin/bash +#!/usr/bin/env bash🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Use #!, not just #, for the shebang.
(SC1113)
♻️ Duplicate comments (2)
examples/llm/benchmarks/README.md (1)
166-175: Same crash / grammar issues duplicatedReplicate fixes from single-node section (
--artifacts-root-dir, plural “options”, missing comma).🧰 Tools
🪛 LanguageTool
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ... careful in specifying these options inperf.shscript. They should closely ref...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...(AI_HYDRA_LEO_MISSING_COMMA)
examples/llm/benchmarks/plot_pareto.py (1)
60-75:⚠️ Potential issue
parse_gpusbreaks on missing/ string values & contains deadelseMultiplying
Noneor string JSON values raisesTypeError; the unconditionalelsemasks unknown modes.- if deployment_config.get("mode") == "aggregated": - return deployment_config.get("tensor_parallelism") * deployment_config.get( - "data_parallelism" - ) - else: - return deployment_config.get( - "prefill_tensor_parallelism" - ) * deployment_config.get("prefill_data_parallelism") + deployment_config.get( - "decode_tensor_parallelism" - ) * deployment_config.get( - "decode_data_parallelism" - ) + mode = deployment_config.get("mode") + try { + if mode == "aggregated": + return int(deployment_config["tensor_parallelism"]) * int(deployment_config["data_parallelism"]) + elif mode == "disaggregated": + return ( + int(deployment_config["prefill_tensor_parallelism"]) * int(deployment_config["prefill_data_parallelism"]) + + int(deployment_config["decode_tensor_parallelism"]) * int(deployment_config["decode_data_parallelism"]) + ) + } except (KeyError, ValueError, TypeError): + print(f"Warning: cannot parse GPU count from {deployment_config_json_path}") + return 0🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 63-74: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🧹 Nitpick comments (6)
benchmarks/llm/perf.sh (1)
131-144: Duplicatemkdir–artifact_diralready createdLines 142-144 recreate the directory that was just proven not to exist.
The secondmkdir -p(line 213) is redundant.-# Create the new artifacts directory -artifact_dir="${artifacts_root_dir}/artifacts_${index}" -mkdir -p "${artifact_dir}" +artifact_dir="${artifacts_root_dir}/artifacts_${index}"examples/llm/benchmarks/README.md (2)
108-109: Grammar: “these option” → “these options”Minor doc polish.
-... learn more about these option. +... learn more about these options.🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: You might be missing the article “the” here.
Context: ... careful in specifying these options inperf.shscript. They should closely ref...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
261-265: Spelling: “Iterpreting” → “Interpreting”Header typo.
-## Iterpreting Results +## Interpreting Resultsexamples/llm/benchmarks/plot_pareto.py (3)
159-176:pareto_efficientmutates outerdfinside per-group loopWhen multiple groups are processed later, earlier flags can be overwritten.
Safest fix – mark rows viadf.loc[group.index]after computing efficient indices.
218-229: Hard-coded tick intervals make small/large datasets unreadableExpose
--x-interval/--y-intervalCLI options or compute automatically usingMaxNLocator.
243-255: CLI should accept multiple--artifactsdirs instead of a rootMatches earlier doc discussion; simplifies usage and removes globbing assumptions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
benchmarks/llm/perf.sh(3 hunks)examples/llm/benchmarks/README.md(4 hunks)examples/llm/benchmarks/plot_pareto.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
examples/llm/benchmarks/README.md
[uncategorized] ~108-~108: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...and 1 decode worker with TP=4 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~246-~246: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ...ers for two nodes) with TP=4. For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...
(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...
(VARIOUS_DIFFERENT)
[typographical] ~292-~292: Do not use a colon (:) before a series that is introduced by a preposition (‘up’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ement is around <y_diff>/<y_old>x speed up: 
[style] ~292-~292: Using many exclamation marks might seem excessive (in this case: 20 exclamation marks for a text that’s 10692 characters long)
Context: ...t is around <y_diff>/<y_old>x speed up: 
🪛 markdownlint-cli2 (0.17.2)
examples/llm/benchmarks/README.md
239-239: Headings must start at the beginning of the line
null
(MD023, heading-start-left)
🪛 Pylint (3.3.7)
examples/llm/benchmarks/plot_pareto.py
[refactor] 63-74: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 83-83: Too many local variables (19/15)
(R0914)
[refactor] 135-135: Too many local variables (16/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (2)
examples/llm/benchmarks/README.md (1)
231-244:⚠️ Potential issuePlotting section outdated –
plot_pareto.pyexpects--artifacts-root-dirRunning the script without the required argument throws
argparseerror.-python3 /workspace/examples/llm/benchmarks/plot_pareto.py +python3 /workspace/examples/llm/benchmarks/plot_pareto.py \ + --artifacts-root-dir artifacts_rootLikely an incorrect or invalid review comment.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
239-239: Headings must start at the beginning of the line
null(MD023, heading-start-left)
examples/llm/benchmarks/plot_pareto.py (1)
90-99: Metric key is stale for GenAI-Perf ≥ 0.0.13
output_token_throughput_per_userwas renamed to"Output token throughput (per sec)".
Fallback to the new key for forward compatibility.-output_token_throughput_per_user = data.get( - "output_token_throughput_per_user", {} -).get(stat_value) +output_token_throughput_per_user = ( + data.get("Output token throughput (per sec)", {}).get(stat_value) + or data.get("output_token_throughput_per_user", {}).get(stat_value) +)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
benchmarks/llm/plot_pareto.py (1)
238-276:⚠️ Potential issueShebang but file not executable – CI fails
plot_pareto.pyhas#!/usr/bin/env python3yet the file mode is not executable, causing the “Pre Merge Validation” failure.Either:
- Make it executable in Git (
chmod +x benchmarks/llm/plot_pareto.py), or- Drop the shebang if users will always call
python3 script.py.Pick one to unblock the pipeline.
♻️ Duplicate comments (1)
examples/llm/benchmarks/README.md (1)
166-175: Same grammar issue appears – keep docs consistentThe “Important” block repeats the “these option” wording. Please apply the same fix here to avoid drift between sections.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ... careful in specifying these options inperf.shscript. They should closely ref...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...(AI_HYDRA_LEO_MISSING_COMMA)
🧹 Nitpick comments (6)
examples/llm/benchmarks/README.md (3)
101-110: Fix grammar + clarify option names in the Important call-outThere are two minor wording slips:
- “these option” → “these options”
- Missing article before perf.sh script.
While you are touching the paragraph, consider shortening the sentence—keeping only the what and why makes it easier to scan.
- > We should be careful in specifying these options in `perf.sh` script. They should closely reflect the deployment config that is being benchmarked. + > Be careful when specifying **these options** in the *perf.sh* script; they must exactly reflect the deployment configuration being benchmarked.🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: You might be missing the article “the” here.
Context: ... careful in specifying these options inperf.shscript. They should closely ref...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...and 1 decode worker with TP=4 For more information see [Collecting Performance Numbers](#c...(AI_HYDRA_LEO_MISSING_COMMA)
252-255: Redundant phrase “various different” – tighten languageThe sentence contains “various different” twice; “various” is enough.
- … at various different request concurrencies … + … at various request concurrencies …Similarly for “various different deployments”.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...(VARIOUS_DIFFERENT)
291-299: Colon misuse before inline imageA colon directly before the image breaks Markdown linting (MD042 / RP_COLON). Just end the sentence or move the image to a new line.
- speed up:  + speed-up + benchmarks/llm/plot_pareto.py (3)
63-74: Unnecessaryelseafterreturn(pylint R1705)After the first
return, theelseis superfluous. Removing it shortens indentation and improves readability.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 63-74: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
159-176:pareto_efficientmutates outerdf– hidden side-effectThe helper both computes the frontier and mutates
dfviadf.at[…].
Side-effects inside an inner loop are easy to miss and make future refactors risky.Return the set of efficient indices and do the mutation in the caller:
- pareto_points.append(point) - df.at[point_id, "is_pareto_efficient"] = True + pareto_points.append((point_id, point)) ... - return np.array(pareto_points) + return pareto_pointsThis keeps the function pure and the outer loop explicit.
218-229: Fixed tick interval (5) limits generalityHard-coding
x_interval = y_interval = 5works for specific data ranges but crops or bunches larger datasets.Expose these as optional CLI arguments with sensible defaults:
- x_interval = 5 - y_interval = 5 + x_interval = args.x_tick or 5 + y_interval = args.y_tick or 5Add the parameters to
argparseand document them in README.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/llm/benchmarks/example_plots/single_node_pareto_plot.pngis excluded by!**/*.pngexamples/llm/benchmarks/example_plots/two_node_pareto_plot.pngis excluded by!**/*.png
📒 Files selected for processing (2)
benchmarks/llm/plot_pareto.py(1 hunks)examples/llm/benchmarks/README.md(4 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
benchmarks/llm/plot_pareto.py
[refactor] 63-74: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 83-83: Too many local variables (19/15)
(R0914)
[refactor] 135-135: Too many local variables (16/15)
(R0914)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/701/merge) by tanmayv25.
benchmarks/llm/plot_pareto.py
[error] 1-1: File has a shebang but is not marked executable. Run 'chmod +x benchmarks/llm/plot_pareto.py' to fix.
🪛 LanguageTool
examples/llm/benchmarks/README.md
[uncategorized] ~108-~108: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...and 1 decode worker with TP=4 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~246-~246: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ... TP=4(or TP=8 for two nodes). For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...
(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...
(VARIOUS_DIFFERENT)
[typographical] ~301-~301: Do not use a colon (:) before a series that is introduced by a preposition (‘up’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ement is around <y_diff>/<y_old>x speed up: 
[style] ~301-~301: Using many exclamation marks might seem excessive (in this case: 20 exclamation marks for a text that’s 10738 characters long)
Context: ...t is around <y_diff>/<y_old>x speed up: 
🪛 markdownlint-cli2 (0.17.2)
examples/llm/benchmarks/README.md
239-239: Headings must start at the beginning of the line
null
(MD023, heading-start-left)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
examples/llm/benchmarks/README.md (1)
326-331: 🛠️ Refactor suggestionConsolidate Interpreting Results sections
The second top-level "## Interpreting Results" should be a subsection under the initial interpreting-results section (or merged into it). Consider renaming it to "### Example Comparison" (or similar) and adjusting the anchor link accordingly to maintain proper hierarchy.
♻️ Duplicate comments (1)
examples/llm/benchmarks/README.md (1)
261-263: Remove duplicate misspelled headingThe "## Iterpreting Results" heading is misspelled and duplicates the later "## Interpreting Results". Please remove or merge this section to avoid redundancy.
🧹 Nitpick comments (4)
examples/llm/benchmarks/README.md (4)
101-109: Explicit single-node perf.sh invocation is clear; minor grammar fix neededThe added example clarifies how to collect performance numbers for the disaggregated single-node scenario. Please pluralize "option" to "options" in the help reference.
- See `perf.sh --help` to learn more about these option. + See `perf.sh --help` to learn more about these options.🧰 Tools
🪛 LanguageTool
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
166-174: Explicit multinode perf.sh invocation is clear; minor grammar fix neededThe example for disaggregated multinode nicely demonstrates the command. Please pluralize "option" to "options" in the help text.
- See `perf.sh --help` to learn more about these option. + See `perf.sh --help` to learn more about these options.🧰 Tools
🪛 LanguageTool
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
231-247: Explicit aggregated perf.sh examples improve clarity; minor grammar fix neededThe single-node and two-node aggregated examples are helpful. Please pluralize "option" to "options" in the admonition (and consider adding a comma before "For more information").
- See `perf.sh --help` to learn more about these option. + See `perf.sh --help` to learn more about these options.🧰 Tools
🪛 LanguageTool
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
🪛 markdownlint-cli2 (0.17.2)
239-239: Headings must start at the beginning of the line
null(MD023, heading-start-left)
252-252: Simplify redundant phrasingThe phrase "various different" is redundant. Consider using "various" or "different":
- ...collect the performance numbers at various different request concurrencies. + ...collect the performance numbers at various request concurrencies.🧰 Tools
🪛 LanguageTool
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...(VARIOUS_DIFFERENT)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/llm/benchmarks/README.md(4 hunks)
🧰 Additional context used
🪛 LanguageTool
examples/llm/benchmarks/README.md
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...and 1 decode worker with TP=4 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ... TP=4(or TP=8 for two nodes). For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...
(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...
(VARIOUS_DIFFERENT)
[typographical] ~300-~300: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...s/user, the increase in tokens/s/gpu is 145 - 80 = 65, from the orange baseline to th...
(DASH_RULE)
[typographical] ~301-~301: Do not use a colon (:) before a series that is introduced by a preposition (‘up’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...o the improvement is around 1.44x speed up: 
[style] ~301-~301: Using many exclamation marks might seem excessive (in this case: 20 exclamation marks for a text that’s 10704 characters long)
Context: ...e improvement is around 1.44x speed up: 
🪛 markdownlint-cli2 (0.17.2)
examples/llm/benchmarks/README.md
239-239: Headings must start at the beginning of the line
null
(MD023, heading-start-left)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
examples/llm/benchmarks/README.md (2)
101-110:⚠️ Potential issueAdd mandatory
--artifacts-root-dirflag & fix plural typo.
perf.shnow requires--artifacts-root-dir; running the snippet as-is exits early.
The sentence “these option” should be “these options”.-bash -x /workspace/benchmarks/llm/perf.sh --mode disaggregated --deployment-kind dynamo_vllm --prefill-tensor-parallelism 1 --prefill-data-parallelism 4 --decode-tensor-parallelism 4 --decode-data-parallelism 1 +bash -x /workspace/benchmarks/llm/perf.sh --mode disaggregated --deployment-kind dynamo_vllm \ + --prefill-tensor-parallelism 1 --prefill-data-parallelism 4 \ + --decode-tensor-parallelism 4 --decode-data-parallelism 1 \ + --artifacts-root-dir /path/to/artifacts_root @@ -See `perf.sh --help` to learn more about these option. +See `perf.sh --help` to learn more about these options.🧰 Tools
🪛 LanguageTool
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...and 1 decode worker with TP=4 For more information see [Collecting Performance Numbers](#c...(AI_HYDRA_LEO_MISSING_COMMA)
261-304: 🛠️ Refactor suggestionDuplicate / misspelled top-level heading – collapse into one section.
You currently have:
## Iterpreting Results(misspelled, earlier)## Interpreting Results(later)Keep one correctly spelled heading and remove the duplicate to maintain heading hierarchy.
-## Iterpreting Results +## Interpreting Results ... -## Interpreting Results - -In this section, we are comparing the [Disaggregated Single Node Benchmarking](#disaggregated-single-node-benchmarking) -result to the [vLLM Aggregated Baseline Benchmarking](#vllm-aggregated-baseline-benchmarking) as an example. The multi-node -results can also be compared in similar steps.🧰 Tools
🪛 LanguageTool
[typographical] ~300-~300: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...s/user, the increase in tokens/s/gpu is145 - 80 = 65, from the orange baseline to th...(DASH_RULE)
[typographical] ~301-~301: Do not use a colon (:) before a series that is introduced by a preposition (‘up’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...o the improvement is around 1.44x speed up: 
[style] ~301-~301: Using many exclamation marks might seem excessive (in this case: 20 exclamation marks for a text that’s 10704 characters long)
Context: ...e improvement is around 1.44x speed up: 
benchmarks/llm/plot_pareto.py (2)
48-56:parse_concurrencystill breaks on nested paths – reuse previous fix.The strict
re.findall+ length check raises on legitimate nested folders (run-8-concurrency16). Switch tore.searchand return the first match:-def parse_concurrency(name): - matches = re.findall(r"-concurrency(\d+)", name) - if len(matches) != 1: - raise Exception(f"non-unique matches: {matches}") - concurrency = 0 - for c in matches: - concurrency += int(c) - return concurrency +def parse_concurrency(name: str) -> int: + match = re.search(r"-concurrency(\d+)", name) + if not match: + raise ValueError(f"Cannot determine concurrency from {name}") + return int(match.group(1))
60-74:parse_gpusunsafe when keys are missing (None * int).Add defensive checks and remove the unnecessary
elseafterreturn:- if deployment_config.get("mode") == "aggregated": - return deployment_config.get("tensor_parallelism") * deployment_config.get( - "data_parallelism" - ) - else: - return deployment_config.get( - "prefill_tensor_parallelism" - ) * deployment_config.get("prefill_data_parallelism") + deployment_config.get( - "decode_tensor_parallelism" - ) * deployment_config.get( - "decode_data_parallelism" - ) + if deployment_config.get("mode") == "aggregated": + tp = deployment_config.get("tensor_parallelism") + dp = deployment_config.get("data_parallelism") + if tp is None or dp is None: + raise KeyError("`tensor_parallelism` or `data_parallelism` missing in deployment_config.json") + return tp * dp + + tp_p = deployment_config.get("prefill_tensor_parallelism") + dp_p = deployment_config.get("prefill_data_parallelism") + tp_d = deployment_config.get("decode_tensor_parallelism") + dp_d = deployment_config.get("decode_data_parallelism") + if None in (tp_p, dp_p, tp_d, dp_d): + raise KeyError("Prefill/Decode TP/DP values missing in deployment_config.json") + return tp_p * dp_p + tp_d * dp_d🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 63-74: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🧹 Nitpick comments (2)
examples/llm/benchmarks/README.md (1)
252-255: Remove “various different” redundancy.-... collect the performance numbers at various different request concurrencies. +... collect the performance numbers at various request concurrencies.🧰 Tools
🪛 LanguageTool
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...(VARIOUS_DIFFERENT)
benchmarks/llm/plot_pareto.py (1)
186-190: Legend labels are noisy – drop the “Label ” prefix.The legend currently shows “Label disaggregated_dynamo” etc. Use the label directly:
- label=f"Label {label}", + label=label,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
benchmarks/llm/plot_pareto.py(1 hunks)examples/llm/benchmarks/README.md(4 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
benchmarks/llm/plot_pareto.py
[refactor] 63-74: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 83-83: Too many local variables (19/15)
(R0914)
[refactor] 135-135: Too many local variables (16/15)
(R0914)
🪛 LanguageTool
examples/llm/benchmarks/README.md
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...and 1 decode worker with TP=4 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ... TP=4(or TP=8 for two nodes). For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...
(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...
(VARIOUS_DIFFERENT)
[typographical] ~300-~300: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...s/user, the increase in tokens/s/gpu is 145 - 80 = 65, from the orange baseline to th...
(DASH_RULE)
[typographical] ~301-~301: Do not use a colon (:) before a series that is introduced by a preposition (‘up’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...o the improvement is around 1.44x speed up: 
[style] ~301-~301: Using many exclamation marks might seem excessive (in this case: 20 exclamation marks for a text that’s 10704 characters long)
Context: ...e improvement is around 1.44x speed up: 
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
examples/llm/benchmarks/README.md (1)
231-247: Aggregated baseline example also misses--artifacts-root-dir.Please append it and fix “these option(s)” as above to prevent user error.
🧰 Tools
🪛 LanguageTool
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
examples/llm/benchmarks/README.md (1)
166-175: Please apply the same article and pluralization fixes to the multinode section’s perf.sh note as above.🧰 Tools
🪛 LanguageTool
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ... careful in specifying these options inperf.shscript. They should closely ref...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...(AI_HYDRA_LEO_MISSING_COMMA)
🧹 Nitpick comments (6)
examples/llm/benchmarks/README.md (6)
101-108: Fix grammar and article in perf.sh note
The important note incorrectly refers to “options inperf.shscript” and “these option.” It should add “the” before the script name and pluralize “options.”
Apply this diff:< > We should be careful in specifying these options in `perf.sh` script. See `perf.sh --help` to learn more about these option. > > We should be careful in specifying these options in the `perf.sh` script. See `perf.sh --help` to learn more about these options.🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: You might be missing the article “the” here.
Context: ... careful in specifying these options inperf.shscript. They should closely ref...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
231-248: Fix grammar, spacing, and casing in aggregated perf.sh note
In the aggregated baseline note:
- Add “the” before “
perf.shscript” and pluralize “options.”- Correct the casing of
vLLmtovLLM.- Insert a space before “(or” in the TP value.
Apply this diff:< > We should be careful in specifying these options in `perf.sh` script. See `perf.sh --help` to learn more about these option. In the above command, we described that our deployment is using aggregated serving in `vLLm serve`. We have also accurately described that we have 2 workers with TP=4(or TP=8 for two nodes). > > We should be careful in specifying these options in the `perf.sh` script. See `perf.sh --help` to learn more about these options. In the above command, we described that our deployment is using aggregated serving in `vLLM serve`. We have also accurately described that we have 2 workers with TP=4 (or TP=8 for two nodes).🧰 Tools
🪛 LanguageTool
[uncategorized] ~246-~246: You might be missing the article “the” here.
Context: ... careful in specifying these options inperf.shscript. They should closely ref...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...eeperf.sh --helpto learn more about these option. In the above command, we described tha...(IN_THESE_NN)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ... TP=4(or TP=8 for two nodes). For more information see [Collecting Performance Numbers](#c...(AI_HYDRA_LEO_MISSING_COMMA)
261-264: Correct typo in section heading
The heading “## Iterpreting Results” is misspelled. Update to:- ## Iterpreting Results + ## Interpreting Results
267-270: Pin plotting dependencies for reproducibility
To prevent unexpected version changes, consider pinningmatplotlibandseaborn. For example:- pip3 install matplotlib seaborn + pip3 install 'matplotlib>=3.5' 'seaborn>=0.11'
271-277: Clarify the artifacts root placeholder in single-node example
The placeholderartifacts_rootmay be mistaken for a literal directory. Update the example to indicate it’s a user-supplied path:- python3 /workspace/benchmarks/llm/plot_pareto.py --artifacts-root-dir artifacts_root + python3 /workspace/benchmarks/llm/plot_pareto.py --artifacts-root-dir <artifacts_root>
279-283: Clarify the artifacts root placeholder in two-node example
Similarly, make the placeholder explicit:- python3 /workspace/benchmarks/llm/plot_pareto.py --artifacts-root-dir artifacts_root --title "Two Nodes" + python3 /workspace/benchmarks/llm/plot_pareto.py --artifacts-root-dir <artifacts_root> --title "Two Nodes"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/llm/benchmarks/README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
examples/llm/benchmarks/README.md
[uncategorized] ~108-~108: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~108-~108: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...and 1 decode worker with TP=4 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~173-~173: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...and 1 decode worker with TP=8 For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~246-~246: You might be missing the article “the” here.
Context: ... careful in specifying these options in perf.sh script. They should closely ref...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~246-~246: Did you mean “this option” or “these options”?
Context: ...ee perf.sh --help to learn more about these option. In the above command, we described tha...
(IN_THESE_NN)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ... TP=4(or TP=8 for two nodes). For more information see [Collecting Performance Numbers](#c...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~252-~252: Possible missing article found.
Context: ...ovide this information to the script in form of command line arguments. The benchmar...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...l to collect the performance numbers at various different request concurrencies. The perf.sh scri...
(VARIOUS_DIFFERENT)
[style] ~252-~252: This phrase is redundant. Consider writing “various” or “different”.
Context: ...n multiple times to collect numbers for various different deployments. Each script execution will...
(VARIOUS_DIFFERENT)
[typographical] ~300-~300: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...s/user, the increase in tokens/s/gpu is 145 - 80 = 65, from the orange baseline to th...
(DASH_RULE)
[typographical] ~301-~301: Do not use a colon (:) before a series that is introduced by a preposition (‘up’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...o the improvement is around 1.44x speed up: 
[style] ~301-~301: Using many exclamation marks might seem excessive (in this case: 20 exclamation marks for a text that’s 10391 characters long)
Context: ...e improvement is around 1.44x speed up: 
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
Co-authored-by: Ziqi Fan <ziqif@nvidia.com>
Co-authored-by: Ziqi Fan <ziqif@nvidia.com>
5 participants
Overview:
Add interpreting result section for users to plot the pareto graph and what it means.
Details:
#509 (comment)
Where should the reviewer start?
N/A
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
N/A
Summary by CodeRabbit