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

upsert document info infront of hatchet #1806

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

emrgnt-cmplxty
Copy link
Contributor

@emrgnt-cmplxty emrgnt-cmplxty commented Jan 11, 2025

Important

Add logic to upsert document information in create_document() when orchestration is enabled in documents_router.py.

  • Behavior:
    • In create_document() in documents_router.py, added logic to upsert document information when run_with_orchestration is True.
    • Calls upsert_documents_overview() with document info created from _create_document_info_from_file().
  • Misc:
    • Removed outdated TODO comment about modifying create_chunks in create_document().

This description was created by Ellipsis for 74cc1b4. It will automatically update as commits are pushed.

@emrgnt-cmplxty emrgnt-cmplxty marked this pull request as ready for review January 11, 2025 00:35
@emrgnt-cmplxty emrgnt-cmplxty merged commit 924d2a3 into main Jan 11, 2025
2 of 3 checks passed
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 74cc1b4 in 1 minute and 59 seconds

More details
  • Looked at 35 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/core/main/api/v3/documents_router.py:549
  • Draft comment:
    Consider adding error handling for the upsert_documents_overview operation to manage potential database errors gracefully.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    Error handling is generally good practice for database operations. However:
  1. This is inside a larger try/catch block (the route handler)
  2. FastAPI will automatically handle and format any exceptions
  3. The comment is speculative - it doesn't point to a specific issue
  4. The comment isn't actionable - it doesn't specify what kind of error handling to add
  5. Database errors would be caught by the outer error handling
    The comment raises a valid general point about defensive programming. Database operations can fail and should be handled gracefully.
    While error handling is important, this code already has error handling through FastAPI's exception handling and the route's error handling. Adding another try/catch would be redundant.
    Delete the comment. It makes a speculative suggestion about error handling without identifying a specific issue, and the code already has appropriate error handling through FastAPI.

Workflow ID: wflow_I1PQ1hHRM99URbRb


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

1 participant