Allow chunked prefill when num_prompt_tokens > max_seq_len

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---

[navsud]

Summary:
Remove the early num_prompt_tokens <= max_seq_len check in TextLLMRunner. TextPrefiller::prefill() already supports chunked prefill — when the prompt is longer than max_seq_len it splits the input into max_seq_len-sized chunks and prefills them sequentially. The previous check rejected this valid case, breaking models exported with max_seq_len < max_context_len (e.g. a 1024 prefill chunk over a 4096 KV cache).

The total-capacity bound is preserved:

• For non-sliding-window models (max_seq_len >= max_context_len), the existing pos_ + num_prompt_tokens < max_context_len check is unchanged.
• For sliding-window models (max_seq_len < max_context_len), a new per-call check num_prompt_tokens < max_context_len ensures the prompt itself fits in KV cache; pos_ doesn't represent consumed capacity for these models since the model handles position wrapping internally.

Differential Revision: D101728720

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19052

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull & trunk workflows in PyTorch main

❌ 1 New Failure, 2 Unrelated Failures

As of commit 501ba12 with merge base 32702ac ():

NEW FAILURE - The following job has failed:

• pull / unittest-editable / macos / macos-job (gh)
  export/tests/test_target_recipes.py::TestTargetRecipes::test_mv2_model

FLAKY - The following job failed but was likely due to flakiness present on trunk:

• pull / unittest / windows / windows-job (gh) (matched win rule in flaky-rules.json)
  ##[error]The operation was canceled.

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@navsud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101728720.

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 14s —— View job

---

Review of PR #19052

• Read the diff and changed files
• Review logic correctness
• Post review feedback

---

Summary: This PR removes the num_prompt_tokens <= max_seq_len guard in TextLLMRunner::generate() and adds a sliding-window-specific capacity check. The motivation is correct: TextPrefiller::prefill() already handles chunked prefill (splitting into max_seq_len-sized chunks at text_prefiller.cpp:41-63), so the old guard was rejecting a valid code path.

Verdict: The change looks correct. A few observations:

What's good:

• The removed check was redundant/incorrect — it rejected prompts that the downstream prefiller already handles via chunking.
• The new else branch (text_llm_runner.cpp:161-169) correctly guards the sliding-window case (max_seq_len < max_context_len) with a per-call bound that doesn't involve pos_, which makes sense since the ring buffer manages position wrapping internally.
• The existing non-sliding-window branch (pos_ + num_prompt_tokens < max_context_len) is preserved unchanged.
• The comment block at lines 141-150 clearly explains the rationale.

Minor observations (non-blocking):

1. Strict inequality on capacity check (text_llm_runner.cpp:163): The sliding-window check uses num_prompt_tokens < max_context_len, meaning a prompt of exactly max_context_len tokens is rejected. This matches the existing non-sliding-window check's use of < (line 153), so it's consistent — just noting that a prompt of exactly max_context_len tokens would fill the entire KV cache leaving no room for generation. This is the correct behavior.

2. resolve_max_new_tokens with effective_pos = 0 for sliding-window (line 196): For sliding-window models, effective_pos is set to 0, so resolve_max_new_tokens(max_context_len, 0) gives the full budget. This was pre-existing logic (not introduced by this PR), but it means the generation loop doesn't account for the prompt tokens that were just prefilled consuming some of the sliding window. This seems intentional since the ring buffer recycles, but worth being aware of for very long generations.

3. No test coverage for the new path: There are no test additions for the case where num_prompt_tokens > max_seq_len with max_seq_len < max_context_len. An integration or unit test covering chunked prefill through the runner would help prevent regressions on this path, but this can be a follow-up.

Overall this is a clean, well-scoped fix.

---