Goodput initial implementation #32
Conversation
There was a problem hiding this comment.
Excellent work, Andy! This POC looks good. I added some small comments as I reviewed to update once the core development is done.
What are the next steps? We can also chat offline. I think it'd be great to get unit testing up for this.
Once you finish doing this work for LLMs, we can look at creating a class for non-LLMs (specifically embeddings/rankings). With the way you coded this, I don't see anything special about the llm_goodput_reporter that couldn't be used for non-LLM models. You may want to try using it with embeddings/rankings and seeing if it works.
| for val, slo in zip(request_metric_values, target_metric_values) | ||
| ): | ||
| good_req_count += 1 | ||
| self._good_req_count = good_req_count |
There was a problem hiding this comment.
I think we should avoid setting return values that will be used later as an attribute within the method. This is very confusing from the reader's perspective because the "count" is hiding inside the method, and it's being used under compute_goodput.
Let's return the counts
def count_good_reqs(self) -> int:
...
return good_req_countand same goes for other methods as well.
There was a problem hiding this comment.
This is what is called a side effect.
In general, we want to avoid those if they are not necessary.
I agree with @nv-hwoo here.
| if all(val < slo | ||
| for val, slo in zip(request_metric_values, target_metric_values) | ||
| ): |
There was a problem hiding this comment.
Could we unroll this? It hurts readability and I don't think there's much gain in fitting this all into the if conditional block.
There was a problem hiding this comment.
+1. Readability and Maintainability are the top priority. We optimize only if we find a bottleneck impacting performance.
genai-perf/genai_perf/parser.py
Outdated
| Parse and check goodput args | ||
| """ | ||
| ''' | ||
| if args.goodput: |
There was a problem hiding this comment.
This comment is functionally a duplication of the code below.
Comments should steer towards information that is not in the code base. The code should be readable enough alongside a meaningful method name, to avoid this level of detail.
It also has the downside of being able to quickly grow stale and become misleading.
genai-perf/genai_perf/parser.py
Outdated
| @@ -733,6 +778,7 @@ def _parse_profile_args(subparsers) -> argparse.ArgumentParser: | |||
| _add_profile_args(profile) | |||
| _add_output_args(profile) | |||
| _add_other_args(profile) | |||
| _add_goodput_args(profile) | |||
There was a problem hiding this comment.
do we foresee more arguments that are going to fall under the goodput general topic?
I am unclear on whether we need to add a new group for this one argument.
There was a problem hiding this comment.
Maybe more options for users to see exactly what metrics contribute most to the low goodput? Something like this. Knowing a goodput number might not be enough information, right?
Just my random thoughts: )
genai-perf/docs/goodput_tutorial.md
Outdated
|
|
||
| docker run -it --net=host --gpus=1 nvcr.io/nvidia/tritonserver:${RELEASE}-py3-sdk | ||
|
|
||
| # Run GenAI-Perf in the container: |
There was a problem hiding this comment.
I would remove any options that are just stating the default values here. Less code to maintain.
|
|
||
| @abstractmethod | ||
| def compute_goodput(self) -> None: | ||
| """Compute the goodput. To be implemented by subclasses.""" |
|
|
||
|
|
||
| class LLMGoodputReporter(GoodputReporter): | ||
| """A subclass to report goodput for language models.""" |
There was a problem hiding this comment.
We know its a subclass from the line above.
| if all(val < slo | ||
| for val, slo in zip(request_metric_values, target_metric_values) | ||
| ): |
There was a problem hiding this comment.
+1. Readability and Maintainability are the top priority. We optimize only if we find a bottleneck impacting performance.
| for val, slo in zip(request_metric_values, target_metric_values) | ||
| ): | ||
| good_req_count += 1 | ||
| self._good_req_count = good_req_count |
There was a problem hiding this comment.
This is what is called a side effect.
In general, we want to avoid those if they are not necessary.
I agree with @nv-hwoo here.
…embeddings usages
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Fantastic work, Andy! Looks good to me. 🚀
Please make sure to confirm the last CI pipeline passed all tests and that running with goodput constraints manually still works for LLM and non-LLM models (since there is no CI test for it yet). Assuming that is all still good, feel free to merge.
GenAI-Perf goodput support implementation
Migrate goodput dev branch to this new repo.