-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(responses)!: add Prompts API to Responses API #3514
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
base: main
Are you sure you want to change the base?
feat(responses)!: add Prompts API to Responses API #3514
Conversation
ef753bc to
fe6ea4c
Compare
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.
this is an api change unrelated to how prompts are used in /v1/responses
please review your code assistant output before posting as a PR.
Hi @mattf ! Could you please elaborate on how the prompts should be used in Responses API in your opinion. My understanding was that they should be propagated to Agent’s messages context as OpenAISystemMessageParam |
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.
Hey @r3v5 it looks like you've suggested adding prompt_id here where you need to add a Prompt object with an id, version, and variables, which would then be consistent with OpenAI's client usage, as outlined here:
response = client.responses.create(
prompt={
"id": "pmpt_68b0c29740048196bd3a6e6ac3c4d0e20ed9a13f0d15bf5e",
"version": "2",
"variables": {
"city": "San Francisco",
"age": 30,
}
}
)So this is currently incorrect. As @mattf suggested, let's make sure we double check this. Thank you.
Oh yeah, this makes sense. I got it. I will adjust the implementation then |
fe6ea4c to
a3cdf78
Compare
| """ | ||
|
|
||
| id: str | ||
| version: str | None = None |
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.
Version has type string because OpenAI has it like string. Reference is here
|
@cdoern this is an enhancement to the /openai/v1/responses api, does it match the openai /v1/responses api spec? |
there seems to be some breaking changes, BUT these might have existed in main, let me check. |
a3cdf78 to
d76b15b
Compare
|
hey @cdoern any update on the main branch check here? |
fadf1d0 to
f474e0c
Compare
4175600 to
f37efb1
Compare
|
@r3v5 yeah additional tests/validation would be great with a working example. |
The PR description shows a working example of Prompt inside the Response create, but I'd like to see |
d63e31f to
b954305
Compare
|
@r3v5 unit tests are failing |
I just rebased commit from main today. Still haven't finished my implementation |
1660935 to
7a7b2b7
Compare
|
Hey @leseb , @franciscojavierarceo ! Here I provide a comprehensive testing of support Prompts in Responses API via curl requests to LLS server. Test Prompts with Images with text on them in Responses API: I used this image for testing purposes: iphone 17 image
Output after inferencing:
The same example but without providing the description of product: Output:
Test Prompts with PDF files in Responses API: I used this PDF file for testing purposes: invoicesample.pdf
Output after inferencing:
Test simple text Prompt in Responses API:
Output after inferencing:
|
The implementation is there :) |
|
sorry @r3v5 this keeps getting wrecked 😭 |
|
last rebase and i think we're good to go |
|
We haven't landed prompts API implementation have we? |
No worries, @franciscojavierarceo ! I will do rebase today. |
7a7b2b7 to
59169bf
Compare
I rebased from main, CI is green! |
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.
make sure to expand the title & description of this pr to match the expanded scope
also, make sure there is test coverage for new apis as they're used outside of prompts
I have updated PR description now. |
| output: list[OpenAIResponseOutput] | ||
| parallel_tool_calls: bool = False | ||
| previous_response_id: str | None = None | ||
| prompt: Prompt | None = None |
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.
but then is this object the full one?! and not the same object you created above?
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.
When instance of OpenAIResponseObject class is created, it correctly contains link to object of Prompt class. If user doesn't provide prompt during creation of response, then there is no link to any prompt.
The prompt params we use during creation of response, refer to OpenAIResponsePromptParam class that is the standing to handle different types of prompt's variables.
@json_schema_type
class OpenAIResponsePromptParam(BaseModel):
"""Prompt object that is used for OpenAI responses.
:param id: Unique identifier of the prompt template
:param variables: Dictionary of variable names to OpenAIResponseInputMessageContent structure for template substitution
:param version: Version number of the prompt to use (defaults to latest if not specified)
"""
id: str
variables: dict[str, OpenAIResponseInputMessageContent] | None = None
version: str | None = None
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.
Prompt class sits in Prompts API while OpenAIResponsePromptParam helper structure is defined in Agents API in apis/agents/openai_responses.py
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.
@r3v5 now I am not so sure. OpenAI's "Response object" doc (on their reference) says the prompt field within contains exactly three fields { id, variables, version }. On the other hand the Prompt field in our incantation of the prompts API (which is NOT part of the OpenAI API set) has the fields { prompt_id, version, variables, is_default, prompt }
This is a discrepancy -- at least the { id } field being a clear discrepancy but even referencing that other object we made up seems wrong. This is what @leseb brought up before.
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.
Got it. What do you think we can settle down with in terms of classes? Because as you said, now we have two classes for prompts on different API layers.
| backend="sql_default", | ||
| table_name="openai_conversations", | ||
| ).model_dump(exclude_none=True), | ||
| "prompts": SqlStoreReference( |
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.
this change should be a separate PR completely
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.
I think you should just separate this PR into three PRs
- first is the change I point out here
- second is the change to the API only -- no implementation at all
- implementation and tests
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.
Yeah, I see. I will do it. Should I create issues for each PR or just submit PRs?
|
I submitted the first PR of three |
What does this PR do?
The purpose of this PR is to integrate Prompts API to Responses API to achieve full OpenAI compatibility for current Responses API in Llama Stack.
OpenAIResponseInputMessageContentTextobjectOpenAIResponseInputMessageContentImageobjectOpenAIResponseInputMessageContentFileobjectThis is done to match OpenAI API specs. Reference can be found here
Closes #3321
Test Plan
Manual API testing and running newly added unit tests.
Prerequisites:
uv run --with llama-stack llama stack build --distro starter --image-type venv --runThe comprehensive testing can be found here