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

Swap situation & system_template | Other misc fixes #996

Merged
merged 5 commits into from
Dec 27, 2024

Conversation

HamadaSalhab
Copy link
Contributor

@HamadaSalhab HamadaSalhab commented Dec 27, 2024

PR Type

Enhancement


Description

  • Swapped situation and system_template fields in session models:

    • Made situation nullable and optional
    • Moved system template content to system_template field
    • Updated field descriptions for clarity
  • Enhanced system template structure:

    • Removed tools section from template
    • Added situation field to template format
    • Improved template organization
  • Improved chat functionality:

    • Added user data to chat environment
    • Fixed async operations in message gathering
    • Updated system message rendering logic
  • Enhanced error handling and edge cases:

    • Better handling of empty users/agents lists
    • Improved SQL query processing

Changes walkthrough 📝

Relevant files
Enhancement
Sessions.py
Refactor session models and system template structure       

agents-api/agents_api/autogen/Sessions.py

  • Swapped situation and system_template fields in session models
  • Made situation nullable and optional
  • Updated field descriptions and default values
  • Removed tools section from system template
  • +20/-20 
    sessions.py
    Enhance chat environment with user data                                   

    agents-api/agents_api/common/protocol/sessions.py

  • Added user data to chat environment
  • Fixed handling of empty users/agents lists
  • +1/-0     
    chat.py
    Update system message rendering logic                                       

    agents-api/agents_api/routers/sessions/chat.py

  • Changed system message rendering to use system_template instead of
    situation
  • +2/-2     
    constants.tsp
    Revise system template structure                                                 

    typespec/common/constants.tsp

  • Updated system template format
  • Added situation field to template
  • Removed tools section
  • +6/-9     
    models.tsp
    Update session model specifications                                           

    typespec/sessions/models.tsp

  • Updated session model field definitions
  • Changed situation to be nullable
  • Updated field descriptions
  • +4/-4     
    Bug fix
    gather_messages.py
    Fix async operations and improve query handling                   

    agents-api/agents_api/queries/chat/gather_messages.py

  • Fixed function calls to be async
  • Improved SQL query handling
  • Enhanced error handling
  • +6/-6     

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


    Important

    Enhances session models by swapping situation and system_template, improves chat functionality, and fixes async operations.

    • Session Model Enhancements:
      • Swapped situation and system_template fields in Sessions.py.
      • Made situation nullable and optional.
      • Updated field descriptions and default values.
    • System Template Structure:
      • Removed tools section and added situation field in constants.tsp.
    • Chat Functionality:
      • Added user data to chat environment in sessions.py.
      • Updated system message rendering in chat.py to use system_template.
    • Bug Fixes:
      • Fixed async operations and improved SQL query handling in gather_messages.py.
      • Enhanced error handling for empty users/agents lists in prepare_chat_context.py.

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

    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 23de839 in 21 seconds

    More details
    • Looked at 613 lines of code in 8 files
    • Skipped 0 files when reviewing.
    • Skipped posting 4 drafted comments based on config settings.
    1. agents-api/agents_api/queries/chat/gather_messages.py:43
    • Draft comment:
      The change from synchronous to asynchronous function calls for get_history, get_session, and document search functions improves performance in an async environment. Ensure all related functions are properly awaited.
    • Reason this comment was not posted:
      Comment did not seem useful.
    2. agents-api/agents_api/common/protocol/sessions.py:120
    • Draft comment:
      The addition of self.users[0].model_dump() if len(self.users) > 0 else None ensures the user key is always present, preventing potential KeyErrors. This is a good practice.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The addition of self.users[0].model_dump() if len(self.users) > 0 else None ensures that the user key is always present in the dictionary, which is a good practice to avoid potential KeyErrors.
    3. agents-api/agents_api/queries/chat/prepare_chat_context.py:14
    • Draft comment:
      The change to use sqlglot for parsing SQL queries improves readability and management of SQL code. This is a positive change.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The change from a string to a parsed SQL query using sqlglot is a significant improvement for SQL query management and readability. This should be noted as a positive change.
    4. typespec/common/constants.tsp:41
    • Draft comment:
      The removal of the tools section from the default system template might affect functionality if tools are expected in the session context. Verify if this change aligns with the intended functionality.
    • Reason this comment was not posted:
      Comment did not seem useful.

    Workflow ID: wflow_J6R3eZWZnF5UKnTo


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

    Copy link

    qodo-merge-pro-for-open-source bot commented Dec 27, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit 9e82981)

    Action: Typecheck

    Failed stage: Generate openapi code [❌]

    Failed test name: ""

    Failure summary:

    The action failed because the tsp command was not found in the system. Specifically:

  • The script attempted to run tsp compile . in the typespec/ directory
  • The error "command not found" indicates that the TypeSpec compiler (tsp) is not installed or not in
    the system PATH
  • This resulted in exit code 127, which typically indicates a command not found error

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    187:  Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.8/x64
    188:  Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.8/x64
    189:  LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.8/x64/lib
    190:  ##[endgroup]
    191:  + set -e
    192:  + cd typespec/
    193:  + tsp compile .
    194:  scripts/generate_openapi_code.sh: line 10: tsp: command not found
    195:  ##[error]Process completed with exit code 127.
    ...
    
    197:  [command]/usr/bin/git version
    198:  git version 2.47.1
    199:  Temporarily overriding HOME='/home/runner/work/_temp/6f8f089c-ce3f-4154-9556-7da5bb13ee22' before making global git config changes
    200:  Adding repository directory to the temporary git global config as a safe directory
    201:  [command]/usr/bin/git config --global --add safe.directory /home/runner/work/julep/julep
    202:  [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
    203:  [command]/usr/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
    204:  fatal: No url found for submodule path 'drafts/cozo' in .gitmodules
    205:  ##[warning]The process '/usr/bin/git' failed with exit code 128
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    SQL Injection:
    The SQL query in prepare_chat_context.py is constructed using string formatting with parse_one(). While parameters are used for developer_id and session_id, the query construction should be reviewed to ensure proper SQL injection prevention.

    ⚡ Recommended focus areas for review

    Error Handling

    The function prepare_chat_context returns a tuple with SQL query and parameters, but lacks error handling for invalid UUIDs or database connection issues.

    async def prepare_chat_context(
        *,
        developer_id: UUID,
        session_id: UUID,
    ) -> tuple[str, list]:
    Template Validation

    The system template is rendered without validating if it's a valid Jinja2 template, which could cause runtime errors if the template syntax is invalid.

    if system_template := chat_context.session.system_template:
        system_message = dict(
            role="system",
            content=system_template,
        )

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Initialize empty lists for missing data to prevent KeyError exceptions when accessing dictionary fields

    Initialize empty lists for users and agents in the transformed data to prevent
    potential KeyError exceptions when accessing these fields later in the code.

    agents-api/agents_api/queries/chat/prepare_chat_context.py [120-126]

    +# Ensure users and agents lists exist
    +d["users"] = d.get("users", [])
    +d["agents"] = d.get("agents", [])
    +
     transformed_data = {
         **d,
         "session": make_session(
    -        agents=[a["id"] for a in d.get("agents") or []],
    -        users=[u["id"] for u in d.get("users") or []],
    +        agents=[a["id"] for a in d["agents"]],
    +        users=[u["id"] for u in d["users"]],
             **d["session"],
         ),
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code robustness by explicitly initializing empty lists for users and agents before accessing them, preventing potential KeyError exceptions in production.

    7

    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! Incremental review on 9e82981 in 10 seconds

    More details
    • Looked at 38 lines of code in 2 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/queries/chat/prepare_chat_context.py:16
    • Draft comment:
      Consider using sqlglot to parse and format the SQL query for better validation and readability.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The SQL query in prepare_chat_context.py is not being parsed by sqlglot anymore, which might affect SQL validation and formatting.

    Workflow ID: wflow_d6gDIgVHtnHIM9h6


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

    @HamadaSalhab HamadaSalhab merged commit 97982b6 into f/switch-to-pg Dec 27, 2024
    7 of 12 checks passed
    @HamadaSalhab HamadaSalhab deleted the c/session-situation-system-template branch December 27, 2024 05:40
    @Vedantsahai18 Vedantsahai18 linked an issue Jan 2, 2025 that may be closed by this pull request
    1 task
    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.

    [Bug]: Move session.situation to session.system
    2 participants