Skip to content

[KVConnector] Remove redundant method KVConnectorOutput::merge() - #38546

Merged
orozery merged 2 commits into
vllm-project:mainfrom
hickeyma:rem-redundant-merge-func
Mar 31, 2026
Merged

[KVConnector] Remove redundant method KVConnectorOutput::merge()#38546
orozery merged 2 commits into
vllm-project:mainfrom
hickeyma:rem-redundant-merge-func

Conversation

@hickeyma

@hickeyma hickeyma commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Purpose

merge() method calling was removed in #37013 and as its no longer used then it better to remove this function for maintainability purposes.


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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@hickeyma hickeyma changed the title Remove redundant function KVConnectorOutput::merge() [KVConnector] Remove redundant function KVConnectorOutput::merge() Mar 30, 2026
@mergify mergify Bot added the v1 label Mar 30, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request removes the merge method from the KVConnectorOutput class in vllm/v1/outputs.py. The review feedback suggests that the _combine_non_none helper function should also be removed, as it likely becomes unused after the deletion of the merge method.

I am having trouble creating individual review comments. Click here to see my feedback.

vllm/v1/outputs.py (170-206)

high

With the removal of this merge method, the helper function _combine_non_none (defined from line 131) appears to be no longer used. To improve maintainability, please consider removing it as part of this pull request.

@hickeyma hickeyma changed the title [KVConnector] Remove redundant function KVConnectorOutput::merge() [KVConnector] Remove redundant method KVConnectorOutput::merge() Mar 30, 2026
Function calling was removed in
vllm-project#37013

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
hickeyma force-pushed the rem-redundant-merge-func branch from 5afe66d to 38b7cac Compare March 30, 2026 12:59
@orozery

orozery commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

@fynnsu Any objections on this one?

@fynnsu

fynnsu commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

@fynnsu Any objections on this one?

No, #35158 changes the way this is handled so that we only ever produce one output. It seems like this is the preferred approach moving forward, so we should be good to remove this.

@orozery orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 30, 2026
@orozery
orozery merged commit 350af48 into vllm-project:main Mar 31, 2026
49 checks passed
neweyes pushed a commit to neweyes/vllm that referenced this pull request Mar 31, 2026
…m-project#38546)

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Signed-off-by: neweyes <328719365@qq.com>
@hickeyma
hickeyma deleted the rem-redundant-merge-func branch April 1, 2026 08:34
puririshi98 pushed a commit to puririshi98/vllm that referenced this pull request Apr 7, 2026
…m-project#38546)

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Signed-off-by: Rishi Puri <riship@nvidia.com>
mtparet pushed a commit to blackfuel-ai/vllm that referenced this pull request Apr 9, 2026
mystous pushed a commit to mystous/vllm_hybrid that referenced this pull request May 10, 2026
my-other-github-account pushed a commit to my-other-github-account/vllm that referenced this pull request May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants