Skip to content

Conversation

@anuraaga
Copy link
Collaborator

@anuraaga anuraaga commented Apr 10, 2025

Same implementation as vercel-ai

image

@anuraaga anuraaga marked this pull request as draft April 10, 2025 05:35
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

mind doing stdio so that the impl matches the flow?


For example, to run with Docker:
```bash
docker compose run --build --rm genai-function-calling -- --mcp
Copy link
Contributor

Choose a reason for hiding this comment

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

I think..

Suggested change
docker compose run --build --rm genai-function-calling -- --mcp
docker compose run --build --rm genai-function-calling --mcp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops yeah just copied this from vercel-ai without testing it. I think it may need the CMD -> ENTRYPOINT change

@@ -1,5 +1,6 @@
openai-agents~=0.0.8
Copy link
Contributor

Choose a reason for hiding this comment

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

mind checking latest doesn't break this?

@anuraaga anuraaga marked this pull request as ready for review April 10, 2025 05:51
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

Hmm I think this demonstrates that SSE is worse than stdio wrt the propagation concern. We can still merge it, but I'd go back and try to change it to stdio later. We should take the lessons about SSE with us as we progress MCP propagation.

SG?

mcp_server = FastMCP(log_level="WARNING")
for tool in tools:
mcp_server.add_tool(tool)
# Mysteriously, cleanup such as from opentelemetry-instrument does not run on exit
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry absolutely no clue what's going on here

@anuraaga
Copy link
Collaborator Author

Went ahead and switched to stdio

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

great job with pragmatic workarounds!

@codefromthecrypt codefromthecrypt merged commit 15e74b4 into elastic:main Apr 10, 2025
3 checks passed
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.

2 participants