Skip to content

return proper error from agent instance to the client #493

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

return proper error from agent instance to the client #493

wants to merge 1 commit into from

Conversation

chuenlok
Copy link
Contributor

@chuenlok chuenlok commented Nov 20, 2024 •
edited
Loading

What does this PR do?

The assert errors are not returned to the client and hid by the 500 internal server error. Append the real error in the error message so the client can show it. Updates on the python client llamastack/llama-stack-client-python#44

Test Plan

Please describe:

  • tests you ran to verify your changes with result summaries.
  • provide instructions so it can be reproduced.
  1. Kick off the server with llama stack run /Users/henrytai/.llama/distributions/llamastack-ollama/ollama-run.yaml --port 5050
  2. Trigger an error case from client. The example is PYTHONPATH=. python -m examples.agents.client localhost 5050 from the llama-stack-apps repo. The issue details: "This is a known issue with Llama3.2-3B-Instruct on ollama, where the model produces bad tool calling outputs. In this case, the server agent fails due to assert error that "code_interpreter" is not in the AgentConfig's tools field" but the code_interpreter is not really necessary as it is not needed in Llama3.1-8B.

client log:

inference> assistant<|end_header_id|>

Switzerland is a beautiful country with a rich history, stunning mountains, and vibrant cities. Here are three must-visit places in Switzerland:

1. **Geneva**: Geneva is a global hub for international diplomacy and is often referred to as the "City of Peace". It's home to ...

These are just a few examples of Switzerland's amazing attractions!
500: Internal server error: An unexpected error occurred. Tool code_interpreter not found
inference> brave_search.call(query="what is special about #1")...

Showing the real error in the client: Tool code_interpreter not found

Sources

Please link relevant resources if necessary.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Ran pre-commit to handle lint / formatting issues.
  • Read the contributor guideline,
    Pull Request section?
  • Updated relevant documentation.
  • Wrote necessary unit or integration tests.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 20, 2024
@chuenlok chuenlok mentioned this pull request Nov 20, 2024
Merged
6 tasks
ashwinb
ashwinb requested changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

So this is actually not what we want. Because this would mean we are potentially exposing private server-side state (could be something sensitive too) directly to the user. The best you can do is log the exception (which I believe is already logged?)

@chuenlok
Copy link
Contributor Author

So this is actually not what we want. Because this would mean we are potentially exposing private server-side state (could be something sensitive too) directly to the user. The best you can do is log the exception (which I believe is already logged?)

Agree that reporting all exceptions in http 500 response is too much and might include some sensitive info from the server.

We have a bunch of asserts in llama_stack/providers/inline/agents/meta_reference/agent_instance.py, like assert name in tools_dict, f"Tool {name} not found" and these assert errors are not reported to the client. The server indeed logs those errors but the server is not sending the error message to the client. The client side has no idea what happens and only receives http 500.

I have updated to catch the AssertionError and return the details in the http 400 response. This ensures we only send important error instead of all errors.

@chuenlok chuenlok requested a review from ashwinb November 21, 2024 14:06
ashwinb
ashwinb reviewed Nov 21, 2024
View reviewed changes
llama_stack/distribution/server/server.py Outdated
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 assertions are still internal. They should be translated into appropriate ValueErrors at the place where you are throwing that assert -- if you think there is no internal state being divulged. The assertion message can then be approrpiately changed to indicate that this is a user-facing message (and would be nicer.)

Copy link
Contributor Author

@chuenlok chuenlok Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was tempted to change those assert to raise exception. Is there any reason why we are using assert? Usually assert is for fatal issue and we need to shutdown the application as I know?

Let me change those assert to raise value exception then, if appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chuenlok Indeed, the assert may be inappropriate and perhaps we should introduce a specific UnexpectedModelOutputError which we then explicitly handle.

Copy link
Contributor

@ashwinb ashwinb Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here's a friendly list of what generic Exceptions could be caught at the highest level and potentially exposed to the user: https://gist.github.com/ashwinb/8f2ced735408774f6bd41b5188ae974c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a friendly list of what generic Exceptions could be caught at the highest level and potentially exposed to the user: https://gist.github.com/ashwinb/8f2ced735408774f6bd41b5188ae974c

This is really useful! I am still new to the stack and might not be able to use the correct exception. Let me give it a try based on your list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted some assert to exceptions and leave the internal one untouched.

append the error in the internal server error
663f222 
undo the linter changes

catch the assert error and report to the client

fix linter issue

convert asserts to raise exception for better error hanlding

translate assert to exception accordingly
@chuenlok chuenlok requested a review from ashwinb November 22, 2024 06:28
@chuenlok chuenlok changed the title append the error in the internal server error return proper error from agent instance to the client Nov 25, 2024
@chuenlok
Copy link
Contributor Author

@ashwinb Can you review this PR please?

ashwinb
ashwinb requested changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

I think several of these changes are not quite correct

llama_stack/providers/inline/agents/meta_reference/agent_instance.py
) -> AsyncGenerator:
assert request.stream is True, "Non-streaming not supported"
if not request.stream:
raise NotImplementedError("Non-streaming not supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is good

llama_stack/providers/inline/agents/meta_reference/agent_instance.py
assert (
len(result_messages) == 1
), "Currently not supporting multiple messages"
if len(result_messages) != 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

it is still an assertion failure so this change is not good

llama_stack/providers/inline/agents/meta_reference/agent_instance.py
memory = self._memory_tool_definition()
assert memory is not None, "Memory tool not configured"
if memory is None:
raise KeyError("Memory tool not configured")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this is right either

llama_stack/providers/inline/agents/meta_reference/agent_instance.py
# Whether to call tools on each message and aggregate
# or aggregate and call tool once, reamins to be seen.
assert len(messages) == 1, "Expected single message"
if len(messages) != 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

or this

llama_stack/providers/inline/agents/meta_reference/agent_instance.py
name = tool_call.tool_name
assert isinstance(name, BuiltinTool)

if not isinstance(name, BuiltinTool):
Copy link
Contributor

Choose a reason for hiding this comment

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

nope, this needs to be an assert

llama_stack/providers/inline/agents/meta_reference/agent_instance.py
raise TypeError(f"Tool {name} is not an instance of BuiltinTool.")
name = name.value
if name not in tools_dict:
raise KeyError(f"Tool {name} not found in agent config.")
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems correct but I might call it a ValueError

@ashwinb ashwinb closed this Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashwinb ashwinb ashwinb requested changes

@yanxi0830 yanxi0830 Awaiting requested review from yanxi0830 yanxi0830 is a code owner

@hardikjshah hardikjshah Awaiting requested review from hardikjshah hardikjshah is a code owner

@dltn dltn Awaiting requested review from dltn

@raghotham raghotham Awaiting requested review from raghotham raghotham is a code owner

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chuenlok @ashwinb @facebook-github-bot