triton-inference-server / client

Triton Python, C++ and Java client libraries, and GRPC-generated client examples for go, java and scala.
BSD 3-Clause "New" or "Revised" License
521 stars 225 forks source link

Unit tests for artifacts produced by GenAI-Perf #677

Closed lkomali closed 3 weeks ago

lkomali commented 1 month ago

This PR is a part of unit test epic for genai-perf and handles unit tests for artifacts produced by genai-perf. https://jirasw.nvidia.com/browse/TPA-83

Folder structure genai-perf | genai-perf | tests | artifacts | file1 | file 2 | plots

Artifacts are created inside genai-perf directory. Plots directory is created inside artifacts.

Ran the test with wrong path (used /genai_perf_artifact instead of /genai_perf_artifacts) to confirm the test fails. image

When given the correct path, the test runs successfully. image

dyastremsky commented 1 month ago

Could you actually break this out into two tests:

You can break this into another PR or ticket, if easier. I forgot that --artifact-dir was added, so we'd want to test both the default and non-default behavior.

lkomali commented 1 month ago

Sure, I will work on your comment @dyastremsky

dyastremsky commented 4 weeks ago

Could you actually break this out into two tests:

  • One not specifying an arg (i.e. it should use the default path)
  • One specifying an arg (i.e. it should use that path)

You can break this into another PR or ticket, if easier. I forgot that --artifact-dir was added, so we'd want to test both the default and non-default behavior.

Discussed offline. Since this function uses the arg passed from the parser, it doesn't make sense to unit test the default path. If we want to unit test the default, we would do that in test_cli.py (though we'd be mostly replicating testing ArgParse defaults) or do an integration/end-to-end test (probably not worth the resources unless we find a low-cost way to run those tests). We should use the artifact directories in some of our end-to-end testing, so hopefully that'd provide some code coverage.