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

X/migration fix: Fixed migrations and docs search route #1026

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

Vedantsahai18
Copy link
Member

@Vedantsahai18 Vedantsahai18 commented Jan 7, 2025

PR Type

Bug fix, Tests


Description

  • Fixed type annotation for confidence parameter in search functions.

  • Updated test to reflect changes in document content format.

  • Added migration scripts to fix unique constraint on tools table.

  • Improved documentation and search route functionality.


Changes walkthrough 📝

Relevant files
Enhancement
search_docs_by_embedding.py
Update type annotation for `confidence` parameter               

agents-api/agents_api/queries/docs/search_docs_by_embedding.py

  • Changed type annotation for confidence parameter to support int |
    float.
  • Minor adjustment to improve type flexibility in the function.
  • +1/-1     
    search_docs_hybrid.py
    Update type annotation for `confidence` parameter               

    agents-api/agents_api/queries/docs/search_docs_hybrid.py

  • Changed type annotation for confidence parameter to support int |
    float.
  • Improved type flexibility in hybrid search function.
  • +1/-1     
    Tests
    test_docs_routes.py
    Update test for document content format                                   

    agents-api/tests/test_docs_routes.py

  • Updated test to reflect change in document content format.
  • Ensured test consistency with updated API response structure.
  • +1/-1     
    Bug fix
    000021_fix_toolname_contraint.down.sql
    Add migration script to revert unique constraint                 

    memory-store/migrations/000021_fix_toolname_contraint.down.sql

  • Added script to revert unique constraint changes on tools table.
  • Restored original constraint excluding developer_id.
  • +10/-0   
    000021_fix_toolname_contraint.up.sql
    Add migration script to update unique constraint                 

    memory-store/migrations/000021_fix_toolname_contraint.up.sql

  • Added script to update unique constraint on tools table.
  • Included developer_id in the new unique constraint.
  • +10/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Data Migration

    The constraint change could affect existing data. Need to verify there's no data loss when adding developer_id to the unique constraint.

    ALTER TABLE tools ADD CONSTRAINT ct_unique_name_per_agent 
    UNIQUE (developer_id, agent_id, name, task_id);

    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 e588162 in 22 seconds

    More details
    • Looked at 76 lines of code in 5 files
    • Skipped 0 files when reviewing.
    • Skipped posting 3 drafted comments based on config settings.
    1. agents-api/agents_api/queries/docs/search_docs_by_embedding.py:39
    • Draft comment:
      The docstring mentions confidence (float), but the type hint is int | float. Update the docstring to reflect the correct type.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The change from confidence: float to confidence: int | float is not reflected in the docstring, which still mentions confidence (float). This should be updated for consistency and clarity.
    2. agents-api/agents_api/queries/docs/search_docs_hybrid.py:49
    • Draft comment:
      The docstring mentions confidence (float), but the type hint is int | float. Update the docstring to reflect the correct type.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The change from confidence: float to confidence: int | float is not reflected in the docstring, which still mentions confidence (float). This should be updated for consistency and clarity.
    3. agents-api/tests/test_docs_routes.py:72
    • Draft comment:
      The content field is expected to be a list here, but it was posted as a string. Ensure consistency in the content field format.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The test for deleting a document initially posts a document with content as a string, but the subsequent GET request expects content to be a list. This inconsistency should be addressed.

    Workflow ID: wflow_Ay9DUBj0xOWgR4an


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

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve database migration safety by preventing table locks during constraint creation

    Add a NOT VALID clause to the constraint and validate it separately to prevent table
    locks during migration in production. This helps avoid potential downtime when the
    table contains data.

    memory-store/migrations/000021_fix_toolname_contraint.up.sql [7-8]

     ALTER TABLE tools ADD CONSTRAINT ct_unique_name_per_agent 
    -UNIQUE (developer_id, agent_id, name, task_id);
    +UNIQUE (developer_id, agent_id, name, task_id) NOT VALID;
     
    +ALTER TABLE tools VALIDATE CONSTRAINT ct_unique_name_per_agent;
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical suggestion for production deployments, as it prevents table locks during constraint creation which could cause significant downtime in a production environment with existing data.

    9
    Possible issue
    Ensure migration script can be safely re-run by adding proper error handling

    Add explicit error handling in case the constraint already exists to ensure
    idempotency of the migration script.

    memory-store/migrations/000021_fix_toolname_contraint.down.sql [7-8]

    -ALTER TABLE tools ADD CONSTRAINT ct_unique_name_per_agent 
    -UNIQUE (agent_id, name, task_id);
    +DO $$ 
    +BEGIN
    +    IF NOT EXISTS (
    +        SELECT 1 FROM pg_constraint WHERE conname = 'ct_unique_name_per_agent'
    +    ) THEN
    +        ALTER TABLE tools ADD CONSTRAINT ct_unique_name_per_agent 
    +        UNIQUE (agent_id, name, task_id);
    +    END IF;
    +END $$;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves migration reliability by making it idempotent, preventing errors when running migrations multiple times which is a common scenario in database management.

    8

    @Vedantsahai18 Vedantsahai18 merged commit fefec22 into dev Jan 7, 2025
    12 of 14 checks passed
    @Vedantsahai18 Vedantsahai18 deleted the x/migration-fix branch January 7, 2025 02:16
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant