-
-
Notifications
You must be signed in to change notification settings - Fork 12.4k
[gpt-oss] Add Tool/ConversationContext classes and harmony_utils #22340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Woosuk Kwon <[email protected]>
There was a problem hiding this comment.
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 introduces new classes for managing conversation context and tools, along with utility functions. The changes are generally well-structured, but there are a few areas that need attention. I've identified a critical bug that could cause a crash, along with several high-severity issues related to code duplication and non-deterministic behavior that impact maintainability and testability. Addressing these points will improve the robustness and quality of the new components.
| last_n = -1 | ||
| to_process = [] | ||
| while rendered_tokens[last_n] != self.last_tok: | ||
| to_process.append(rendered_tokens[last_n]) | ||
| last_n -= 1 | ||
| for tok in reversed(to_process): | ||
| self.parser.process(tok) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The while loop condition rendered_tokens[last_n] != self.last_tok is unsafe and can lead to an IndexError if self.last_tok is not present in rendered_tokens. This can occur if append_output hasn't been called, leaving self.last_tok as None, or if token generation logic changes. The loop needs a boundary check to prevent it from accessing an invalid index.
if self.last_tok is not None:
try:
# Find the index of the last token from the end.
last_tok_r_idx = rendered_tokens[::-1].index(self.last_tok)
last_tok_idx = len(rendered_tokens) - 1 - last_tok_r_idx
# Process tokens that appear after the last_tok
for tok in rendered_tokens[last_tok_idx + 1:]:
self.parser.process(tok)
except ValueError:
# self.last_tok not found in rendered_tokens.
# The original code would crash.
passThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @simon-mo
| if start_date is None: | ||
| start_date = datetime.datetime.now().strftime("%Y-%m-%d") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using datetime.datetime.now() for a default start_date introduces non-determinism, which can make testing difficult and lead to flaky tests. It's better to use a fixed default date to ensure deterministic behavior, or require the caller to provide the date explicitly.
| if start_date is None: | |
| start_date = datetime.datetime.now().strftime("%Y-%m-%d") | |
| if start_date is None: | |
| start_date = "2024-01-01" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think this is something we should remember and be careful about. No immediate fix though.
Signed-off-by: Woosuk Kwon <[email protected]>
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]> Signed-off-by: Noam Gat <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]> Signed-off-by: Paul Pak <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
…m-project#22340) Signed-off-by: Woosuk Kwon <[email protected]> Co-authored-by: LiuXiaoxuanPKU <[email protected]> Co-authored-by: simon-mo <[email protected]> Co-authored-by: Chen Zhang <[email protected]> Co-authored-by: Hongxia Yang <[email protected]> Co-authored-by: Minseok Lee <[email protected]> Co-authored-by: Yongye Zhu <[email protected]>
This PR does not include MCP yet. cc @heheda12345