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.

Sign up for GitHub

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

Jump to bottom

lazy compute input ids #2216

Merged
merged 5 commits into from
Jul 23, 2024
Merged

lazy compute input ids #2216

merged 5 commits into from
Jul 23, 2024

Conversation

lanking520
Copy link
Contributor

Description

Make input ids computation to lazy, not required all the time

@lanking520 lanking520 requested review from zachgk, frankfliu and a team as code owners July 22, 2024 05:58
@lanking520 lanking520 changed the title lazy compute input ids [DO NOT MERGE] lazy compute input ids Jul 22, 2024
davidthomas426
davidthomas426 approved these changes Jul 22, 2024
View reviewed changes
engines/python/setup/djl_python/input_parser.py
# assigns input_ids
# TODO: for dynamic batching, or HF pipeline, tokenizer is applied differently.
if kwargs.get("tokenizer") and kwargs.get("is_rolling_batch"):
request_input.input_ids = tokenizer.encode(request_input.input_text)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the tokenizer at all anymore in this function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed for chat_completions and bedrock use-cases to apply the chat-template.

siddvenk
siddvenk reviewed Jul 22, 2024
View reviewed changes
engines/python/setup/djl_python/rolling_batch/rolling_batch_vllm_utils.py
if vllm_request_output.prompt_logprobs and not request_output.prompt_tokens_details:
# TODO: Temp check adding the check fo T5.
if isinstance(vllm_request_output.prompt_token_ids, list):
for index, prompt_token_id in enumerate(
vllm_request_output.prompt_token_ids):
prompt_token = Token(
id=prompt_token_id,
text=tokenizer.decode([prompt_token_id]),
text=tokenizer.convert_ids_to_tokens(prompt_token_id),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can choose to leave this as None, but this method is just a dict lookup

@siddvenk
Copy link
Contributor

I ran some benchmarks to understand the impact of these changes:

For all tests, I used the command:

TOKENIZER=llama3-8b ./awscurl -c 64 -N 1000 -X POST http://127.0.0.1:8080/invocations \
  --connect-timeout 120  \
  --duration 300   \
 --delay "rand(0, 10000)"  \
 -H "Content-type: application/json"   \
 --dataset openorca_djlserving.jsonl  \
 -t -P -o response.txt

v0.28.0 container:

{
  "tokenizer": "llama3-8b",
  "totalTimeMills": 306441.11,
  "totalRequests": 1038,
  "failedRequests": 0,
  "errorRate": 0.0,
  "concurrentClients": 64,
  "tps": 3.55,
  "tokenThroughput": 632.39,
  "totalTokens": 185078,
  "tokenPerRequest": 178,
  "averageLatency": 18044.76,
  "p50Latency": 23678.73,
  "p90Latency": 24960.39,
  "p99Latency": 26134.46,
  "timeToFirstByte": 5117.05,
  "p50TimeToFirstByte": 4292.293989,
  "p90TimeToFirstByte": 10004.263995,
  "p99TimeToFirstByte": 13723.521713
}

current lmi-nightly without changes in this PR:

{
  "tokenizer": "llama3-8b",
  "totalTimeMills": 309870.87,
  "totalRequests": 995,
  "failedRequests": 0,
  "errorRate": 0.0,
  "concurrentClients": 64,
  "tps": 3.37,
  "tokenThroughput": 601.01,
  "totalTokens": 177572,
  "tokenPerRequest": 178,
  "averageLatency": 19004.11,
  "p50Latency": 24581.14,
  "p90Latency": 25958.34,
  "p99Latency": 27906.81,
  "timeToFirstByte": 5733.94,
  "p50TimeToFirstByte": 5107.926974,
  "p90TimeToFirstByte": 10124.48556,
  "p99TimeToFirstByte": 14215.705665
}

current lmi + changes in this PR:

{
  "tokenizer": "llama3-8b",
  "totalTimeMills": 308637.27,
  "totalRequests": 1037,
  "failedRequests": 0,
  "errorRate": 0.0,
  "concurrentClients": 64,
  "tps": 3.52,
  "tokenThroughput": 633.63,
  "totalTokens": 186596,
  "tokenPerRequest": 179,
  "averageLatency": 18174.85,
  "p50Latency": 23589.39,
  "p90Latency": 24972.48,
  "p99Latency": 26618.66,
  "timeToFirstByte": 5423.92,
  "p50TimeToFirstByte": 4612.326048,
  "p90TimeToFirstByte": 9597.682596,
  "p99TimeToFirstByte": 13975.139746
}

lanking520
lanking520 commented Jul 23, 2024
View reviewed changes
Copy link
Contributor Author

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @siddvenk

@lanking520 lanking520 changed the title [DO NOT MERGE] lazy compute input ids lazy compute input ids Jul 23, 2024
@lanking520 lanking520 merged commit 8750525 into deepjavalibrary:master Jul 23, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siddvenk siddvenk siddvenk left review comments

@davidthomas426 davidthomas426 davidthomas426 approved these changes

@frankfliu frankfliu frankfliu approved these changes

@zachgk zachgk Awaiting requested review from zachgk zachgk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lanking520 @siddvenk @davidthomas426 @frankfliu