Skip to content

Conversation

@pierDipi
Copy link
Member

@pierDipi pierDipi commented Nov 20, 2025

Bump + fix breaking changes

@vMaroon
Copy link
Member

vMaroon commented Nov 20, 2025

There will be a v0.4.0 release sometime today - this PR can import it.

@nirrozenbaum
Copy link
Collaborator

/hold until kv-cache manager v0.4 is released.

@github-actions github-actions bot added the hold label Nov 20, 2025
@pierDipi
Copy link
Member Author

yeah, it's WIP for that reason, is this a known flake?

  STEP: Waiting for pods of qwen-qwen2-5-1-5b-instruct-vllm-sim to be ready @ 11/20/25 08:59:30.392
panic: test timed out after 10m0s
	running tests:
		TestEndToEnd (10m0s)

@vMaroon
Copy link
Member

vMaroon commented Nov 20, 2025

https://github.com/llm-d/llm-d-kv-cache-manager/releases/tag/v0.4.0-rc1 was released, it requires some changes in addition to the version bump. I tagged you in the relevant PR, this one is mandatory: https://github.com/llm-d/llm-d-kv-cache-manager/pull/150/files#r2547769214, 2nd comment is optional.

Do you want to handle? An alternative can be merging this and handling separately.

yeah, it's WIP for that reason, is this a known flake?

  STEP: Waiting for pods of qwen-qwen2-5-1-5b-instruct-vllm-sim to be ready @ 11/20/25 08:59:30.392
panic: test timed out after 10m0s
	running tests:
		TestEndToEnd (10m0s)

This is not common - weird.

@pierDipi pierDipi force-pushed the bump-kv-cache-manager branch 3 times, most recently from 3151f47 to 08b27db Compare November 21, 2025 07:04
@pierDipi
Copy link
Member Author

It should be fixed now

@pierDipi pierDipi changed the title [WIP] Bump kv-cache-manager to latest [WIP] Bump kv-cache-manager to v0.4.0-rc1 Nov 21, 2025
@pierDipi pierDipi changed the title [WIP] Bump kv-cache-manager to v0.4.0-rc1 Bump kv-cache-manager to v0.4.0-rc1 Nov 21, 2025
@pierDipi
Copy link
Member Author

/cc @vMaroon @nirrozenbaum

@nirrozenbaum
Copy link
Collaborator

/hold cancel
/cc @vMaroon

@nirrozenbaum nirrozenbaum removed the hold label Nov 23, 2025
@vMaroon vMaroon force-pushed the bump-kv-cache-manager branch from 7be2fe0 to 1f18375 Compare November 23, 2025 09:07
Comment on lines +56 to 61
if token := os.Getenv("HF_TOKEN"); token != "" &&
parameters.IndexerConfig != nil &&
parameters.IndexerConfig.TokenizersPoolConfig != nil &&
parameters.IndexerConfig.TokenizersPoolConfig.HFTokenizerConfig != nil {
parameters.IndexerConfig.TokenizersPoolConfig.HFTokenizerConfig.HuggingFaceToken = token
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

these lines look like internal implementation details of kvcache manager code.
can we move this to kvcache repo?
I'd expect the call indexerConfig, err := kvcache.NewDefaultConfig() in L45 to initialize that internally.

Copy link
Member

Choose a reason for hiding this comment

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

The kvcache library defines a configuration with default values. This code piece is that of a user, permitting configuration through the env-var HF_TOKEN. It is arguable that this is a special env-var that is widely accepted but generally in the kvcache library we attempt to contain all configuration in the referenced structure, leaving customized UX to the users.

Copy link
Collaborator

Choose a reason for hiding this comment

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

right. it's a user defined env var (with the user's token).
HF_TOKEN as env var is very acceptable.

the question I was trying to answer is why do we read that env var here and not in kvcache code.
this part looks very not natural, the scorer factory function writes to an internal config of the kvcache indexer parameters.
I was expecting to have the os.getenv call inside kvcache.NewDefaultConfig().

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this discussion is applicable to the current setup, should we discuss it separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that kv cache manager library can do this since the use can still opt-out from the default env-var injection by not using the NewDefaultConfig function if they want to have different defaults

Copy link
Member

Choose a reason for hiding this comment

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

Let's track this in a new issue, I think this should not be a blocker.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not a blocker 👍

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@pierDipi pierDipi force-pushed the bump-kv-cache-manager branch from 1f18375 to 2125f03 Compare November 24, 2025 07:48
@pierDipi pierDipi changed the title Bump kv-cache-manager to v0.4.0-rc1 Bump kv-cache-manager to v0.4.0-rc2 Nov 24, 2025
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@vMaroon
Copy link
Member

vMaroon commented Nov 24, 2025

/lgtm
/approve

@github-actions github-actions bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2025
@github-actions github-actions bot merged commit 4c77042 into llm-d:main Nov 24, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in llm-d-inference-scheduler Nov 24, 2025
@pierDipi pierDipi deleted the bump-kv-cache-manager branch November 24, 2025 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm "Looks good to me", indicates that a PR is ready to be merged.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants