-
Notifications
You must be signed in to change notification settings - Fork 109
Support analysis channel in gptoss renderer #51
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
Support analysis channel in gptoss renderer #51
Conversation
|
Thanks for the PR! That's according to this, and also that's what I get in the HF chat template for this example (If you can also add a version of test_against_hf_chat_templates that tests the thinking it would be great too) Also, if you can add an assert in the other render_message functions that the new message.thinking field is None, at least if/until we support those there too (Also maybe worth asserting that it's None also in gpt-oss if it's a user message) |
|
According to @opherlieber's comment, the gpt-oss thinking block is in its own Message. So are you sure we want thinking as a separate attribute of the Message, as opposed to just using the content field? That said, having thinking as a separate field of the Message could actually make sense for models like the Qwen3 ones where every message includes thinking followed by text. Currently, for these models, content field includes the tags rendered as text, which requires parsing the string, and could lead to prompt injection issues. |
|
It's worth looking at other libraries like https://github.com/UKGovernmentBEIS/inspect_ai, which work with various models with different thinking conventions, to see how they deal with these issues. |
|
I think it might make sense to have the channel as a separate message in this specific gpt-oss case, but might still be preferable to have it in a single message like the current PR does since:
|
|
Thanks for the review @opherlieber @joschu! I was probably oversimplifying for the gpt-oss case a bit. I based this off the gpt-oss chat template and fine-tuning cookbook where It looks like Inspect used to do the same, but switched to defining a ContentReasoning type alongside other Content blocks, e.g. ContentText, ContentReasoning, ContentToolUse, ContentImage, etc. (They changed it to support multiple fields in the reasoning content as with Anthropic’s API) In that case, the reasoning is still part of the same message, but a separate content block. Happy to try something more like that as 1) I like the extensibility for things like multimodal content, 2) user messages won’t have a “thinking” field so might make more sense to not have that explicitly defined on the message (or else to define a separate subclass for assistant messages like they also do in Inspect), 3) and like you said, it might fit nicely with some of the other models where the tags are currently part of the content field. So for now my thinking is either 1) have reasoning in a separate field like this or 2) define a separate Content block type similar to Inspect and allow the content field to be an array of such but let me know what you prefer! Happy to also include changes for other renders in this PR if it makes sense to see it across the board before approving. |
|
jinx, what @opherlieber said :) |
Add support for CoT in the gptoss renderer based on https://cookbook.openai.com/articles/gpt-oss/fine-tune-transfomers and https://cookbook.openai.com/articles/gpt-oss/handle-raw-cot
An example like this:
Now renders to the harmony format like this:
Left commentary channel / tooling for a separate PR.