-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Add instructions parameter in response object #3741
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
feat: Add instructions parameter in response object #3741
Conversation
Hi @s-akhtar-baig! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
"type": "string", | ||
"description": "(Optional) Truncation strategy applied to the response" | ||
}, | ||
"instructions": { |
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.
openai's docs say this can be a string or an array
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.
Thanks, @ashwinb, for reviewing! I have updated the definition using openai/types/responses as my reference. Please take a look when you get a chance.
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.
Interesting that this shows a String only but the Python client is definitely the source of truth!
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.
FWIW, I tried this with the Python client talking directly to OpenAI (not Llama Stack):
r3 = open_ai_client.responses.create(
model="gpt-4o",
input="What is the capital of France?",
instructions=["Always answer with rhyming poetry.", "Include the word 'cat' in your answer."]
)
What I got was:
BadRequestError: Error code: 400 - {'error': {'message': "Invalid type for 'instructions': expected a string, but got an array instead.", 'type': 'invalid_request_error', 'param': 'instructions', 'code': 'invalid_type'}}
So the OpenAI client does indeed accept the list and send it off to the server but then the server rejected it because the server API expects a string only. So I guess the API reference that @leseb links to above is correct.
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.
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.
So is the conclusion that this PR is fine the way it is? (i.e., that Llama Stack should really just accept a string and not a list for instructions)
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.
With these changes, we will be conforming to the OpenAI spec:
- create response accepts instructions as a string
- response object can store either a string or an array
- instructions from previous response is not carried over
But, I am not sure whether that's what we want in Llama Stack or not. If we change create response to accept both a string and an array, then we won't be conforming to the spec and we don't have use-cases (that I can think of) to correctly implement logic that will handle array data type.
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.
In [7]: from openai import OpenAI; client = OpenAI()
In [8]: openai.__version__
Out[8]: '1.107.0'
In [9]: client.responses.create.__annotations__
Out[9]:
{'background': 'Optional[bool] | NotGiven',
'conversation': 'Optional[response_create_params.Conversation] | NotGiven',
'include': 'Optional[List[ResponseIncludable]] | NotGiven',
'input': 'Union[str, ResponseInputParam] | NotGiven',
'instructions': 'Optional[str] | NotGiven',
'max_output_tokens': 'Optional[int] | NotGiven',
'max_tool_calls': 'Optional[int] | NotGiven',
'metadata': 'Optional[Metadata] | NotGiven',
'model': 'ResponsesModel | NotGiven',
'parallel_tool_calls': 'Optional[bool] | NotGiven',
'previous_response_id': 'Optional[str] | NotGiven',
'prompt': 'Optional[ResponsePromptParam] | NotGiven',
'prompt_cache_key': 'str | NotGiven',
'reasoning': 'Optional[Reasoning] | NotGiven',
'safety_identifier': 'str | NotGiven',
'service_tier': "Optional[Literal['auto', 'default', 'flex', 'scale', 'priority']] | NotGiven",
'store': 'Optional[bool] | NotGiven',
'stream': 'Optional[Literal[False]] | Literal[True] | NotGiven',
'stream_options': 'Optional[response_create_params.StreamOptions] | NotGiven',
'temperature': 'Optional[float] | NotGiven',
'text': 'ResponseTextConfigParam | NotGiven',
'tool_choice': 'response_create_params.ToolChoice | NotGiven',
'tools': 'Iterable[ToolParam] | NotGiven',
'top_logprobs': 'Optional[int] | NotGiven',
'top_p': 'Optional[float] | NotGiven',
'truncation': "Optional[Literal['auto', 'disabled']] | NotGiven",
'user': 'str | NotGiven',
'extra_headers': 'Headers | None',
'extra_query': 'Query | None',
'extra_body': 'Body | None',
'timeout': 'float | httpx.Timeout | None | NotGiven',
'return': 'Response | Stream[ResponseStreamEvent]'}
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 looks like the client (as mentioned by @s-akhtar-baig) only lists instructions
as a str
5f503d6
to
2ce56ab
Compare
# Omit instructions from previous response | ||
previous_instructions = previous_response.instructions | ||
if previous_instructions: | ||
assert isinstance(previous_instructions, str) |
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.
@ashwinb and @leseb, I'd appreciate your thoughts on this:
This condition will ensure that instructions is handled and stored as a string for the time being. Because the spec for create response specifies instructions as a string, and I am not sure what the use case would be for storing instructions as an array. Please let me know if I am missing anything.
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.
My take on this is that if the spec say str | list, we should also update the create response.
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.
Thanks, @leseb, for reviewing! I have handled other comments and added tests and recordings. Please take a look when you get a chance.
My take on this is that if the spec say str | list, we should also update the create response.
This makes sense! But the thing that comes to mind is that without use cases for other types, I am not sure if we can handle those types correctly, for the time being.
llama_stack/providers/inline/agents/meta_reference/responses/openai_responses.py
Outdated
Show resolved
Hide resolved
# Omit instructions from previous response | ||
previous_instructions = previous_response.instructions | ||
if previous_instructions: | ||
assert isinstance(previous_instructions, str) |
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.
My take on this is that if the spec say str | list, we should also update the create response.
llama_stack/providers/inline/agents/meta_reference/responses/openai_responses.py
Outdated
Show resolved
Hide resolved
125aa75
to
3487d0a
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.
The logic here looks correct to me.
5dc97a0
to
a615f7b
Compare
tools: list[OpenAIResponseTool] | None = None | ||
truncation: str | None = None | ||
usage: OpenAIResponseUsage | None = None | ||
instructions: str | list[OpenAIResponseInput] | 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.
based on all the discussion so far, I think we should just do what you did first @s-akhtar-baig -- which is that this is just a simple str
, that's it. this specific type is not good -- you really are not going to accept arbitrarily complex input as instructions here.
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.
Thanks, @ashwinb!
The tests have passed, can you please merge when you get a chance? Thanks again!
a615f7b
to
f7b6d7f
Compare
Problem
The current inline provider appends the user provided instructions to messages as a system prompt, but the returned response object does not contain the instructions field (as specified in the OpenAI responses spec).
What does this PR do?
This pull request adds the instruction field to the response object definition and updates the inline provider. It also ensures that instructions from previous response is not carried over to the next response (as specified in the openAI spec).
Closes #3566
Test Plan