Insert Instructions at the end of System Prompt #3614
Conversation
DouweM
left a comment
DouweM
left a comment
There was a problem hiding this comment.
@siddhantbhagat8 Thanks Siddhant! Please look at my notes and add a test in test_agent.py, by using both system_prompt and instructions decorators on one agent
| """Just maps a `pydantic_ai.Message` to a `cohere.ChatMessageV2`.""" | ||
| cohere_messages: list[ChatMessageV2] = [] | ||
| system_prompt_count = sum( | ||
| 1 for m in messages if isinstance(m, ModelRequest) for p in m.parts if isinstance(p, SystemPromptPart) |
There was a problem hiding this comment.
This will result in a wrong index when the system prompt parts are not all of the beginning, and it seems brittle to use an index derived from Pydantic AI messages in a list named cohere_messages, which may or may not map 1:1. So I think we should count system parts at the start of the actual cohere_messages list instead
| """Just maps a `pydantic_ai.Message` to a `groq.types.ChatCompletionMessageParam`.""" | ||
| groq_messages: list[chat.ChatCompletionMessageParam] = [] | ||
| system_prompt_count = sum( | ||
| 1 for m in messages if isinstance(m, ModelRequest) for p in m.parts if isinstance(p, SystemPromptPart) |
| """Just maps a `pydantic_ai.Message` to a `huggingface_hub.ChatCompletionInputMessage`.""" | ||
| hf_messages: list[ChatCompletionInputMessage | ChatCompletionOutputMessage] = [] | ||
| system_prompt_count = sum( | ||
| 1 for m in messages if isinstance(m, ModelRequest) for p in m.parts if isinstance(p, SystemPromptPart) |
| ) -> list[MistralMessages]: | ||
| """Just maps a `pydantic_ai.Message` to a `MistralMessage`.""" | ||
| mistral_messages: list[MistralMessages] = [] | ||
| system_prompt_count = sum( |
| for message in messages: | ||
| for part in message.parts: | ||
| if isinstance(part, SystemPromptPart): | ||
| system_prompt_count += 1 |
| assert isinstance(instructions, str) | ||
| openai_messages.insert(0, responses.EasyInputMessageParam(role='system', content=instructions)) | ||
| system_prompt_count = sum( | ||
| 1 for m in messages if isinstance(m, ModelRequest) for p in m.parts if isinstance(p, SystemPromptPart) |
There was a problem hiding this comment.
And here as well. We should find the right index in the actual openai_messages list
| assert run.all_messages_json().startswith(b'[{"parts":[{"content":"Hello",') | ||
|
|
||
|
|
||
| def test_instructions_inserted_after_system_prompt(): |
There was a problem hiding this comment.
@DouweM Even though I did write this test I am not confident that this test actually tests the intended functionality.
The PR changes how providers internally combine the system prompt parts and instructions but the pydantic_ai message structure remains unchanged, i.e., instructions remains a separate field on Agent whereas system_prompt is in a SystemPromptPart in the messages
I tried to find a similar test in this file for inspiration, but could not find one
Should I instead try to write tests in provider specific test files as there might be a more robust way of testing it there due to the existing mocking patterns? Or do you have another suggestion?
I believe I have addressed the core logic feedback as per my understanding :) |
|
@DouweM Sorry for bothering you but would love to get this over the line! |
2 participants
Description
Changed the insertion logic to insert instructions after all system prompt parts
Simple append:
anthropic.pygoogle.pygemini.pybedrock.pyInsert at a particular position:
For models that build a messages list, count
SystemPromptPartinstances and insert at that indexopenai.pycohere.pygroq.pymistral.pyhuggingface.pyTesting
I ran the tests locally, everything passed. If I should add a test for this change, please let me know the correct location for the test :)