Add compare subcommand #623

nv-hwoo · 2024-05-01T08:08:26Z

Add compare subcommand that allows users to generate plots to compare multiple runs.

Users can provide paths to profile export files using --files CLI option as following

genai-perf compare --files [file, ...]

which will generate a set of plots using these runs as well as pre-filled, default yaml configuration file that users can edit to tweak the plotting parameters.

Users can also directly pass the yaml config file to generate customized plots detailed in the yaml file as following

genai-perf compare --config [YAML file]

General workflow of user can be:

first run genai-perf compare --files [file, ...] that gives plots as well as the initial yaml config file
repeatedly run genai-perf compare --config [YAML file] by tweaking plotting parameters such as title, x and y labels, x and y metrics, and etc.

dyastremsky

Great start!

Would you be able to look into what the most Pythonic way of doing this is? It seems to be that having some functionality be under a subcommand and some functionality would not be the generally suggested approach. I'd also want to see if this messes up the help menu.

I think we'd want to start introducing subcommands under GenAi-Perf, but that discussion is more on the design side of things.

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

dyastremsky · 2024-05-01T19:42:46Z

src/c++/perf_analyzer/genai-perf/tests/test_cli.py

@@ -210,9 +210,33 @@ def test_load_level_mutually_exclusive(self, monkeypatch, capsys):
        captured = capsys.readouterr()
        assert expected_output in captured.err

+    def test_compare_mutually_exclusive(self, monkeypatch, capsys):


Would you be able to add sections headers for regular vs compare subcommand, if we're going that route?

Sorry what is sections headers?

Thanks for asking for clarification! By a header, I mean something like a multi-line comment at the start of each test header. Anything that can help visually separate the tests. Something like this: https://github.com/triton-inference-server/triton_cli/blob/58ba74c4156b24541fd1fd6f462fe17f019d48d9/src/triton_cli/parser.py#L101-L103

Added section header.

src/c++/perf_analyzer/genai-perf/genai_perf/main.py

debermudez · 2024-05-01T19:44:51Z

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

+### Handlers ###
+
+
+def handler(args, extra_args):


Suggested change

def handler(args, extra_args):

def profile_handler(args, extra_args):

I don't want to add any more work for you, but if you do make more changes, it would be nice to make this handle_profile to be consistent with Triton CLI and make any integration with it more straightforward.

If we do not do it as part of this ticket, the next person to add handlers should move to this approach to set the standard IMO.

debermudez · 2024-05-01T19:49:11Z

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

@@ -280,7 +287,7 @@ def _add_endpoint_args(parser):
        "-m",
        "--model",
        type=str,
-        required=True,
+        default=None,


This would break the profile workflow if you did not add the check_model_args method?
Am I understanding that correctly?

I want to make sure we did not break any of the unit testing. Is this currently passing?

This is mainly to make compare subcommand work. If this is required, then we cannot do genai-perf compare ... because we are missing -m option (we will need to call it like genai-perf -m <model> compare ...).

The model name is required because we need model names to prep for PA run and that is being done under _check_model_args.

I think this leads naturally to making profile a sub command.

Are you going to do that as a separate ticket?

Do you mean adding a profile subcommand?

I agree. It sounds like that is an active discussion, but we're going to run into these types of issues if we don't make each different use of GenAI-Perf a subcommand. I think if we're going to start coding things to work around or duplicate what our libraries do by default (e.g. the required check), it usually means we're doing something wrong design-wise.

Hyunjae, if we do end up making a separate sub-command and you are the person to implement it, please make sure to create a ticket for that extra work so that your time is being tracked. You should also update the relevant design document, since it sounds like this was out of scope for it.

debermudez · 2024-05-01T19:49:45Z

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

+    """
+    if args.subcommand == "compare":
+        if not args.config and not args.files:
+            parser.error("Either --config or --files option must be specified.")


Suggested change

parser.error("Either --config or --files option must be specified.")

parser.error("Either the --config or --files option must be specified when comparing.")

Updated. The error format already specifies that we are in compare so I don't think we need to highlight "when comparing" again.

Example:

$ genai-perf compare usage: genai-perf compare [-h] [--config CONFIG | -f FILES [FILES ...]] genai-perf compare: error: Either the --config or --files option must be specified.

I agree with this approach, though it looks like "the" was not pushed.

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

debermudez · 2024-05-01T19:51:21Z

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

@@ -431,6 +438,46 @@ def get_extra_inputs_as_dict(args: argparse.Namespace) -> dict:
    return request_inputs


+def _parse_compare_args(subparsers) -> argparse.ArgumentParser:
+    compare = subparsers.add_parser(
+        "compare", help="Generate plots that compare multiple runs."


Suggested change

"compare", help="Generate plots that compare multiple runs."

"compare", help="Generate plots that compare multiple profile runs."

src/c++/perf_analyzer/genai-perf/genai_perf/wrapper.py

nv-hwoo · 2024-05-01T21:44:43Z

Would you be able to look into what the most Pythonic way of doing this is? It seems to be that having some functionality be under a subcommand and some functionality would not be the generally suggested approach. I'd also want to see if this messes up the help menu.

Great point. Yeah I am not really satisfied on how the current command/subcommand is structured, and we should definitely look into this (and I think @debermudez has already started the conversation on this). But I think that effort will require more effort than this current PR. I have TMA-1900 ticket to refactor CLI for this.

dyastremsky

Thanks for addressing the feedback!

dyastremsky · 2024-05-02T17:04:46Z

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

+    """
+    if args.subcommand == "compare":
+        if not args.config and not args.files:
+            parser.error("Either --config or --files option must be specified.")


I agree with this approach, though it looks like "the" was not pushed.

dyastremsky · 2024-05-02T17:08:58Z

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

@@ -280,7 +287,7 @@ def _add_endpoint_args(parser):
        "-m",
        "--model",
        type=str,
-        required=True,
+        default=None,


I agree. It sounds like that is an active discussion, but we're going to run into these types of issues if we don't make each different use of GenAI-Perf a subcommand. I think if we're going to start coding things to work around or duplicate what our libraries do by default (e.g. the required check), it usually means we're doing something wrong design-wise.

Hyunjae, if we do end up making a separate sub-command and you are the person to implement it, please make sure to create a ticket for that extra work so that your time is being tracked. You should also update the relevant design document, since it sounds like this was out of scope for it.

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

@@ -431,6 +438,46 @@ def get_extra_inputs_as_dict(args: argparse.Namespace) -> dict:
    return request_inputs


+def _parse_compare_args(subparsers) -> argparse.ArgumentParser:
+    compare = subparsers.add_parser(
+        "compare", help="Generate plots that compare multiple runs."


dyastremsky · 2024-05-02T17:11:12Z

src/c++/perf_analyzer/genai-perf/genai_perf/parser.py

+### Handlers ###
+
+
+def handler(args, extra_args):


I don't want to add any more work for you, but if you do make more changes, it would be nice to make this handle_profile to be consistent with Triton CLI and make any integration with it more straightforward.

If we do not do it as part of this ticket, the next person to add handlers should move to this approach to set the standard IMO.

src/c++/perf_analyzer/genai-perf/tests/test_cli.py

@@ -210,9 +210,33 @@ def test_load_level_mutually_exclusive(self, monkeypatch, capsys):
        captured = capsys.readouterr()
        assert expected_output in captured.err

+    def test_compare_mutually_exclusive(self, monkeypatch, capsys):


dyastremsky

Very clean code. Great work!

* Move for better visibility * Add compare subparser * Add subcommand compare * Fix test * Add ticket * add --files option and minor fix * Fix tests * Add unit tests * Address feedback * Fix minor error and add section header

* Fix empty response bug * Fix unused variable Fix test Initialize logger to capture logs Add unit test Change to _ instead of removing Check if args.model is not None fix artifact path Support Python 3.8 in GenAI-Perf (#643) Add automation to run unit tests and check code coverage for GenAI-Perf against Python 3.10 (#640) Changes to support Ensemble Top Level Response Caching (#560) Support for fixed number of requests (#633) * first pass. Hardcoded values * Working for concurrency (hardcoded whenever count windows is used for now) * working for req rate as well * Add CLI. Add/fix unit tests * Remove hack. Restore all normal functionality * Refactor thread config into one class. Add more testing * Rename arg to request-count * Fix request rate bug * Update info print * fix corner case * move fixme to a story tag * add assert to avoid corner case * rename variables * self review #1 * copyright changes * add doxygen to functions * Don't allow sweeping over multiple concurrency or request rate with request-count fix test (#637) Support custom artifacts directory and improve default artifacts directory (#636) * Add artifacts dir option and more descriptive profile export filename * Clean up * fix input data path * Add tests * create one to one plot dir for each profile run * change the directory look * add helper method Extend genai perf plots to compare across multiple runs (#635) * Modify PlotManager and plots classes * Support plots for multiple runs -draft * Fix default plot visualization * Remove artifact * Set default compare directory * Support generating parquet files * Remove annotations and fix heatmap * Fix errors * Fix pre-commit * Fix CodeQL warning * Remove unused comments * remove x axis tick label for boxplot * Add logging and label for heatmap subplots * Allow users to adjust width and height * fix grammer --------- Co-authored-by: Hyunjae Woo <[email protected]> Generate plot configurations for plot manager (#632) * Introduce PlotConfig and PlotConfigParser class * Port preprocessing steps and introduce ProfileRunData * Create plot configs for default plots * fix minor bug * Fix comment * Implement parse method in PlotConfigParser * refactor * fix test * Add test * Address feedback * Handle custom endpoint Add more metadata to profile export JSON file (#627) * Add more metadata to profile export data * Fix minor bug * refactor Add compare subcommand (#623) * Move for better visibility * Add compare subparser * Add subcommand compare * Fix test * Add ticket * add --files option and minor fix * Fix tests * Add unit tests * Address feedback * Fix minor error and add section header Revert "Changes to support Ensemble Top Level Response Caching (#560) (#642)" This reverts commit cc6a3b2. Changes to support Ensemble Top Level Response Caching (#560) (#642)

* Move for better visibility * Add compare subparser * Add subcommand compare * Fix test * Add ticket * add --files option and minor fix * Fix tests * Add unit tests * Address feedback * Fix minor error and add section header

nv-hwoo added 8 commits April 30, 2024 13:30

Move for better visibility

1b96d72

Add compare subparser

7d8123f

Add subcommand compare

f2534bd

Fix test

1118cc0

Add ticket

56426e1

add --files option and minor fix

4b48cb4

Fix tests

67e6d0a

Add unit tests

3da5255

nv-hwoo requested review from debermudez, dyastremsky and lkomali May 1, 2024 08:08

dyastremsky reviewed May 1, 2024

View reviewed changes

debermudez reviewed May 1, 2024

View reviewed changes

nv-hwoo added 2 commits May 1, 2024 16:10

Address feedback

70a7e9b

Fix minor error and add section header

d63ff97

nv-hwoo requested review from dyastremsky and debermudez May 2, 2024 05:04

dyastremsky reviewed May 2, 2024

View reviewed changes

dyastremsky approved these changes May 2, 2024

View reviewed changes

debermudez approved these changes May 2, 2024

View reviewed changes

nv-hwoo merged commit 02fafb1 into compare-subcommand May 2, 2024
3 checks passed

nv-hwoo deleted the hwoo-add-compare-subcommand branch May 2, 2024 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compare subcommand #623

Add compare subcommand #623

nv-hwoo commented May 1, 2024

dyastremsky left a comment

dyastremsky May 1, 2024

nv-hwoo May 1, 2024

dyastremsky May 1, 2024 •

edited

Loading

nv-hwoo May 2, 2024

This comment was marked as outdated.

debermudez May 1, 2024

nv-hwoo May 1, 2024

dyastremsky May 2, 2024

debermudez May 1, 2024

debermudez May 1, 2024

nv-hwoo May 1, 2024

debermudez May 1, 2024

debermudez May 1, 2024

nv-hwoo May 2, 2024

dyastremsky May 2, 2024

debermudez May 1, 2024

nv-hwoo May 1, 2024

dyastremsky May 2, 2024

debermudez May 1, 2024

nv-hwoo May 1, 2024

This comment was marked as outdated.

nv-hwoo commented May 1, 2024

dyastremsky left a comment

dyastremsky May 2, 2024

dyastremsky May 2, 2024

This comment was marked as outdated.

dyastremsky May 2, 2024

This comment was marked as outdated.

dyastremsky left a comment

	def handler(args, extra_args):
	def profile_handler(args, extra_args):

	parser.error("Either --config or --files option must be specified.")
	parser.error("Either the --config or --files option must be specified when comparing.")

	"compare", help="Generate plots that compare multiple runs."
	"compare", help="Generate plots that compare multiple profile runs."

Add compare subcommand #623

Add compare subcommand #623

Conversation

nv-hwoo commented May 1, 2024

dyastremsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyastremsky May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

nv-hwoo commented May 1, 2024

dyastremsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

dyastremsky left a comment

Choose a reason for hiding this comment

dyastremsky May 1, 2024 •

edited

Loading