[CI Failure] fix_test_auto_prefix_cache_support

[hl475]

Purpose

Recent changes from #25075 breaks two tests from tests/models/language/pooling/test_auto_prefix_cache_support.py (https://buildkite.com/vllm/ci/builds/33031/steps/canvas?sid=01999dee-5c62-422e-9c31-054b1091dc6b).

Per suggestion, the breaking change from previous PR is from https://github.com/vllm-project/vllm/blob/main/vllm/config/vllm.py#L399-L402 or

        if (self.scheduler_config.chunked_prefill_enabled is False
                or disable_chunked_prefill_reasons):

This PR is changing this predicate by considering self.cache_config.enable_prefix_caching as well.

Test Plan

pytest -v -s tests/models/language/pooling/test_auto_prefix_cache_support.py

pytest -v -s tests/v1/core/test_scheduler.py

Test Result

both succeed

---

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.

[DarkLight1337]

cc @noooop can you review this?

[noooop]

cc @noooop can you review this?

Sorry. I’m currently traveling and can only run the test locally when I return in a few days.

[DarkLight1337]

This is due to recent V1 config finalization ties prefix caching (APC) to the scheduler’s chunked prefill (CP).

For reference, which PR is this referring to?

[noooop]

This is due to recent V1 config finalization ties prefix caching (APC) to the scheduler’s chunked prefill (CP).

For reference, which PR is this referring to?

Should these models have enable_prefix_caching=True by default? ？？Start from #20930

I’ll need some time to catch up.

I think it would be best to fix this default behavior.

[noooop]

https://github.com/vllm-project/vllm/blob/main/vllm/config/vllm.py

Did one of these three PRs break it?

[hl475]

@noooop @DarkLight1337

my guess will be e23cacd#diff-bee6813076031d3ca1edc903c1b02b81e4676519afc562ce3fefe37f20c7b650 which is changing some logic about chunked_prefill_enabled config

[DarkLight1337]

The enable_chunked_prefill flag should be enabled by default for these models. I think we should fix the previous PR to avoid this regression instead of updating the tests.

[noooop]

before line if self.enable_chunked_prefill is None: so it will not going into the if branch on L1576

Was it also self.enable_chunked_prefill = false before #25075 here？

[hl475]

before line if self.enable_chunked_prefill is None: so it will not going into the if branch on L1576

Was it also self.enable_chunked_prefill = false before #25075 here？

Yeah, just checked with commit 2e1b8bc2 (right before commit e23cacda3 or #25075 ), I am seeing the same

(Pdb) self.enable_chunked_prefill
False

[noooop]

before line if self.enable_chunked_prefill is None: so it will not going into the if branch on L1576

Was it also self.enable_chunked_prefill = false before #25075 here？

Yeah, just checked with commit 2e1b8bc2 (right before commit e23cacda3 or #25075 ), I am seeing the same

(Pdb) self.enable_chunked_prefill
False

Will the test pass at this point？

[hl475]

before line if self.enable_chunked_prefill is None: so it will not going into the if branch on L1576

Was it also self.enable_chunked_prefill = false before #25075 here？

Yeah, just checked with commit 2e1b8bc2 (right before commit e23cacda3 or #25075 ), I am seeing the same

(Pdb) self.enable_chunked_prefill
False

Will the test pass at this point？

Yes!

[noooop]

before line if self.enable_chunked_prefill is None: so it will not going into the if branch on L1576

Was it also self.enable_chunked_prefill = false before #25075 here？

Yeah, just checked with commit 2e1b8bc2 (right before commit e23cacda3 or #25075 ), I am seeing the same

(Pdb) self.enable_chunked_prefill
False

Will the test pass at this point？

Yes!

I can’t think of why it would work.

╮(╯_╰)╭

we’ve got to refactor this piece of logic.

[hl475]

@noooop

i am thinking some other alternative without introducing this complex helper function - as we already identified the root cause PR, and the key change from that PR which breaks the two model tests are from

if (self.scheduler_config.chunked_prefill_enabled is False
                or disable_chunked_prefill_reasons):

(before that PR, it is just if disable_chunked_prefill_reasons:), I am thinking maybe I can update the condition to something like

apc_requested = (self.cache_config is not None and self.cache_config.enable_prefix_caching)
if (
     disable_chunked_prefill_reasons
     or (self.scheduler_config.chunked_prefill_enabled is False and not apc_requested)
):

I think the test will pass but not sure which way is more appropriate. What do you think?

[noooop]

without introducing this complex helper function

I am thinking maybe I can update the condition to something like

apc_requested = (self.cache_config is not None and self.cache_config.enable_prefix_caching)
if (
     disable_chunked_prefill_reasons
     or (self.scheduler_config.chunked_prefill_enabled is False and not apc_requested)
):

I think the test will pass but not sure which way is more appropriate. What do you think?

I prefer the approach below. The helper function feels too complex.

[hl475]

Thanks @noooop and @DarkLight1337 !

I updated this PR again based on the discussion. Please take another look when you have a chance.

[noooop]

Honestly, since I can only look at the code on my phone, I have a ton of questions about this part of the logic.

LGTM if @DarkLight1337 approves. Or, I’ll give it a closer look when I’m back on the 8th.

[DarkLight1337]

Let's unblock the CI first. @noooop can clean it up later if needed

[noooop]

@ maxdebayser

Sorry. I’m currently traveling, I can only look at the code on my phone.

Please look into this issue. The logic for enabling chunked prefill in the pooling model and the encode-decode model is somewhat coupled. As more and more PRs modify this logic, it has become a complete mess.

The strangest thing is, self.enable_chunked_prefill == false is on line 1575. （maybe unrelated）

vllm/vllm/engine/arg_utils.py

Lines 1549 to 1580 in 47b9339

	if model_config.runner_type != "pooling":
	self.enable_chunked_prefill = True
	# TODO: When prefix caching supports prompt embeds inputs, this
	# check can be removed.
	if (self.enable_prompt_embeds
	and self.enable_prefix_caching is not False):
	logger.warning(
	"--enable-prompt-embeds and --enable-prefix-caching "
	"are not supported together in V1. Prefix caching has "
	"been disabled.")
	self.enable_prefix_caching = False
	if self.enable_prefix_caching is None:
	self.enable_prefix_caching = True
	else:
	pooling_type = model_config.pooler_config.pooling_type
	is_causal = getattr(model_config.hf_config, "is_causal", True)
	incremental_prefill_supported = (pooling_type is not None
	and pooling_type.lower() == "last"
	and is_causal)
	action = "Enabling" if \
	incremental_prefill_supported else "Disabling"
	if self.enable_chunked_prefill is None:
	self.enable_chunked_prefill = incremental_prefill_supported
	logger.info("(%s) chunked prefill by default", action)
	if self.enable_prefix_caching is None:
	self.enable_prefix_caching = incremental_prefill_supported
	logger.info("(%s) prefix caching by default", action)

The function is named “Set Default Argument,” but the value may come from elsewhere, and it just makes the logic a more confusing.

---

I feel the next release is just around the corner, so we need to fix this issue ASAP.

[noooop]

@hl475

Current fix broke the encode-decode model. Let’s find a way to fix it.

[hl475]

@hl475

Current fix broke the encode-decode model. Let’s find a way to fix it.

Thanks! Just submitted a new version. Let's wait for CI

[noooop]

test_full_cudagraph.py fail is not related to this PR

Thanks for the fix

cc @DarkLight1337