Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions tests/parser/test_harmony.py
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,27 @@ def test_single_channel(self, harmony_parser):
(s.channel, s.recipient, s.delta) for s in result.segments if s.delta
] == [("final", None, "Hello")]

def test_constrained_output_segment_recipient_normalized(self, harmony_parser):

This comment was marked as resolved.

result = harmony_parser.process_chunk(
encode_output(
'<|channel|>final <|constrain|>json<|message|>{"result":true}<|end|>'
)
)

content_segments = [segment for segment in result.segments if segment.delta]
assert all(segment.channel == "final" for segment in content_segments)
assert all(segment.recipient is None for segment in content_segments)
assert (
"".join(segment.delta for segment in content_segments) == '{"result":true}'
)
completed_messages = [
segment.completed_message
for segment in result.segments
if segment.completed_message is not None
]
assert len(completed_messages) == 1
assert completed_messages[0].recipient is None

def test_cross_channel(self, harmony_parser):
result = harmony_parser.process_chunk(
encode_output(
Expand Down
20 changes: 18 additions & 2 deletions vllm/parser/harmony.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def _poll_completed_message(self) -> Message | None:
if len(messages) <= self._num_processed_messages:
return None
msg = messages[self._num_processed_messages]
msg.recipient = self._normalize_recipient(msg.recipient)
self._num_processed_messages += 1
return msg

Expand Down Expand Up @@ -192,7 +193,9 @@ def parse_delta(
*,
finished: bool,
) -> DeltaMessage | None:
prev_recipient = self._harmony_parser.current_recipient
prev_recipient = self._normalize_recipient(
self._harmony_parser.current_recipient
)
result = self.process_chunk(delta_token_ids)
if finished:
flushed_segment = self.flush()
Expand Down Expand Up @@ -274,7 +277,9 @@ def process_chunk(self, token_ids: Sequence[int]) -> ChunkResult:
for token_id in token_ids:
self._harmony_parser.process(token_id)
channel = self._harmony_parser.current_channel
recipient = self._harmony_parser.current_recipient
recipient = self._normalize_recipient(
self._harmony_parser.current_recipient
)
delta = self._harmony_parser.last_content_delta or ""
completed_message = self._poll_completed_message()

Expand All @@ -298,3 +303,14 @@ def process_chunk(self, token_ids: Sequence[int]) -> ChunkResult:
segments=segments,
reasoning_token_count=reasoning_token_count,
)

@staticmethod
def _normalize_recipient(recipient: str | None) -> str | None:
"""Remove constrained formats misparsed into recipients by older Harmony."""
if recipient is None:
return None

constrain_index = recipient.find("<|constrain|>")
if constrain_index == -1:
return recipient
return recipient[:constrain_index].rstrip() or None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this work in the real world? Here's a prior example I happened to have written down of how the constrain often leaks into the wrong place:

<|channel|>analysis<|message|>User asks: "What's the weather like in Paris today?" We need to get weather info. Use get_weather function. We need coordinates for Paris. Latitude 48.8566, longitude 2.3522.<|end|><|start|>assistant<|channel|>commentary <|constrain|>functions.get_weather<|message|>{"latitude":48.8566,"longitude":2.3522}<|call|>

I'm worried about the find and rstrip here in cases like this. It would potentially catch some cases, but I'm unsure how much it will hold up in the typical long context real-world failure scenarios we frequently see with these models. Do we have evidence this helps those broadly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does this work in the real world? Here's a prior example I happened to have written down of how the constrain often leaks into the wrong place:

<|channel|>analysis<|message|>User asks: "What's the weather like in Paris today?" We need to get weather info. Use get_weather function. We need coordinates for Paris. Latitude 48.8566, longitude 2.3522.<|end|><|start|>assistant<|channel|>commentary <|constrain|>functions.get_weather<|message|>{"latitude":48.8566,"longitude":2.3522}<|call|>

I'm worried about the find and rstrip here in cases like this. It would potentially catch some cases, but I'm unsure how much it will hold up in the typical long context real-world failure scenarios we frequently see with these models. Do we have evidence this helps those broadly?

Hi @bbrowning

Thanks for confirming, this works for the specific case addressed here. This PR is only a narrow fix for #45570.

I’ll continue looking into the broader <|constrain|> leakage issue and follow up with a more robust solution in a separate PR.

Loading