verify flow #484

debermudez · 2024-03-02T00:41:01Z

Clean up
link up llm_inputs
link up llm_profile
rename llm_profile to llm_metrics
rename test_llm_profile to test_llm_metrics
handle extra args after "--" in the command line

src/c++/perf_analyzer/genai-pa/genai_pa/constants.py

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py

rmccorm4 · 2024-03-05T01:23:39Z

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py

+        "-i",
+        type=str.lower,
+        choices=["http", "grpc"],
+        default="http",


This can be a later decision if needed, but my 2cents from similar work is that using a default of GRPC+streaming will work out of box for all triton models when using the infer endpoint, so most users won't even need to think about setting this arg. If you wanted to use a decoupled model in triton on the infer endpoint (which seems like a sensible default for LLM tasks), then you need to use GRPC streaming anyway - so it would be better to not make the user have to set these args everytime for the "common case".

You can also consider selecting the endpoint default values based on the --service-kind wherever it applies.

ex something like this:

triton-infer -> default = grpc streaming - localhost:8001

openai -> default = http - localhost:<common_openai_port>

triton-generate -> default = http (grpc not supported) - localhost:8000

This is something i needed to put more thought into so I am glad you are bringing it up.
I think going based on service-kind is a good method to handle this. Thank you for the suggestion

I will handle this in the next PR if that is okay with you.

rmccorm4 · 2024-03-05T01:26:42Z

src/c++/perf_analyzer/genai-pa/genai_pa/wrapper.py

+        # TODO: Once the OpenAI endpoint support is in place in PA core,
+        # update the input-data option arg
+        # cmd += f"--input-data {DEFAULT_INPUT_DATA_JSON}  -p 10000 -s 99"
+        cmd += f"--input-data ./input_data.json -p 10000 -s 99"


You could also uncomment this code now, and just change DEFAULT_INPUT_DATA_JSON=input_data.json until the other filename workflow is ready

I will handle this in the next PR if that is okay with you.

src/c++/perf_analyzer/genai-pa/genai_pa/llm_metrics.py

src/c++/perf_analyzer/genai-pa/genai_pa/main.py

rmccorm4 · 2024-03-05T19:42:25Z

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

@@ -97,3 +97,10 @@ def test_arguments_model_not_provided(self):
    def test_exception_on_nonzero_exit(self):
        with pytest.raises(GenAiPAException) as e:
            run(["-m", "nonexistent_model"])
+
+    def test_pass_through_args(self):


How does PA handle duplicate arguments? Which should take priority? There should probably be some tests for a handful of conflicting arguments and making sure they behave however you want it to.

genai-perf -m my_model --url localhost:8000 -- -u abc:123 genai-perf -m my_model -- -m your_model genai-perf -m my_model --concurrency 10 -- --request-rate 5 ...

more extensive cli testing is my next task.
i need to find the bugs i dont know about.
that is certainly a test case that needs to be included.

src/c++/perf_analyzer/genai-pa/genai_pa/main.py

rmccorm4 · 2024-03-05T19:58:10Z

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py

    endpoint_group = parser.add_argument_group("Endpoint")

+    endpoint_group.add_argument(


nit for later: PA core looks like it has --endpoint for specifying the HTTP route for openai like /v1/chat/completions

and this tool uses --endpoint as the same as --protocol. We should probably align on names where it makes sense later.

Let me take this as an AI for my next PR.
This was has blown up in scope and I dont want to gate any other submissions too much longer.

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py

src/c++/perf_analyzer/genai-pa/genai_pa/main.py

Clean up and link up llm_inputs

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py

rmccorm4 · 2024-03-06T00:42:50Z

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py

+    if extra_args:
+        # strip off the "--" demarking the pass through arguments
+        extra_args = extra_args[1:]
+        logger.info(f"Additional pass through args: {extra_args}")


Can be covered in follow-up, but I think this will be fragile. If a user makes a typo or something like:

genai-perf --typo my_model -- extra_arg_1

I think this will capture --typo as the first arg here.

There are probably more practical ways to handle this, such as either:

not having any separator, so all extra_args are valid

or having a specific named separator arg to denote separation like --passthrough or something that can be checked through args.passthrough

We can talk more offline or in follow-up though, not a blocker for this PR.

(replace --typo with --mode or --endpoin or --concurency etc)

rmccorm4

LGTM, most opens have been delegated to a follow-up PR.

Make sure to address the codeql unused import warning though, I can re-approve after.

rmccorm4

Nice work! 🚀

* Clean up and link up llm_inputs Clean up and link up llm_inputs * Connect metrics module and silence transformer import warning * Rename test_llm_profile.py to test_metrics.py * Rename again to prefix files with llm * Remove ./ from constant path * Add comment for silencing and handle extra args * Handle pass through arguments * Parse extra args correctly and update testing * Add logging and fix some error checking * extra commit from rebase * Remove unused import

debermudez force-pushed the dbermudez-verify-flow branch 2 times, most recently from 994ac1c to 47e10fe Compare March 5, 2024 01:00

debermudez marked this pull request as ready for review March 5, 2024 01:01

debermudez requested review from dyastremsky and matthewkotila March 5, 2024 01:01

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/constants.py Outdated Show resolved Hide resolved

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py Outdated Show resolved Hide resolved

debermudez force-pushed the dbermudez-verify-flow branch from 52f648c to 432dc3c Compare March 5, 2024 01:20

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/llm_metrics.py Show resolved Hide resolved

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/main.py Show resolved Hide resolved

github-advanced-security bot found potential problems Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/main.py Fixed Show resolved Hide resolved

dyastremsky reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/main.py Fixed Show resolved Hide resolved

src/c++/perf_analyzer/genai-pa/genai_pa/main.py Outdated Show resolved Hide resolved

debermudez requested a review from rmccorm4 March 5, 2024 18:37

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/main.py Show resolved Hide resolved

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/main.py Outdated Show resolved Hide resolved

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py Outdated Show resolved Hide resolved

dyastremsky reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py Show resolved Hide resolved

rmccorm4 reviewed Mar 5, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/main.py Outdated Show resolved Hide resolved

debermudez added 8 commits March 5, 2024 16:18

Clean up and link up llm_inputs

b2128a6

Clean up and link up llm_inputs

Connect metrics module and silence transformer import warning

ba657a5

Rename test_llm_profile.py to test_metrics.py

af0d218

Rename again to prefix files with llm

a0b29d1

Remove ./ from constant path

98548fe

Add comment for silencing and handle extra args

921058d

Handle pass through arguments

701e75a

Parse extra args correctly and update testing

35c7728

debermudez added 2 commits March 5, 2024 16:18

Add logging and fix some error checking

a0de947

extra commit from rebase

49fb648

debermudez force-pushed the dbermudez-verify-flow branch from baf41a5 to 49fb648 Compare March 6, 2024 00:21

github-advanced-security bot found potential problems Mar 6, 2024

View reviewed changes

src/c++/perf_analyzer/genai-pa/genai_pa/parser.py Fixed Show resolved Hide resolved

rmccorm4 reviewed Mar 6, 2024

View reviewed changes

rmccorm4 approved these changes Mar 6, 2024

View reviewed changes

Remove unused import

a03ccbe

rmccorm4 approved these changes Mar 6, 2024

View reviewed changes

debermudez merged commit e701b84 into feature-genai-pa Mar 6, 2024
3 checks passed

debermudez deleted the dbermudez-verify-flow branch March 6, 2024 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verify flow #484

verify flow #484

debermudez commented Mar 2, 2024 •

edited

Loading

rmccorm4 Mar 5, 2024 •

edited

Loading

debermudez Mar 5, 2024

debermudez Mar 5, 2024 •

edited

Loading

rmccorm4 Mar 5, 2024

debermudez Mar 5, 2024 •

edited

Loading

rmccorm4 Mar 5, 2024 •

edited

Loading

debermudez Mar 5, 2024

rmccorm4 Mar 5, 2024

debermudez Mar 5, 2024

rmccorm4 Mar 6, 2024 •

edited

Loading

rmccorm4 left a comment

rmccorm4 left a comment

		endpoint_group = parser.add_argument_group("Endpoint")

		endpoint_group.add_argument(

verify flow #484

verify flow #484

Conversation

debermudez commented Mar 2, 2024 • edited Loading

rmccorm4 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

debermudez Mar 5, 2024

Choose a reason for hiding this comment

debermudez Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

rmccorm4 Mar 5, 2024

Choose a reason for hiding this comment

debermudez Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

rmccorm4 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

debermudez Mar 5, 2024

Choose a reason for hiding this comment

rmccorm4 Mar 5, 2024

Choose a reason for hiding this comment

debermudez Mar 5, 2024

Choose a reason for hiding this comment

rmccorm4 Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

rmccorm4 left a comment

Choose a reason for hiding this comment

rmccorm4 left a comment

Choose a reason for hiding this comment

debermudez commented Mar 2, 2024 •

edited

Loading

rmccorm4 Mar 5, 2024 •

edited

Loading

debermudez Mar 5, 2024 •

edited

Loading

debermudez Mar 5, 2024 •

edited

Loading

rmccorm4 Mar 5, 2024 •

edited

Loading

rmccorm4 Mar 6, 2024 •

edited

Loading