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

improve error handling for builtin assistants #350

Merged
merged 8 commits into from
Mar 11, 2024
Merged

Conversation

pmeier
Copy link
Member

@pmeier pmeier commented Mar 11, 2024

I spend almost an hour today trying to find a bug that wasn't there, because we didn't raise a proper error when our API call failed.

@pmeier pmeier requested a review from nenb March 11, 2024 13:31
@@ -103,10 +103,11 @@ testpaths = [
]
filterwarnings = [
"error",
"ignore::ResourceWarning",
Copy link
Member Author

Choose a reason for hiding this comment

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

You can find plenty of issues all over the web that have problems with unclosed resource warnings when using pytest. We are properly using context managers everywhere, so whatever warning is happening in the test suite stems from it rather than the code we are trying to test.

]


@skip_on_windows
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried multiple things now, but got nothing to work. All data points to the test code being the problem rather than the code that is tested here. Let's ignore this for now. Maybe someone else has more insights later.

@pmeier pmeier merged commit 1043241 into main Mar 11, 2024
12 checks passed
@pmeier pmeier deleted the assistant-error-handling branch March 11, 2024 20:37
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.

None yet

2 participants