-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update dynamic flows to specify transition_callback within the function definition #84
Conversation
…ion_callbacks from FlowManager
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@filipi87 I ended up making this change for the reasons I outlined in the description. What do you think of this solution vs the one on main that you approved yesterday, #82? I'm hoping to make the FlowManager args simpler and co-locate node information in the node itself. But, I want to make sure this is a positive experience for developers. |
I think both approaches are easy to understand and provide a good developer experience. As you outlined here, this one has the advantage of mimicking the static flow format. So, I think it makes sense for us to proceed with this approach. |
Hi @markbackman, This might not be directly related to this PR, but I wanted to mention it here in case it's worth discussing. While reviewing our examples, I noticed that for each provider (OpenAI, Gemini, and Anthropic), the functions are declared differently inside the examples. Because of this, we need to handle them differently in Shouldn't this logic be encapsulated within the adapters? Regardless of the provider being used, shouldn't we declare the functions in the same way ? How we are currently declaring the functions for each provider in the examples: OpenAI:
Gemini:
Anthropic:
|
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 PR looks great.
I’ve left a comment about establishing a standard for how we declare functions, regardless of the provider. If we think it’s worthwhile, we could consider implementing this in a follow-up PR. 🙂
You raise a good point. For messages, Pipecat has aligned with the OpenAI format, but for function calling, there are enough differences that Pipecat still requires the native function calling format. This has the inconvenient side effect of requiring these types of differences to be used. I think this is really an issue we should take up in the core Pipecat code; that is, we should determine if we could standardize on a single format. When we last discussed (Q4 '24), we decided that there were still too many differences to handle. @filipi87 given this, any issue with me merging this change? |
No issue at all. We can discuss the other topic completely independently of this PR. |
In switching to a per function handler, we have the opportunity to mimic the static flow format of including the transition callback within the node itself. This change does exactly that;
transition_callback
is now defined within the function itself, matching the implementation for static flows.