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

Changes from 3 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
79 changes: 72 additions & 7 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,79 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

import io
import sys

import pytest
from genai_pa.main import run


# NOTE: Placeholder
class TestHelp:
@pytest.mark.parametrize("arg", ["-h", "--help"])
def test_help(self, arg):
args = [arg]
with pytest.raises(SystemExit):
run(args)
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):
combined_args = ["-m", "test_model"] + arg

with pytest.raises(SystemExit) as exc_info:
run(combined_args)

# Confirm the exit was successful
assert exc_info.value.code == 0

# Confirm the message is correct
captured = capsys.readouterr()
assert expected_output in captured.out

@pytest.mark.parametrize(
"arg, expected_output",
[
(["-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=PosixPath('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.
# (["--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_output, capsys):
combined_args = ["-m", "test_model"] + arg
# Redirect stdout and stderr to capture output
original_stdout, original_stderr = sys.stdout, sys.stderr
sys.stdout, sys.stderr = io.StringIO(), io.StringIO()

try:
# Call the run function
run(combined_args)
finally:
# Restore stdout and stderr
captured_stdout, captured_stderr = (
sys.stdout.getvalue(),
sys.stderr.getvalue(),
)
sys.stdout, sys.stderr = original_stdout, original_stderr
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.

Nice use of parameterize!

I think this handling is fine for getting a "speed-of-light" solution out, but longer term, it may be more maintainable and easy to understand to use the parser directly for this kind of test (assuming I'm interpreting the test correctly).

I haven't tested this would actually work, but I'm thinking something like this:

    from genai_pa import parser
    combined_args = ["-m", "test_model"] + arg
    args = parser.parse_args(combined_args)
    # ... operate checks/assertions on the args object here

Rather than working around stdout/redirecting/parsing it, etc.

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.

Note, all the run() function is doing is calling:

args = parser.parse_args(argv)
args.func()

so if you're just looking for input/parser validation and don't care about running the PA/subcommand for this test, I think this would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are confident that the parser is correct, that should work. Just let the parser do the creation of the ArgParser object for you.
I think you just need to pass in a list of comma separated strings to represent the sys args.

Copy link
Contributor

Choose a reason for hiding this comment

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

To summarize, I'd recommend trying to operate on the parser directly in python objects rather than parsing stdout if it makes sense to do so for this test -- but I wouldn't block the PR approval if you don't think it's worth the time to try right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Let me give that a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! It looks much cleaner, thanks for the feedback.


assert expected_output in captured_stdout
assert "" == captured_stderr # Assuming no error, adjust if needed

def test_arguments_model_not_provided(self):
with pytest.raises(SystemExit) as exc_info:
run()
assert exc_info.value.code != 0
Loading