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.

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

Incorrect results reported for TensorRT-LLM #187

Open
lopuhin opened this issue May 17, 2024 · 5 comments
Open

Incorrect results reported for TensorRT-LLM #187

lopuhin opened this issue May 17, 2024 · 5 comments

Comments

@lopuhin
Copy link

lopuhin commented May 17, 2024

I was able to run float16 and int4 trt-llm benchmarks with mistral 7B on L4 GPU (GCP), the reported performance is 40.96 ± 0.37 t/s in float16 and 166.02 ± 0.52 t/s with int4, which is significantly faster compared to both exllamav2 and vllm with batch size 1 on llama 3 8B (also int4), and also 2x higher than theoretically possible given the memory bandwidth available.

I did some debugging and believe that reported results are incorrect, in terms of number of generated tokens, e.g. after this line

output_tokens = output_ids[0][0].detach().cpu().tolist()[num_input_tokens:]
the output_tokens variable contains only values "2" after a while, and I suspect that generation finishes once the first "2" is encountered, and the rest of "2" are not generated actually. This also matches what happens in the second stage where quality is checked -- there speed is not measured but if it was measured using the same method as used in this repo, it would be even higher. If I compute num_output_tokens as output_tokens.index(2) (which is obviously not a general solution but works for now for mistral), then I get the values which are much closer to vllm and also get same generation speed in the speed test and in the subsequent quality test.

@Anindyadeep
Copy link
Member

Hey, sorry for the late reply, however, upon seeing that, I did cross check, and it is to be noted that the device I am using is an A100 80 GB and also I maxed out the gpu utilization (with higher kv cache), so that is why there is so much high throughput.

Speaking of the code, I also printed out the line, and it seems fine, otherwise, the output that you see here would have been very different from ground truth.

@lopuhin
Copy link
Author

lopuhin commented Jun 3, 2024

Hey, thanks for looking into this and no worries. And btw thanks for a really nicely reproducible code, it was super helpful, as trt-llm is showing really good scaling with larger batch sizes for us.

I think there is a way to see that there is something strange going on, without any code changes -- to observe how reported generation speed changes with different prompts, which have different generation lengths.

Using only int4 models for simplicity, L4 GPU:

$ ./bench_tensorrtllm/bench.sh -d cuda -n mistral -r 5

(repo) 2024-06-03 10:42:20,290 - INFO - mistral-Nvidia-TRT-LLM (token/sec), int4: 248.57 ± 8.80
(fixed) 2024-06-03 10:37:06,271 - INFO - mistral-Nvidia-TRT-LLM (token/sec), int4: 59.04 ± 2.87

$ ./bench_tensorrtllm/bench.sh -d cuda -n mistral -r 5 -p "just say hi and nothing else, ok?"

(repo) 2024-06-03 10:43:02,444 - INFO - mistral-Nvidia-TRT-LLM (token/sec), int4: 7617.51 ± 2783.96
(fixed) 2024-06-03 10:39:22,647 - INFO - mistral-Nvidia-TRT-LLM (token/sec), int4: 45.19 ± 16.50

$ ./bench_tensorrtllm/bench.sh -d cuda -n mistral -r 5 -p "write a super long essey about transformers, never stop"

(repo) 2024-06-03 10:44:19,333 - INFO - mistral-Nvidia-TRT-LLM (token/sec), int4: 176.23 ± 4.39
(fixed) 2024-06-03 10:40:57,054 - INFO - mistral-Nvidia-TRT-LLM (token/sec), int4: 59.70 ± 1.50

I expect that A100 would have better absolute performance, but similar relative performance -- when generation is short, the token/s as measured by current code is off the charts (7600 t/s), and when it's long, it's much slower (176 t/s). While with the fix the generation speed is slightly lower when generation is short (due to kernel launch overhead), and consistent with other measurements. This is happening because the measured "num_output_tokens": len(output_tokens), is around 1.5k for all generations, long or short.

Curious if you see similar discrepancy in reported generation speed with TensoRT-LLM with different prompts? Could be an issue with my build, sorry if that's the case.

otherwise, the output that you see here would have been very different from ground truth.

I think extra EOS tokens in the end would be ignored during decoding, so the output is still fine, it's only how we measure the number of generated tokens is off.

@Anindyadeep
Copy link
Member

I expect that A100 would have better absolute performance, but similar relative performance -- when generation is short, the token/s as measured by current code is off the charts (7600 t/s), and when it's long, it's much slower (176 t/s). While

Wow, 7K tokens/sec, this is concerning, well I saw some discrepencies where the tokens/sec was differing by 100 (like sometimes getting 400 and sometimes 300), but one way to analyse this, is out of 10 generations, I am going to print all of them and see if all of them outputs the same thing or not.

Also, I see you using two envs, can you show me what you changed in the fixed env?

Thanks

@lopuhin
Copy link
Author

lopuhin commented Jun 3, 2024

Makes sense, thank you! If you can print the output_tokens variable inside run_model in bench.py that should show the issue I think.

Also, I see you using two envs, can you show me what you changed in the fixed env?

Sure, here is a mistral-specific fix:

$ git diff bench_tensorrtllm/bench.py
diff --git a/bench_tensorrtllm/bench.py b/bench_tensorrtllm/bench.py
index 22fb1e6..b44be44 100644
--- a/bench_tensorrtllm/bench.py
+++ b/bench_tensorrtllm/bench.py
@@ -100,9 +100,15 @@ class TensorRTLLMBenchmark(BaseBenchmarkClass):
         output_ids = output["output_ids"]
         output_tokens = output_ids[0][0].detach().cpu().tolist()[num_input_tokens:]
 
+        eos = 2  # FIXME this is mistral-only
+        if eos in output_tokens:
+            num_output_tokens = output_tokens.index(eos) + 1
+        else:
+            num_output_tokens = len(output_tokens)
+
         return {
             "output_tokens": output_tokens,
-            "num_output_tokens": len(output_tokens),
+            "num_output_tokens": num_output_tokens,
         }

@Anindyadeep
Copy link
Member

Thanks @lopuhin, will update you very soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants