[Bugfix] In LongRoPE, decide short vs long based on max_model_len

[MatthewBonanni]

Purpose

Per suggestion by @LucasWilkinson, fixes #27414 by choosing short vs long RoPE factors at init time based on max_model_len

Test Plan

Functionality

vllm serve microsoft/Phi-3.5-mini-instruct

guidellm benchmark \
  --target "http://localhost:8080" \
  --rate-type concurrent \
  --rate 16 \
  --data "prompt_tokens=3900,output_tokens=500" \
  --max-requests 100

Accuracy

vllm serve microsoft/Phi-3.5-mini-instruct --max-model-len <len>

lm_eval \
  --model local-chat-completions \
  --model_args base_url=http://localhost:8000/v1/chat/completions,
               model=microsoft/Phi-3.5-mini-instruct,
               num_concurrent=32,
               max_retries=3,
               tokenized_requests=False \
  --tasks gsm8k \
  --apply_chat_template

Test Result

Functionality

No longer produces random tokens after threshold

Accuracy

with max-model-len=4096 (new default):

|Tasks|Version|     Filter     |n-shot|  Metric   |   |Value |   |Stderr|
|-----|------:|----------------|-----:|-----------|---|-----:|---|-----:|
|gsm8k|      3|flexible-extract|     5|exact_match|↑  |0.7559|±  |0.0118|
|     |       |strict-match    |     5|exact_match|↑  |0.5679|±  |0.0136|

with max-model-len=4097 (to trigger long rope)

|Tasks|Version|     Filter     |n-shot|  Metric   |   |Value |   |Stderr|
|-----|------:|----------------|-----:|-----------|---|-----:|---|-----:|
|gsm8k|      3|flexible-extract|     5|exact_match|↑  |0.6702|±  |0.0129|
|     |       |strict-match    |     5|exact_match|↑  |0.3071|±  |0.0127|

---

Essential Elements of an Effective PR Description Checklist

• The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
• The test plan, such as providing test command.
• The test results, such as pasting the results comparison before and after, or e2e results
• (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
• (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

[gemini-code-assist]

Code Review

This pull request addresses a bug in the LongRoPE implementation for Phi-3 models. Previously, the selection between short and long RoPE factors was dynamic, which could lead to KV cache invalidation during generation. The proposed fix makes this decision static at model initialization time, based on max_model_len, ensuring consistency. My review includes a suggestion to improve the implementation by removing a dependency on global configuration, which enhances the component's modularity and testability.

[LucasWilkinson]

Can you please provide gsm8k with a max_model_len > original_max_position_embeddings and max_model_len <= original_max_position_embeddings

[MatthewBonanni]

@LucasWilkinson done!

[hmellor]

Why does this not belong above where derived_max_model_len is calculated?

It could come after the first rope_scaling check?

[MatthewBonanni]

We want to override the default max_model_len to original_max_position_embeddings while still allowing the user to manually specify a max_model_len which is larger than this value. I had originally written the code as modifying derived_max_model_len, but (as I read it) derived_max_model_len is really intended to be max_position_embeddings, because of the later check in line 2143:

https://github.com/vllm-project/vllm/pull/27431/files/684e475735f9594a784dd62aa14e0845884872db#diff-998c640befaf137b9af825f29f4e6e47d273caab1fd04093c97df24b18f5c417L2134

If max_model_len is manually set to a value exceeding derived_max_model_len, it will throw an error. Hopefully I'm understanding your question correctly?

[hmellor]

Thank you for explaining, I think I understand now!