Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CLI unit tests for GenAI-PA CLI #479

Merged
merged 10 commits into from
Feb 29, 2024
Merged

Add CLI unit tests for GenAI-PA CLI #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a32e0a
Add unit tests for CLI
dyastremsky Feb 28, 2024
1d14134
Update comment
dyastremsky Feb 28, 2024
8c17b75
Remove unnecessary check
dyastremsky Feb 28, 2024
394731d
For help test, do not need model name
dyastremsky Feb 29, 2024
26f976a
Add pytest as req, use parser for unit tests
dyastremsky Feb 29, 2024
2064515
Show discarded return value
dyastremsky Feb 29, 2024
e7b934e
Remove unused imports
dyastremsky Feb 29, 2024
655a506
Check correct values for args, nothing printed
dyastremsky Feb 29, 2024
6a00e88
Remove extra print in CLI
dyastremsky Feb 29, 2024
733d6bc
Remove exrta arg
dyastremsky Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/c++/perf_analyzer/genai-pa/genai_pa/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ def prune_args(args: argparse.ArgumentParser) -> argparse.ArgumentParser:
"""
Prune the parsed arguments to remove args with None or False values.
"""
print(args)
debermudez marked this conversation as resolved.
Show resolved Hide resolved
return argparse.Namespace(
**{k: v for k, v in vars(args).items() if v is not None if v is not False}
)
Expand Down
1 change: 1 addition & 0 deletions src/c++/perf_analyzer/genai-pa/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ keywords = []
requires-python = ">=3.8,<4"
dependencies = [
"numpy",
"pytest",
"rich"
]

Expand Down
72 changes: 64 additions & 8 deletions src/c++/perf_analyzer/genai-pa/tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,70 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

from pathlib import Path

import pytest
debermudez marked this conversation as resolved.
Show resolved Hide resolved
from genai_pa.main import run
from genai_pa import parser


class TestCLIArguments:
@pytest.mark.parametrize(
"arg, expected_output",
[
(["-h"], "CLI to profile LLMs and Generative AI models with Perf Analyzer"),
(
["--help"],
"CLI to profile LLMs and Generative AI models with Perf Analyzer",
),
],
)
def test_help_arguments_output_and_exit(self, arg, expected_output, capsys):
with pytest.raises(SystemExit) as excinfo:
_ = parser.parse_args(arg)

# Check that the exit was successful
assert excinfo.value.code == 0

# Capture that the correct message was displayed
captured = capsys.readouterr()
assert expected_output in captured.out

@pytest.mark.parametrize(
"arg, expected_attributes",
[
(["-b", "2"], {"batch_size": 2}),
(["--batch-size", "2"], {"batch_size": 2}),
(["--concurrency", "3"], {"concurrency_range": "3"}),
(["--max-threads", "4"], {"max_threads": 4}),
(
["--profile-export-file", "text.txt"],
{"profile_export_file": Path("text.txt")},
),
(["--request-rate", "1.5"], {"request_rate_range": "1.5"}),
(["--service-kind", "triton"], {"service_kind": "triton"}),
(["--service-kind", "openai"], {"service_kind": "openai"}),
# TODO: Remove streaming from implementation. It is invalid with HTTP.
debermudez marked this conversation as resolved.
Show resolved Hide resolved
# (["--streaming"], {"streaming": True}),
(["--version"], {"version": True}),
(["-u", "test_url"], {"u": "test_url"}),
(["--url", "test_url"], {"u": "test_url"}),
],
)
def test_arguments_output(self, arg, expected_attributes, capsys):
combined_args = ["--model", "test_model"] + arg
args = parser.parse_args(combined_args)

# Check that the attributes are set correctly
for key, value in expected_attributes.items():
assert getattr(args, key) == value
Comment on lines +78 to +82
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is exactly what I was thinking of - nice!!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I think some of the expected attributes may change if Elias uses dest like parser.add_argument("--batch-size", ..., dest="b") in a later PR, so these parameters may have to change to {"b": 2} etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to guard against that with metavar but its a good call out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, metavar only modifies the --help output, but won't change the attributes:

import argparse
parser = argparse.ArgumentParser()
parser.add_argument("-b", "--batch-size", default=1, dest="b", metavar="batch_size")
args = parser.parse_args()

for attr in ["b", "batch_size"]:
    print(f'{attr=}: {hasattr(args, attr)=}')
$ python test.py 
attr='b': hasattr(args, attr)=True
attr='batch_size': hasattr(args, attr)=False

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling this out and expounding on how metavar works, Ryan! We'll have these tests now, so we should make sure they are still passing after any implementation changes. If not, we can update the tests to match changes in how we want to the CLI designed.

Copy link
Contributor

@rmccorm4 rmccorm4 Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's moot since it looks like the current code isn't using metavar for --batch-size and is instead just mapping it later on before calling subprocess. Just mentioning it in case you got unexpected test failures like this later after a dest change.

dest="u" is being used for --url, but looks like David accounted for that already:

            (["--url", "test_url"], {"u": "test_url"}),


# Check that nothing was printed as a byproduct of parsing the arguments
captured = capsys.readouterr()
debermudez marked this conversation as resolved.
Show resolved Hide resolved
assert captured.out == ""

def test_arguments_model_not_provided(self):
with pytest.raises(SystemExit) as exc_info:
_ = parser.parse_args()

# NOTE: Placeholder
class TestHelp:
@pytest.mark.parametrize("arg", ["-h", "--help"])
def test_help(self, arg):
args = [arg]
with pytest.raises(SystemExit):
run(args)
# Check that the exit was unsuccessful
assert exc_info.value.code != 0
Loading