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: improve error handling and logging for eval insertions #2854

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

axiomofjoy
Copy link
Contributor

@axiomofjoy axiomofjoy commented Apr 11, 2024

Removes an error in a logging statement causing #2841 and improves error handling and logging.

resolves #2778

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Apr 11, 2024
@axiomofjoy axiomofjoy changed the base branch from main to sql April 11, 2024 02:31
@axiomofjoy axiomofjoy linked an issue Apr 11, 2024 that may be closed by this pull request
@axiomofjoy axiomofjoy added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Apr 11, 2024
@axiomofjoy axiomofjoy marked this pull request as draft April 11, 2024 02:44
@axiomofjoy axiomofjoy changed the title fix: remove error in logging statement fix: improve error handling and logging for eval insertions Apr 11, 2024
@axiomofjoy axiomofjoy marked this pull request as ready for review April 11, 2024 03:04
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Apr 11, 2024
except Exception:
logger.exception(
"Failed to insert evaluation "
f"for span_id={evaluation.SubjectId.span_id}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing #2841. Nested sessions are working otherwise.

Comment on lines +116 to +117
except InsertEvaluationError as error:
logger.exception(f"Failed to insert evaluation: {str(error)}")
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 this printing is automatic, so it would be a duplication

Suggested change
except InsertEvaluationError as error:
logger.exception(f"Failed to insert evaluation: {str(error)}")
except InsertEvaluationError:
logger.exception(f"Failed to insert evaluation")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more granular error message is included in the stacktrace but not the log messages. I think it's worth keeping.

src/phoenix/db/bulk_inserter.py Outdated Show resolved Hide resolved
src/phoenix/db/bulk_inserter.py Outdated Show resolved Hide resolved
src/phoenix/db/bulk_inserter.py Outdated Show resolved Hide resolved
axiomofjoy and others added 3 commits April 11, 2024 08:37
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
@axiomofjoy axiomofjoy merged commit d04694b into sql Apr 11, 2024
11 checks passed
@axiomofjoy axiomofjoy deleted the fix-logging-statement branch April 11, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix nested sessions [persistence] pyarrow to sqlite ingestion of evals
2 participants