-
Notifications
You must be signed in to change notification settings - Fork 191
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
[test] Ensure that the first token generation is not included into TPOT #1414
[test] Ensure that the first token generation is not included into TPOT #1414
Conversation
@@ -101,7 +101,7 @@ void PerfMetrics::evaluate_statistics(std::optional<TimePoint> start_time) { | |||
|
|||
auto ttft = tok_times[0] - start_time_val; | |||
raw_metrics.m_times_to_first_token = std::vector<MicroSeconds>(); | |||
raw_metrics.m_times_to_first_token.emplace_back(ttft / batch_sizes[0]); | |||
raw_metrics.m_times_to_first_token.emplace_back(ttft); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have batch 10 and it takes 1 sec until the first token is generated then time to the first token is still 1 sec not 100 ms! Therefore i removed / batch_sizes[0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but should we notify llm_bench about the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked @eaidova what she thinks. Let's wait her answer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterchen-intel @wgzintel do you have objections? What the reason to divide first token latency on batch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i see from blame, this division was added by @ialbrecht. Do you have any objection to remove it?
a247803
to
a1e0d35
Compare
CVS-155098