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

fix: enhance llama-index callback support for exception events #1814

Merged
merged 12 commits into from
Nov 28, 2023

Conversation

axiomofjoy
Copy link
Contributor

@axiomofjoy axiomofjoy commented Nov 28, 2023

LlamaIndex recently changed the way their callback system handles exception information. Previously, exceptions always resulted in independent exception callback events. Now, whenever possible, exception information is passed via the on_event_end callback hook for the callback event that caused the exception. When that is not possible (e.g., when the exception arises outside of any event), the callback system produces an independent exception event containing the exception information. This latter scenario should be rare and indicates that there is a bug in LlamaIndex itself.

This PR adjusts our callback handler to accommodate these changes. In particular, it searches each callback event payload for an EventPayload.EXCEPTION key, and if present, grabs the exception information from that key. In the infrequent but possible event that the LlamaIndex callback system registers an independent exception callback event, we create a span of span kind "chain" and attach the exception information to this span.

Note that LlamaIndex is currently incorrectly passing the exception information for failed LLM calls (e.g., due to rate limit errors) to the parent synthesis span in the case of query engines. I've adjusted the unit tests accordingly for now. I've noted to the LlamaIndex team here.

Screenshot 2023-11-27 at 9 39 58 PM

I will handle percolating exceptions up the span tree in a subsequent PR.

Relevant issues:

@axiomofjoy axiomofjoy linked an issue Nov 28, 2023 that may be closed by this pull request
@axiomofjoy axiomofjoy marked this pull request as ready for review November 28, 2023 05:43
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

src/phoenix/trace/llama_index/callback.py Outdated Show resolved Hide resolved
src/phoenix/trace/llama_index/callback.py Show resolved Hide resolved
src/phoenix/trace/llama_index/callback.py Outdated Show resolved Hide resolved
src/phoenix/trace/llama_index/callback.py Outdated Show resolved Hide resolved
@RogerHYang
Copy link
Contributor

should we bump the LLAMA_INDEX_MINIMUM_VERSION_TRIPLET?

@axiomofjoy
Copy link
Contributor Author

should we bump the LLAMA_INDEX_MINIMUM_VERSION_TRIPLET?

Good catch, addressed in e8c8a71.

@axiomofjoy axiomofjoy merged commit 8db01df into main Nov 28, 2023
9 checks passed
@axiomofjoy axiomofjoy deleted the llama-index-error-info-passed-to-events branch November 28, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] llama_index errors get attached to the wrong span
2 participants