chore: throw UnstructuredIngestError correctly in plugins #62
Conversation
| from starlette.responses import RedirectResponse | ||
| from unstructured_ingest.data_types.file_data import BatchFileData, FileData, file_data_from_dict | ||
| from unstructured_ingest.error import UnstructuredIngestError | ||
| from unstructured_ingest.errors_v2 import UnstructuredIngestError as UnstructuredIngestErrorV2 |
There was a problem hiding this comment.
i really don't get why we have a errors_v2 ////
cc @potter-potter ?
There was a problem hiding this comment.
errors v2 was when we started to move toward the new error system.
I think it might be there now to be backward compatible. if i recall in someones pr it was removed and caused all sorts of havoc because it was in lots of repos.
|
@claude please review |
claude you lazy sucker |
There was a problem hiding this comment.
Pull Request Overview
This PR adds proper error handling for UnstructuredIngestError exceptions in the ETL API, ensuring that HTTP status codes are correctly propagated instead of defaulting to 500. The change addresses an issue where Pinecone validation errors (400) were being returned as 500 errors.
- Imports both versions of
UnstructuredIngestErrorto handle the duplicate definitions - Adds specific exception handling to catch and properly format these errors with their status codes
- Includes comprehensive test coverage for various error scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| unstructured_platform_plugins/etl_uvicorn/api_generator.py | Adds exception handling for UnstructuredIngestError types with proper status code propagation |
| unstructured_platform_plugins/version.py | Version bump to 0.0.41 |
| test/assets/exception_status_code.py | Adds test helper functions for UnstructuredIngestError scenarios |
| test/api/test_api.py | Comprehensive test coverage for UnstructuredIngestError handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ) | ||
| except (UnstructuredIngestError, UnstructuredIngestErrorV2) as exc: | ||
| logger.error( | ||
| f"UnstructuredIngestError: {str(exc)} (status_code={exc.status_code})", |
There was a problem hiding this comment.
The error message 'UnstructuredIngestError:' is misleading when the caught exception might be UnstructuredIngestErrorV2. Consider using a generic message like 'Unstructured ingest error:' or dynamically include the exception type name.
| f"UnstructuredIngestError: {str(exc)} (status_code={exc.status_code})", | |
| f"{exc.__class__.__name__}: {str(exc)} (status_code={exc.status_code})", |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
v2 removed so should be fine here
test/assets/exception_status_code.py
Outdated
| error = UnstructuredIngestErrorV2( | ||
| "Async gen test UnstructuredIngestErrorV2 with None status_code" | ||
| ) |
There was a problem hiding this comment.
[nitpick] The string literal is split across lines unnecessarily. Consider keeping the error message on a single line for better readability, or if line length is a concern, use implicit string concatenation or a shorter variable name.
| error = UnstructuredIngestErrorV2( | |
| "Async gen test UnstructuredIngestErrorV2 with None status_code" | |
| ) | |
| error = UnstructuredIngestErrorV2("Async gen test UnstructuredIngestErrorV2 with None status_code") |
Copilot uses AI. Check for mistakes.
|
@yuming-long I think we can remove the v2 stuff since it is just for backward compatibility. ya? |
This reverts commit 368a02f.
|
done ^ - i realize the v2 class is never used :) |
2 participants
Summary
saw error for pinecone
I believe this should be a 400
so I need to raise
UnstructuredIngestErrorhereunfortunately its defined twice: