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

up #1774

Merged
merged 1 commit into from
Jan 8, 2025
Merged

up #1774

merged 1 commit into from
Jan 8, 2025

Conversation

emrgnt-cmplxty
Copy link
Contributor

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

Important

Added missing columns to user tables and refactored knowledge graph extraction terminology for clarity.

  • Database Schema Updates:
    • In users.py, added columns metadata and limits_overrides to users table and description to users_api_keys table if they don't exist.
  • Knowledge Graph Extraction Refactoring:
    • In extraction.py, renamed extractions to chunks for clarity in GraphExtractionPipe class.
    • Updated logging messages to reflect the new terminology.
    • Adjusted logic to handle chunks instead of extractions in various methods.

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

@emrgnt-cmplxty emrgnt-cmplxty marked this pull request as ready for review January 8, 2025 04:17
@emrgnt-cmplxty emrgnt-cmplxty merged commit dea0f09 into main Jan 8, 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 84d167e in 1 minute and 57 seconds

More details
  • Looked at 240 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. py/core/database/users.py:124
  • Draft comment:
    The check_columns_query is redundant because the columns metadata, limits_overrides, and description are already included in the table creation queries. Consider removing this redundant query to avoid unnecessary operations.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    While the columns do appear in both places, this is likely intentional. The ALTER TABLE statements are marked as "(New)" and are specifically for adding these columns if they're missing. This is a common database migration pattern - you define the full schema for new installations, but also provide ALTER TABLE statements to modify existing installations. This is not redundant, it's purposeful.
    The comment author might be right that for brand new installations, these ALTER TABLE statements will have no effect. But they serve an important purpose for existing installations.
    The redundancy is intentional and necessary - it's a standard pattern for handling both new installations and upgrades of existing databases. The comment misunderstands this pattern.
    The comment should be deleted because it incorrectly identifies a common and necessary database migration pattern as a redundancy issue.
2. py/core/pipes/kg/extraction.py:248
  • Draft comment:
    The log message still uses 'extractions_groups' instead of 'chunks_groups'. Consider updating it for consistency with the variable name changes.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR changes variable names from 'extractions' to 'chunks' for clarity. However, there are still some comments and log messages that use the term 'extractions'.

Workflow ID: wflow_WEyeYEEuqkVfjDLB


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