search

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
551 stars 227 forks source link

Add testing for DataLoader #727

Closed AndyDai-nv closed 2 months ago

AndyDai-nv commented 3 months ago

Add one test case for dataloader to improve coverage.

dyastremsky commented 3 months ago

Clean work on this! Appreciate how comprehensive this is. Please don't forget to update the description.

My concern with this approach is that it's not proper unit testing. This is a step in the right direction. However, we already can't parallelize a few of the Perf Analyzer tests successfully, but with a proper unit test, we should be able to. These tests move us further from that goal, because they actually create/delete the files instead of using mocks. That's better than not having these tests and faster to develop. However, this could cause headaches later.

If we're not going to mock the file/dir to avoid having side effects in these tests, I would leave a ticket in the backlog about doing moving these to proper unit tests later. And also just generally moving to a place where these can be run in parallel with one command, similar to what we added to Model Analyzer. That would be a high-value addition.

If we're sticking with this approach, I'd also want to confirm that we have error handling (e.g. setup/teardown or try-catch if that's not possible) that if something goes wrong during the unit test, extra files or directories don't remain on the machine.

CC: @matthewkotila

AndyDai-nv commented 3 months ago

Clean work on this! Appreciate how comprehensive this is. Please don't forget to update the description.

My concern with this approach is that it's not proper unit testing. This is a step in the right direction. However, we already can't parallelize a few of the Perf Analyzer tests successfully, but with a proper unit test, we should be able to. These tests move us further from that goal, because they actually create/delete the files instead of using mocks. That's better than not having these tests and faster to develop. However, this could cause headaches later.

If we're not going to mock the file/dir to avoid having side effects in these tests, I would leave a ticket in the backlog about doing moving these to proper unit tests later. And also just generally moving to a place where these can be run in parallel with one command, similar to what we added to Model Analyzer. That would be a high-value addition.

If we're sticking with this approach, I'd also want to confirm that we have error handling (e.g. setup/teardown or try-catch if that's not possible) that if something goes wrong during the unit test, extra files or directories don't remain on the machine.

CC: @matthewkotila

Thanks, David. Totally understand the concerns. We updated the JIRA ticket to make this just a draft version before changing to a mock way. And this one can be paused for now.

dyastremsky commented 2 months ago

@AndyDai-nv @matthewkotila Would it be possible to add this as a temporary solution? I don't know when we'll get to the JIRA ticket. I suspect it'll end up getting pushed aside for a while due to other work.

matthewkotila commented 2 months ago

@dyastremsky: @AndyDai-nv @matthewkotila Would it be possible to add this as a temporary solution? I don't know when we'll get to the JIRA ticket. I suspect it'll end up getting pushed aside for a while due to other work.

Sure 👍🙏