[gpt-oss] Harmony changes with container tool support

[morgendave]

Purpose

• Support system prompt customization via injections
• remove developer messages if all tools enabled are builtin tools
• support container tool
• support context with session id on MCP
• support MCP session clean up for container

Test Plan

Tested on oss gpt with evals, sample requests
Jupyter python tool for AIME25 is slightly lower than model card

Test Result

OSS tested
Parsed messages

Harmony messages: [Message(author=Author(role=<Role.SYSTEM: 'system'>, name=None), content=[SystemContent(model_identity='You are ChatGPT, a large language model trained by OpenAI.\nRespond in Chinese.', reasoning_effort=<ReasoningEffort.MEDIUM: 'Medium'>, conversation_start_date='2025-08-14', knowledge_cutoff='2024-06', channel_config=ChannelConfig(valid_channels=['analysis', 'final'], channel_required=True), tools={'browser': ToolNamespaceConfig(name='browser', description='Tool for browsing.\nThe `cursor` appears in brackets before each browsing display: `[{cursor}]`.\nCite information from the tool using the following format:\n`【{cursor}†L{line_start}(-L{line_end})?】`, for example: `【6†L9-L11】` or `【8†L3】`. \nDo not quote more than 10 words directly from the tool output.\nsources=web', tools=[ToolDescription(name='search', description='web search tool', parameters={'properties': {'query': {'type': 'string'}}, 'required': ['query'], 'type': 'object'})])})], channel=None, recipient=None, content_type=None), Message(author=Author(role=<Role.DEVELOPER: 'developer'>, name=None), content=[DeveloperContent(instructions=None, tools=None)], channel=None, recipient=None, content_type=None), Message(author=Author(role=<Role.USER: 'user'>, name=None), content=[TextContent(text="When is Alan Turing's birthday?")], channel=None, recipient=None, content_type=None)]

AIME25 high

Writing report to /tmp/aime25_120b-high_temp1.0_20250814_093031.html
{'chars': 2442.35, 'chars:std': 1036.7757283199358, 'score': 0.9458333333333333, 'score:std': 0.22634628092568448}

[gemini-code-assist]

Code Review

This pull request introduces support for container tools, system prompt customization, and session management improvements for Harmony. The changes look good overall, but I've identified a couple of high-severity issues. One is a design flaw in ConversationContext where a method signature in a subclass is incompatible with its abstract base class, violating the Liskov Substitution Principle. The other is a bug in get_system_message that can lead to an incorrect system prompt when instructions are provided without a base model identity. I've provided suggestions to fix both issues.

[gemini-code-assist]

The signature of cleanup_session in the abstract base class ConversationContext is async def cleanup_session(self) -> None:. However, the implementation in HarmonyContext is async def cleanup_session(self, *args, **kwargs) -> None:. This violates the Liskov Substitution Principle, as the subclass method has a different signature.

While Python's dynamic nature might allow this at runtime, it's a design issue that can be flagged by static analysis tools and lead to confusion. The *args, **kwargs are necessary because AsyncExitStack.push_async_exit can pass exception details to the cleanup function.

To fix this, the signature in the base class and all implementing classes should be consistent. Please update this abstract method and the implementation in SimpleContext to match the one in HarmonyContext.

Suggested change

	@abstractmethod
	async def cleanup_session(self) -> None:
	pass
	@abstractmethod
	async def cleanup_session(self, *args, **kwargs) -> None:
	pass

[gemini-code-assist]

There is a potential bug here. If model_identity is None (its default) and instructions are provided, sys_msg_content.model_identity will be None. Consequently, current_identity will be None, and the new model identity will be set to the string "None\n<instructions>", which is likely not the intended behavior.

You should handle the case where current_identity is None to avoid prepending the string "None".

Suggested change

	current_identity = sys_msg_content.model_identity
	sys_msg_content = sys_msg_content.with_model_identity(
	f"{current_identity}\n{instructions}")
	current_identity = sys_msg_content.model_identity
	new_identity = f'{current_identity}\n{instructions}' if current_identity else instructions
	sys_msg_content = sys_msg_content.with_model_identity(new_identity)

[cadedaniel]

What is AIME25 high without this PR?

[morgendave]

What is AIME25 high without this PR?

With tools it is similar. The repro of numbers are not very stable with tools actually
One of the thing this made better is that the commentary channel showed up less when it shouldn't be

[yeqcharlotte]

we should not change the default behavior.

using container tool as built-in should be opt-in through some environment variable. and ultimately we should get rid of this hack by making system prompt easier to modify so vllm is more friendly for research use case.

[heheda12345]

+1. I'm also getting other feature requests to customize system prompt like #23167 so I think we need to allow users to do that in some ways.

[heheda12345]

what is container tool?

[heheda12345]

For the session cleanup logic, maybe just add it here?

[morgendave]

Right now tool sessions are not managed in tool sever lifecycle but with context lifecycle so it has to be attached there unless we do a refactor

[heheda12345]

But I think regardless of exit_stack.push_async_exit(context.cleanup_session) or implementing the exist logic here, the cleanup will be called when the code goes out the async with AsyncExitStack() as exit_stack: block?

And I thought different requests should have difference session so tool sessions should not be managed in tool server lifecycle. Is this understanding correct?

[morgendave]

This is in tool server, everything should be managed in context as it as the actual management of sessions and per request based

[heheda12345]

+1. I'm also getting other feature requests to customize system prompt like #23167 so I think we need to allow users to do that in some ways.

[Ithanil]

If I'm not mistaken, this basically covers #23167 , so if the present PR gets merged, mine is obsolete.

[heheda12345]

Suggested change

	"VLLM_USE_CONTAINER_TOOL":
	lambda: bool(int(os.getenv("VLLM_USE_CONTAINER_TOOL", "0"))),
	# Allows harmony instructions to be injected on system messages
	"VLLM_HARMONY_SYSTEM_INSTRUCTIONS":
	lambda: bool(int(os.getenv("VLLM_HARMONY_SYSTEM_INSTRUCTIONS", "0"))),
	"VLLM_GPT_OSS_USE_CONTAINER_TOOL":
	lambda: bool(int(os.getenv("VLLM_GPT_OSS_USE_CONTAINER_TOOL", "0"))),
	# Allows harmony instructions to be injected on system messages
	"VLLM_GPT_OSS_HARMONY_SYSTEM_INSTRUCTIONS":
	lambda: bool(int(os.getenv("VLLM_GPT_OSS_HARMONY_SYSTEM_INSTRUCTIONS", "0"))),

Feel that these logic are heavily hardcoded to gpt-oss. What about adding a GPT_OSS prefix?

[morgendave]

Sure that might be better. Let me change on both sides

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @morgendave.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[heheda12345]

1. Given MCP + stream generator is supported, can you also update responses_stream_generator?
2. I think we still need discussion about session cleanup logic. Let's sync offline.
3. Can you add VLLM_GPT_OSS_USE_CONTAINER_TOOL and VLLM_GPT_OSS_HARMONY_SYSTEM_INSTRUCTIONS to the recipe?

[houseroad]

add some comment about to explain the tools? What's input and output expectation?

[houseroad]

should be something like raise NotImplementedError("Should not be called.")? So we can force users carefully implement cleanup logic.

[facebook-github-bot]

@lacora2017 has imported this pull request. If you are a Meta employee, you can view this in D81562641.

[houseroad]

Some minor comment to address.

[morgendave]

1. Given MCP + stream generator is supported, can you also update responses_stream_generator?
2. I think we still need discussion about session cleanup logic. Let's sync offline.
3. Can you add VLLM_GPT_OSS_USE_CONTAINER_TOOL and VLLM_GPT_OSS_HARMONY_SYSTEM_INSTRUCTIONS to the recipe?

For 1 I checked the responses_stream_generator, the only change is in calling await context.init_tool_sessions which is included
For 2 I left a comment, basically the clean up is already for different requests and not managed by tool server as it couldn't
For 3 it is already in that PR

[yeqcharlotte]

@morgendave this should be VLLM_GPT_OSS_USE_CONTAINER_TOOL

[lacora]

@heheda12345 regarding cleanup if it's minor let's put it up as a followup for further discussions and not blocking this PR， could you or @morgendave summarize the offline discussion a bit so that we can broadly discuss as well？

[simon-mo]

Entrypoint failure seems related

[morgendave]

Entrypoint failure seems related

Which failure?I saw one GPU memory related only

[facebook-github-bot]

@lacora2017 has imported this pull request. If you are a Meta employee, you can view this in D81562641.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @morgendave.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[aarnphm]

I merged from main to this. Let's wait for CI to run (possibly flaky CI)

[houseroad]

The failed test seems related.

[lacora]

I think the failed test itself it's flaky not a problem with this PR.

Output is just asking if to use Celsius or Fahrenheit.
"ChatCompletionMessage(content='Sure! Would you like the temperature in Celsius or Fahrenheit?', refusal=None, role='assistant', annotations=None, audio=None, function_call=None, tool_calls=[], reasoning_content='We need to call the function get_current_weather with city "Dallas", state "TX", unit maybe default? The user didn't specify unit. We can ask for unit? But we can choose default. Probably ask for unit? The user didn't specify. We can ask: "Would you like Celsius or Fahrenheit?" But we can also default to Fahrenheit for US. Let's ask.')"

If we remove the option for the temperature in the tool itself the test will pass, verify multiple times.