Repository navigation
refactor(harmony): clean up parser signatures, fix delta accumulation, and GptOss tests - #592
Merged
Merged
Conversation
CatherineSue
requested review from
XinyueZhang369,
key4ng and
slin1237
as code owners
March 3, 2026 19:41
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
github-actions
Bot
added
grpc
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where Harmony's path failed to correctly handle stop sequence trimming, leading to stop sequences being included in API responses. The changes introduce comprehensive trimming logic for both streaming and non-streaming scenarios, ensuring that model outputs are clean and adhere to the specified stop criteria. Additionally, it integrates support for the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves per-channel matched_stop propagation and the matched_stop field from HarmonyChannelOutput; adjusts parser/streaming/processor APIs to stop accepting matched_stop, shifts to per-channel delta accumulation and per-index stop buffering, and updates chat-completion tests to tolerate token counting and normalize delta extraction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Processor as Processor
participant Stream as Stream
participant StopBuffer as StopSequenceBuffer
participant Parser as Parser
Client->>Processor: Send chat request (stop_sequences, no_stop_trim)
Processor->>Parser: parse_complete(output_ids, finish_reason)
activate Parser
Parser->>Parser: build per-channel deltas (analysis_delta, final_delta, commentary)
Parser-->>Processor: HarmonyChannelOutput (no matched_stop)
deactivate Parser
Client->>Processor: Open streaming request
Processor->>Stream: start stream (pass stop_sequences/no_stop_trim)
Stream->>StopBuffer: create per-index StopSequenceBuffer
loop incoming chunks
Stream->>Parser: parse_chunk(...)
Parser-->>Stream: per-channel deltas (analysis_delta, final_delta, commentary)
Stream->>StopBuffer: buffer per-channel text
StopBuffer-->>Stream: emit safe prefix (avoid partial stop)
Stream-->>Client: emit safe text chunk
end
Stream->>StopBuffer: flush remaining buffered text
Stream->>Parser: finalize(finish_reason)
Parser-->>Stream: finalize result
Stream->>StopBuffer: use stored per-index stop info for final emission
Stream-->>Client: emit final chunk
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request effectively implements stop sequence trimming for the Harmony path, addressing a key feature gap. The introduction of StopSequenceBuffer for streaming is a solid approach to handle stop sequences that span across multiple chunks. The changes also correctly implement the no_stop_trim flag to give users control over this behavior.
I've provided two suggestions to improve the maintainability of the new code by addressing duplicated logic, aligning with established code review rules. One is a high-priority suggestion to address significant code duplication in streaming.rs.
Overall, these changes are a great improvement and make the Harmony path more robust and compliant with the API specification.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/chat_completions/test_openai_server.py`:
- Around line 403-411: The helper function _delta_text is defined locally inside
the test and should be promoted for reuse; move the logic currently in
_delta_text(delta) to a module-level utility or a class-level method (e.g., on
the test class) and update the test to call the new helper instead of the nested
function; ensure it still returns delta.content or getattr(delta,
"reasoning_content", "") or "" and update any other tests to import or reference
the new helper to avoid duplication.
In `@model_gateway/src/routers/grpc/harmony/parser.rs`:
- Around line 133-145: Add a trace/debug log inside the unknown-channel fallback
branch (the _ => arm) that indicates we're stripping the stop token from both
channels; include identifying details like the stop_str value and the current
lengths or presence of analysis (e.g., analysis.as_ref().map(|t| t.len())) and
final_text.len() to aid debugging. Use the crate's logging macro (e.g.,
tracing::trace! or log::debug!) and ensure to import the logging facade if not
already imported so the message is emitted when tracing/debug is enabled.
In `@model_gateway/src/routers/grpc/harmony/streaming.rs`:
- Around line 102-138: Add unit tests for the buffer function that exercise
UTF-8 multi-byte boundaries and stop-sequence splits: write tests calling buffer
with a buf (String) and new_text containing multi-byte characters (e.g., emojis
or non-Latin scripts) where stop_sequences include sequences that would fall
inside a multi-byte character when concatenated, varying max_stop_len to force
the char-boundary branch in buffer; verify that buffer returns the correct safe
prefix (not splitting a character), that the remaining buf retains the full
multi-byte tail, and cases where safe is empty or a stop sequence is found
immediately. Target the buffer function and assert behavior for (1) stop
sequence spanning a multi-byte char boundary, (2) retention logic when buf.len()
> max_stop_len-1, and (3) no-stop-sequence path to ensure is_char_boundary
handling works.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
e2e_test/chat_completions/test_openai_server.pymodel_gateway/src/routers/grpc/harmony/parser.rsmodel_gateway/src/routers/grpc/harmony/processor.rsmodel_gateway/src/routers/grpc/harmony/streaming.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/harmony/parser.rs`:
- Around line 107-110: Replace the current strict match on matched_stop that
returns early for non-strings with a safer extraction using
matched_stop.as_ref().and_then(|v| v.as_str()) and, if that yields None, handle
the non-string case explicitly (either convert the numeric token ID to a string
if that makes sense for trimming, or log a warning/error about receiving a
non-string matched_stop and return). Update the three occurrences that use the
same pattern (the one shown around matched_stop and the similar blocks at the
other two locations noted) so they consistently try as_str() first and then
either convert numbers to text or emit a diagnostic log instead of silently
skipping trimming.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
e2e_test/chat_completions/test_openai_server.pymodel_gateway/src/routers/grpc/harmony/parser.rs
2 of 3 tasks
This comment was marked as resolved.
This comment was marked as resolved.
mergify
Bot
added
the
needs-rebase
CatherineSue
force-pushed
the
keyang/fix-oss-test
branch
from
March 3, 2026 22:50
5bcc635 to
d7dc80c
Compare
mergify
Bot
removed
the
needs-rebase
CatherineSue
changed the title
CatherineSue
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/grpc/harmony/parser.rs (1)
229-233:⚠️ Potential issue | 🟠 MajorParser entrypoints no longer carry stop-trim control, so request-level
no_stop_trimcannot be honored.At Line 229 and Line 478,
parse_complete/finalizeonly takefinish_reason. Without trim context (e.g., matched stop +no_stop_trim), the parser cannot enforce per-request stop trimming behavior, which is required for Harmony chat parity.🛠️ Suggested API restoration (minimal sketch)
pub fn parse_complete( &mut self, output_ids: &[u32], finish_reason: String, + matched_stop: Option<&serde_json::Value>, + no_stop_trim: bool, ) -> Result<HarmonyChannelOutput, String> { @@ - Ok(HarmonyChannelOutput { + // apply stop trimming only when no_stop_trim == false + // (use matched_stop string when available) + Ok(HarmonyChannelOutput { analysis, commentary, final_text, finish_reason: final_finish_reason, reasoning_token_count, }) } @@ -pub fn finalize(&mut self, finish_reason: String) -> HarmonyChannelOutput { +pub fn finalize( + &mut self, + finish_reason: String, + matched_stop: Option<&serde_json::Value>, + no_stop_trim: bool, +) -> HarmonyChannelOutput { @@ - HarmonyChannelOutput { + // same trim policy here as parse_complete + HarmonyChannelOutput { analysis, commentary, final_text, finish_reason: final_finish_reason, reasoning_token_count: self.reasoning_token_count, } }Also applies to: 478-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/harmony/parser.rs` around lines 229 - 233, The parser entrypoints parse_complete and finalize currently only accept finish_reason so they cannot honor per-request stop-trim control (no_stop_trim / matched-stop info); restore a minimal API by adding a stop-trim context parameter (e.g., no_stop_trim: bool and/or matched_stop: Option<String>) to parse_complete and finalize signatures and propagate that parameter through the internal calls that perform trimming/stop handling (search for parse_complete and finalize usages in parser.rs and callers) so the parser can respect request-level no_stop_trim behavior when deciding to trim stops from outputs; update all callers to pass the request's stop-trim flags accordingly and adjust any downstream logic to consult the new parameter before trimming output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/chat_completions/test_openai_server.py`:
- Around line 593-598: The tests currently set STOP_SEQUENCE_TRIMMED = False
which asserts legacy untrimmed behavior; update STOP_SEQUENCE_TRIMMED to True to
match the restored default stop-trimming behavior and ensure tests validate the
correct default. Also add one explicit test path that sets no_stop_trim=True
(e.g., add a variant in the existing test harness that invokes the same
inference/run flow with no_stop_trim=True) so both default
(STOP_SEQUENCE_TRIMMED=True) and opt-out (no_stop_trim=True) behaviors are
covered; look for the STOP_SEQUENCE_TRIMMED constant and the test invocation
blocks that use STREAMING_TOKEN_TOLERANCE to insert the new no_stop_trim test
case.
In `@model_gateway/src/routers/grpc/harmony/parser.rs`:
- Around line 376-385: The code uses repeated string concatenation (existing +
&delta_text) when updating analysis_delta and final_delta inside the token loop,
causing repeated reallocations; replace those expressions by obtaining a mutable
String via get_or_insert_with(String::new) on analysis_delta and final_delta and
call push_str(&delta_text) to append in-place (i.e., use
analysis_delta.get_or_insert_with(String::new).push_str(&delta_text) and
similarly for final_delta) so growth is amortized and avoids copies in the hot
path.
In `@model_gateway/src/routers/grpc/harmony/streaming.rs`:
- Around line 795-797: You compute final_output via
parser.finalize(finish_reason.clone()) once; avoid re-invoking parser.finalize
later — reuse final_output.analysis instead of calling parser.finalize again at
the later location (around where final_output is re-created). Update the code
paths that currently call parser.finalize a second time (the later block
handling responses/analysis) to reference the previously computed
final_output.analysis so behavior doesn't depend on finalize being idempotent
and to eliminate redundant work.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/harmony/parser.rs`:
- Around line 229-233: The parser entrypoints parse_complete and finalize
currently only accept finish_reason so they cannot honor per-request stop-trim
control (no_stop_trim / matched-stop info); restore a minimal API by adding a
stop-trim context parameter (e.g., no_stop_trim: bool and/or matched_stop:
Option<String>) to parse_complete and finalize signatures and propagate that
parameter through the internal calls that perform trimming/stop handling (search
for parse_complete and finalize usages in parser.rs and callers) so the parser
can respect request-level no_stop_trim behavior when deciding to trim stops from
outputs; update all callers to pass the request's stop-trim flags accordingly
and adjust any downstream logic to consult the new parameter before trimming
output.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
e2e_test/chat_completions/test_openai_server.pymodel_gateway/src/routers/grpc/harmony/parser.rsmodel_gateway/src/routers/grpc/harmony/processor.rsmodel_gateway/src/routers/grpc/harmony/streaming.rs
CatherineSue
force-pushed
the
keyang/fix-oss-test
branch
from
March 3, 2026 23:56
9c83ca2 to
902f2b2
Compare
5 of 6 tasks
CatherineSue
changed the title
CatherineSue
changed the title
CatherineSue
force-pushed
the
keyang/fix-oss-test
branch
from
March 4, 2026 02:28
06b9640 to
bb712e1
Compare
…completion tests - Introduced a `STREAMING_TOKEN_TOLERANCE` to allow for variations in token counts. - Updated assertions to use a fallback mechanism for retrieving message content, accommodating models that may use `reasoning_content`. - Removed skipped test cases related to Harmony's content handling, as the logic has been improved. Signed-off-by: Keyang Ru <rukeyang@gmail.com>
- Added support for user-specified stop sequences to the HarmonyParserAdapter, allowing for more controlled streaming output. - Introduced a jail buffer mechanism to manage text around stop sequences, ensuring that they are not emitted as part of the content deltas. - Updated HarmonyStreamingProcessor to utilize the new stop sequence functionality during streaming processing. Signed-off-by: [Your Name] [Your Email] Signed-off-by: Keyang Ru <rukeyang@gmail.com>
…uenceBuffer - Removed user-specified stop sequences from HarmonyParserAdapter, streamlining its initialization. - Introduced StopSequenceBuffer to manage stop sequences and buffer text for analysis and final channels, ensuring proper handling during streaming. - Updated HarmonyStreamingProcessor to utilize StopSequenceBuffer for improved stop sequence management. Signed-off-by: Keyang Ru <rukeyang@gmail.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
…nish_reason Signed-off-by: Chang Su <chang.s.su@oracle.com>
… and trace logging Signed-off-by: Chang Su <chang.s.su@oracle.com>
…atched_stop Stop sequence stripping in both streaming (StopSequenceBuffer::flush) and non-streaming (strip_stop_sequence) paths now checks the buffer against known stop sequences directly, matching how StopSequenceDecoder works. No longer depends on the backend's matched_stop field for trimming. Signed-off-by: Chang Su <chang.s.su@oracle.com>
The parser no longer passes matched_stop through its signatures. Callers that need matched_stop for API responses now read it directly from the proto wrapper, keeping the parser focused on text parsing. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Harmony's detokenization is not channel-aware, so text-level stop sequence trimming was fundamentally imprecise. Remove StopSequenceBuffer, strip_stop_sequence, and all stop_sequences/no_stop_trim parameter threading. The backend still stops generation correctly; the stop sequence simply remains as a suffix in the output text. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Harmony (gpt-oss) does not trim stop sequences from output since its detokenization is not channel-aware. Add a class-level flag so the base test adapts assertions accordingly. Signed-off-by: Chang Su <chang.s.su@oracle.com>
…alize call Use get_or_insert_with + push_str for delta accumulation instead of repeated string concatenation. Reuse finalized analysis from the first finalize() call instead of calling it a second time. Signed-off-by: Chang Su <chang.s.su@oracle.com>
TRT-LLM requires pre-tokenized stop sequences (TokenSequence), unlike SGLang/vLLM which accept stop strings natively. User-provided stop sequences (e.g. stop=[","]) were silently ignored for Harmony + TRT-LLM because neither the Rust StopSequenceDecoder (incompatible with StreamableParser) nor the backend handled them. Use the Harmony encoding's tokenizer to encode user stop strings and inject them as TokenSequence entries alongside the Harmony protocol stop tokens. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
This reverts commit bf0d1e4. Signed-off-by: Chang Su <chang.s.su@oracle.com>
CatherineSue
force-pushed
the
keyang/fix-oss-test
branch
from
March 4, 2026 02:28
bb712e1 to
ade6616
Compare
2 of 3 tasks
This was referenced Mar 17, 2026
2 of 4 tasks
This was referenced May 23, 2026
Closed
2 of 4 tasks
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
2 participants
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.
Description
Problem
Fixes: #575
matched_stopwas threaded through the parser'sparse_complete/finalizesignatures and stored inHarmonyChannelOutput, but callers already have access to it directly from the proto wrapper. This was unnecessary passthrough.In
parse_chunk, when multiple tokens in a single chunk produce deltas for the same channel, only the last delta survived (overwrite instead of accumulate). Additionally,has_deltawas tracked via a mutable flag set in multiple places instead of being derived from actual output.The gpt-oss (Harmony) stop sequence tests asserted that stop sequences are trimmed from output, but Harmony's
StreamableParserdoes not expose per-channel token IDs, so token-level stop trimming (likeStopSequenceDecoderdoes for the regular path) is not possible.Solution
Remove
matched_stopfromparse_complete/finalizesignatures andHarmonyChannelOutput. Callers now readmatched_stopdirectly from the proto wrapper.Accumulate deltas across tokens in
parse_chunkusingget_or_insert_with(String::new).push_str()and derivehas_deltafrom whether any channel produced output.Add
STOP_SEQUENCE_TRIMMEDclass flag to E2E tests. Regular backends assert stop sequences are absent; Harmony asserts the stop sequence is a suffix (present but at the end).Changes
parser.rs: Removematched_stopparam fromparse_complete()/finalize(); fix multi-token delta accumulation inparse_chunk()withget_or_insert_with; derivehas_deltafrom outputprocessor.rs: Simplifyparse_complete()calls to 2 args; remove unusedmatched_stopvariable in Responses pathstreaming.rs: Removematched_stopfromfinalize()calls; avoid redundantfinalize()call by reusing saved analysis; use direct move instead ofclone_fromforaccumulated_tool_callstypes.rs: Removematched_stopfield fromHarmonyChannelOutput; remove unusedserde_json::Valueimporttest_openai_server.py: AddSTOP_SEQUENCE_TRIMMEDflag (Truefor regular,Falsefor Harmony)Test Plan
cargo buildpassescargo clippy -- -D warningspassesSTOP_SEQUENCE_TRIMMEDflag