[Bugfix] gpt-oss container tool output bug #25485
Conversation
Signed-off-by: Alec Solder <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where reasoning output for the container tool could cause a crash if sent to the 'commentary' channel. The fix correctly handles this case, aligning its behavior with other tools like 'python' and 'browser'. My review includes a suggestion to refactor the conditional logic for improved readability and maintainability.
| elif recipient is not None and (recipient.startswith("python") | ||
| or recipient.startswith("browser")): | ||
| or recipient.startswith("browser") | ||
| or recipient.startswith("container")): |
There was a problem hiding this comment.
The conditional logic is becoming a bit long with the addition of the 'container' tool. To improve readability and maintainability, you can leverage the fact that str.startswith() accepts a tuple of prefixes. This makes the code more concise and easier to update in the future.
elif recipient is not None and recipient.startswith(("python", "browser", "container")):
There was a problem hiding this comment.
probably need a better way to check this lol any better idea?
There was a problem hiding this comment.
Wow, just notice that str.startswith supports passing in a tuple of prefixes. TIL.
qandrew
left a comment
qandrew
left a comment
There was a problem hiding this comment.
could you provide repro instructions in the description, like what prompts caused this?
| elif recipient is not None and (recipient.startswith("python") | ||
| or recipient.startswith("browser")): | ||
| or recipient.startswith("browser") | ||
| or recipient.startswith("container")): |
There was a problem hiding this comment.
probably need a better way to check this lol any better idea?
github-project-automation
bot
moved this from To Triage
to Ready
in gpt-oss Issues & Enhancements
github-actions
bot
added
the
ready
| elif recipient is not None and (recipient.startswith("python") | ||
| or recipient.startswith("browser")): | ||
| or recipient.startswith("browser") | ||
| or recipient.startswith("container")): |
There was a problem hiding this comment.
Non-blocking: I saw "python", "browser", "container" are flooded around the code, could we predefine it as a variable / named literal or enum to avoid typo?
| output_items.append(response_item) | ||
| elif recipient is not None and (recipient.startswith("python") | ||
| or recipient.startswith("browser")): | ||
| or recipient.startswith("browser") |
There was a problem hiding this comment.
non-blocking: The function is getting quite complicated, can we add unit test to protect the behavior?
|
For @yeqcharlotte @Jialin , I completely agree that the logic is getting scary with the different conditions based on the tool names. This code path is also not correct in the first place because messages here are returned as reasoning messages, but are actually tool calls. So basically this whole flow needs a refactor, I think that is best done once we begin adding support for doing server side execution of "function tools" in general. The main challenge here is that there needs to be model specific model to handle certain tools in a special way, which we don't have built out, but definitely shouldn't be part of this PR. |
| elif recipient is not None and (recipient.startswith("python") | ||
| or recipient.startswith("browser")): | ||
| or recipient.startswith("browser") | ||
| or recipient.startswith("container")): |
There was a problem hiding this comment.
Wow, just notice that str.startswith supports passing in a tuple of prefixes. TIL.
github-project-automation
bot
moved this from Ready
to Done
in gpt-oss Issues & Enhancements
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]>
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]> Signed-off-by: yewentao256 <[email protected]>
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]> Signed-off-by: gaojc <[email protected]>
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]>
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]>
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Signed-off-by: Alec Solder <[email protected]> Co-authored-by: Alec Solder <[email protected]>
4 participants
Purpose
Fix a bug related to container tool reasoning output. Occasionally it will output container tool to commentary channel.
Test Plan
Ran serving with container tool enabled.
Test Result
No longer crashes when creating output reasoning messages for container tool.