Skip to content
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

Python: Update sort step method for assistant invoke. #10191

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moonbox3
Copy link
Contributor

Motivation and Context

At times, when the (Azure) OpenAI Assistant Agent makes a tool call, that tool call's creation timestamp is coming after the message creation timestamp (the message creation being the text that the assistant responds with). Currently in our code, if we have a tool call (FunctionCallContent), we first yield that, and then we make a call to get completed steps, to then yield more content like FunctionResultContent and TextContent. There will be two steps (or more, depending upon the number of tool calls).

Right now we sort the completed steps in this way:

completed_steps_to_process: list[RunStep] = sorted(
    [s for s in steps if s.completed_at is not None and s.id not in processed_step_ids],
    key=lambda s: s.created_at,
)

When there are no failures, it's because the tool call was created before the final message content (as has been since this assistant was first coded). However, it appears that processing on the server-side can cause fluctuations in when the steps are created/processed. When we have a failure, we now have the message_creation (TextContent) yielded before the FunctionResultContent, which if sent to an (Azure) OpenAI Chat Completion endpoint will break with a 400 because of the gap in the ordering between the FunctionCallContent and the FunctionResultContent:

FunctionCallContent
TextContent # this should follow `FunctionResultContent` (and it does during times when we don't see a 400)
FunctionResultContent

The 400 error isn't 100% repeatable because of server-side processing, so we will get the correct ordering:

FunctionCallContent
FunctionResultContent
TextContent

This PR updates the ordering of how we sort of the completed steps -- if we have a step_type == "tool_calls" and "message_creation", we sort to allow for "tool_calls" to come first, and any ties are broken by the step.completed_at timestamp. With this update, we no longer receive 400s because the ordering of the content types is correct when sending to an OpenAI model.

Description

This PR:

Contribution Checklist

@moonbox3 moonbox3 self-assigned this Jan 15, 2025
@moonbox3 moonbox3 requested a review from a team as a code owner January 15, 2025 11:02
@markwallace-microsoft markwallace-microsoft added the python Pull requests for the Python Semantic Kernel label Jan 15, 2025
@markwallace-microsoft
Copy link
Member

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
semantic_kernel/agents/open_ai
   open_ai_assistant_base.py480898%255, 341–342, 752, 916, 919, 993, 1057
TOTAL16709177889% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
3000 4 💤 0 ❌ 0 🔥 1m 12s ⏱️

Copy link
Member

@eavanvalkenburg eavanvalkenburg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add some unit tests for this mechanism?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests for the Python Semantic Kernel
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Python: Bug: Kernel Function plugin not working with AzureAssistantAgent
3 participants