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

Update dynamic flows to use a per-function transition handler, support node functions #82

Merged
merged 8 commits into from
Jan 22, 2025

Conversation

markbackman
Copy link
Contributor

Response to this issue: #77.

Taking a chance to improve the architecture and dev ex, though that introduces a breaking change.

Copy link

vercel bot commented Jan 21, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
pipecat-flows ✅ Ready (Inspect) Visit Preview Jan 22, 2025 1:08am

Copy link

@muranski muranski left a comment

Choose a reason for hiding this comment

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

Thank you very much for the super quick response and code change! In the comment, I propose an alternative way of specifying the per-function transition handler. It's only a cosmetic change, but in my opinion, it makes the code more readable.


# After - per-function transitions
flow_manager = FlowManager(
transition_callbacks={

Choose a reason for hiding this comment

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

As an alternative, I would consider specifying the per-function transition handler directly in the function definition code, similar to how static flow management specifies the "transition_to" field.

For example:

def create_marital_status_node() -> NodeConfig:
    """Create node for collecting marital status."""
    return {
        "task_messages": [
            {
                "role": "system",
                "content": "Ask about the customer's marital status for premium calculation.",
            }
        ],
        "functions": [
            {
                "type": "function",
                "function": {
                    "name": "collect_marital_status",
                    "handler": collect_marital_status,
                    "description": "Record marital status",
                    "parameters": {
                        "type": "object",
                        "properties": {
                            "marital_status": {"type": "string", "enum": ["single", "married"]}
                        },
                        "required": ["marital_status"],
                    },
                },
                "transition_callback": handle_marital_status_collection,
            }
        ],
    }

I prefer this approach because it is more consistent with the static flow management style.

Copy link
Contributor Author

@markbackman markbackman Jan 22, 2025

Choose a reason for hiding this comment

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

I'm considering this. I'm going to merge this change, then I'll open a new PR to see how making the change you suggested looks.

@@ -253,7 +254,7 @@ async def get_movies() -> Union[MoviesResult, ErrorResult]:
return MoviesResult(movies=movies)
except Exception as e:
logger.error(f"TMDB API Error: {e}")
return ErrorResult(error="Failed to fetch movies")
return ErrorResult(status="error", error="Failed to fetch movies")

Choose a reason for hiding this comment

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

Do we need to apply these same changes in the example below, movie_explorer_gemini.py
? Include the status="error" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Fixed.

Copy link

@filipi87 filipi87 left a comment

Choose a reason for hiding this comment

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

One minor comment, but other than that all looks good.
Nice improvement. 🚀

@markbackman markbackman merged commit 2fde1cf into main Jan 22, 2025
2 checks passed
@markbackman markbackman deleted the mb/dynamic-node-functions branch January 22, 2025 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants