Remove the unreachable prepare_multimodal_messages_vllm - #6946
Open
behroozazarkhalili wants to merge 2 commits into
Open
Remove the unreachable prepare_multimodal_messages_vllm#6946behroozazarkhalili wants to merge 2 commits into
behroozazarkhalili wants to merge 2 commits into
Conversation
…ges_vllm Nothing in trl has called this function since #5238. Its history is two steps: #4700 moved the call out of the GRPO and RLOO trainers into trl/generation/vllm_generation.py, and #5238 ("Tokenize before vLLM generation call") then removed that call site. The vLLM path has used to_openai_messages since. Only the test suite still exercised the function. #6928 reports that it raises KeyError('content') on a tool-call message that carries no content key. Fixing that would keep an unreachable function alive, so remove it instead. The function was also exported from trl/__init__.py, so this drops a public name. That matches how the repo already treats a redundant utility of this size: #6158 removed get_kbit_device_map() the same way, with no deprecation cycle. Dropping the function leaves `import copy` unused in trl/data_utils.py and in tests/test_data_utils.py, so both imports go too. The sibling prepare_multimodal_messages is untouched and still has 26 call sites across 12 files. Resolves #6928
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
…prepare-multimodal-vllm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Removes
prepare_multimodal_messages_vllmfromtrl/data_utils.py, its two entries intrl/__init__.py, and theTestPrepareMultimodalMessagesVLLMtest class.Resolves #6928.
Why remove rather than fix
#6928 reports that the function raises
KeyError('content')on an assistant tool-call message thatcarries no
contentkey. Reproduced against the function as written:The guard at
if isinstance(message["content"], list)assumes every message has acontentkey.Nothing calls the function, so fixing it would keep unreachable code alive. @qgallouedec said the
same on the issue and asked for removal instead.
Provenance, checked commit by commit
The function did not become dead in one step:
0eb66d8fgrpo_trainerandrloo_trainerintovllm_generation.py:509. Still live.ba89a387So the function has been unreachable since 2026-03-10, and #5238 is what made it so. The vLLM
path uses
to_openai_messages(vllm_client.py:63, called at:327and:573).This drops a public name
Worth flagging explicitly, because a call-site search does not surface it: the function was exported
from
trl/__init__.py(line 46 in_import_structure, line 95 underTYPE_CHECKING), sofrom trl import prepare_multimodal_messages_vllmresolved onmainbefore this PR. TRL uses alazy
_import_structuredict, where the export appears only as a string literal.Removing it outright rather than deprecating follows the existing treatment of a redundant utility
of this size: #6158 removed
get_kbit_device_map()from those same two lines with no deprecationcycle. The move-to-experimental-then-remove path in this repo has been used for trainers, not for
helpers.
If anyone is calling this from their own code, say so on #6928 and it can be reconsidered.
Scope
trl/data_utils.pyimport copy, now unusedtrl/__init__.pytests/test_data_utils.pyimport copy, now unusedprepare_multimodal_messages, the sibling without the_vllmsuffix, is untouched. It has 26 callsites across 12 files.
require_visionstays imported in the test file because line 47 still uses it.No documentation references the removed function.
Verification
ruff checkandruff format --checkon all three files at the CI-pinned 0.13.3, both clean. Adeliberately broken copy was rejected by the same command, so the gate was reading the files.
tests/test_data_utils.pypasses.import trlno longer exposesprepare_multimodal_messages_vllmand still exposesprepare_multimodal_messages.Both
import copyremovals were caught by ruff F401 rather than by inspection, which is the failuremode this change was most likely to hit.
Note
Low Risk
Dead-code deletion with no in-repo callers; the only risk is external code that imported the removed public symbol.
Overview
Removes the unused
prepare_multimodal_messages_vllmhelper and its public export fromtrl, along with the dedicated test class. The vLLM path no longer calls this converter (messages go throughto_openai_messagesinstead), and the function was buggy on assistant turns withtool_callsand nocontentkey (#6928).prepare_multimodal_messagesand the rest of the data-utils surface are unchanged. Unusedcopyimports are dropped where they were only needed for the removed deep-copy logic.Reviewed by Cursor Bugbot for commit 8ead1b8. Bugbot is set up for automated code reviews on this repo. Configure here.