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

Executions queries #993

Merged
merged 7 commits into from
Dec 26, 2024
Merged

Executions queries #993

merged 7 commits into from
Dec 26, 2024

Conversation

whiterabbit1983
Copy link
Contributor

@whiterabbit1983 whiterabbit1983 commented Dec 26, 2024

PR Type

Bug fix, Enhancement


Description

  • Optimized database queries by removing unnecessary sqlglot dependency and simplifying SQL queries
  • Improved code quality by removing unused developer_id parameter from multiple functions
  • Enhanced transitions handling with proper table usage and fixed query structure
  • Added and enabled execution transition tests
  • Updated executions continuous view with better state handling and transition tracking
  • Fixed various small issues in execution-related queries and routes

Changes walkthrough 📝

Relevant files
Enhancement
*.py
Optimize and clean up execution query files                           

agents-api/agents_api/queries/executions/*.py

  • Removed unused sqlglot dependency and simplified SQL queries
  • Removed unnecessary developer_id parameter from several functions
  • Fixed transitions table usage and query structure
  • *.py
    Clean up task routers and execution handling                         

    agents-api/agents_api/routers/tasks/*.py

  • Removed unnecessary developer_id parameter from function calls
  • Updated execution update handling
  • 000013_executions_continuous_view.up.sql
    Update executions continuous view schema                                 

    memory-store/migrations/000013_executions_continuous_view.up.sql

  • Added transition_id to continuous view grouping
  • Changed default execution state from 'pending' to 'queued'
  • +4/-2     
    Tests
    *
    Add and update execution-related tests                                     

    agents-api/tests/*

  • Added test_transition fixture
  • Enabled previously disabled execution transitions test
  • Updated test cases to match new function signatures

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

    @whiterabbit1983 whiterabbit1983 changed the base branch from dev to f/switch-to-pg December 26, 2024 07:06
    Copy link

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

    CI Failure Feedback 🧐

    (Checks updated until commit 268fd3f)

    Action: Typecheck

    Failed stage: Typecheck [❌]

    Failed test name: pytype

    Failure summary:

    The pytype static type checker failed with multiple errors:

  • Type annotation mismatch in search_docs_hybrid.py for the embedding variable
  • Not-callable errors in execute_integration.py where get_tool_args_from_metadata is being called but
    is not a callable object
  • Import error in check_health.py where it cannot find the module router
  • Stray type comments found in test_workflow_routes.py

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    1199:  [14/301] check agents_api.autogen.Tasks
    1200:  [15/301] check agents_api.autogen.Agents
    1201:  [16/301] check agents_api.common.utils.datetime
    1202:  [17/301] check agents_api.dependencies.exceptions
    1203:  [18/301] check agents_api.common.protocol.developers
    1204:  [19/301] check agents_api.common.protocol.agents
    1205:  [20/301] check agents_api.metrics.counters
    1206:  [21/301] check agents_api.queries.utils
    1207:  ERROR:pytype.matcher Invalid type: <class 'pytype.abstract.function.ParamSpecMatch'>
    1208:  [22/301] check agents_api.autogen.openapi_model
    1209:  [23/301] check agents_api.common.exceptions.__init__
    1210:  [24/301] check agents_api.routers.docs.router
    1211:  [25/301] check agents_api.exceptions
    1212:  [26/301] check agents_api.queries.developers.get_developer
    1213:  [27/301] check agents_api.queries.docs.search_docs_hybrid
    1214:  FAILED: /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/queries/docs/search_docs_hybrid.pyi 
    1215:  /home/runner/work/julep/julep/agents-api/.venv/bin/python -m pytype.main --disable pyi-error --imports_info /home/runner/work/julep/julep/agents-api/.pytype/imports/agents_api.queries.docs.search_docs_hybrid.imports --module-name agents_api.queries.docs.search_docs_hybrid --platform linux -V 3.12 -o /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/queries/docs/search_docs_hybrid.pyi --analyze-annotated --nofail --none-is-not-bool --quick --strict-none-binding /home/runner/work/julep/julep/agents-api/agents_api/queries/docs/search_docs_hybrid.py
    1216:  /home/runner/work/julep/julep/agents-api/agents_api/queries/docs/search_docs_hybrid.py:50:1: error: in <module>: Type annotation for embedding does not match type of assignment [annotation-type-mismatch]
    ...
    
    1313:  metadata_filter,
    1314:  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1315:  search_language,
    1316:  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1317:  ],
    1318:  ~~~~~~~~~~
    1319:  )
    1320:  ~~~~~
    1321:  For more details, see https://google.github.io/pytype/errors.html#annotation-type-mismatch
    ...
    
    1408:  [114/301] check agents_api.queries.executions.__init__
    1409:  [115/301] check agents_api.queries.developers.__init__
    1410:  [116/301] check agents_api.queries.entries.__init__
    1411:  [117/301] check agents_api.queries.agents.__init__
    1412:  [118/301] check agents_api.common.retry_policies
    1413:  [119/301] check agents_api.clients.integrations
    1414:  [120/301] check agents_api.activities.task_steps.get_value_step
    1415:  [121/301] check agents_api.activities.execute_integration
    1416:  FAILED: /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/activities/execute_integration.pyi 
    1417:  /home/runner/work/julep/julep/agents-api/.venv/bin/python -m pytype.main --disable pyi-error --imports_info /home/runner/work/julep/julep/agents-api/.pytype/imports/agents_api.activities.execute_integration.imports --module-name agents_api.activities.execute_integration --platform linux -V 3.12 -o /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/activities/execute_integration.pyi --analyze-annotated --nofail --none-is-not-bool --quick --strict-none-binding /home/runner/work/julep/julep/agents-api/agents_api/activities/execute_integration.py
    1418:  /home/runner/work/julep/julep/agents-api/agents_api/activities/execute_integration.py:29:24: error: in execute_integration: 'agents_api.queries.tools.get_tool_args_from_metadata' object is not callable [not-callable]
    1419:  merged_tool_args = get_tool_args_from_metadata(
    1420:  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1421:  developer_id=developer_id, agent_id=agent_id, task_id=task_id, arg_type="args"
    1422:  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1423:  )
    1424:  ~~~~~
    1425:  /home/runner/work/julep/julep/agents-api/agents_api/activities/execute_integration.py:33:25: error: in execute_integration: 'agents_api.queries.tools.get_tool_args_from_metadata' object is not callable [not-callable]
    1426:  merged_tool_setup = get_tool_args_from_metadata(
    1427:  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1428:  developer_id=developer_id, agent_id=agent_id, task_id=task_id, arg_type="setup"
    1429:  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1430:  )
    1431:  ~~~~~
    1432:  For more details, see https://google.github.io/pytype/errors.html#not-callable
    ...
    
    1516:  [205/301] check agents_api.routers.agents.update_agent_tool
    1517:  [206/301] check tests.test_execution_workflow
    1518:  [207/301] check agents_api.autogen.__init__
    1519:  [208/301] check agents_api.common.exceptions.agents
    1520:  [209/301] check agents_api.common.utils.__init__
    1521:  [210/301] check agents_api.rec_sum.__init__
    1522:  [211/301] check agents_api.__init__
    1523:  [212/301] check tests.test_workflow_routes
    1524:  /home/runner/work/julep/julep/agents-api/tests/test_workflow_routes.py:65:1: error: : Stray type comment: object [ignored-type-comment]
    1525:  #   type: object~~~~~~~~~~~~~~~
    1526:  #   type: object
    1527:  /home/runner/work/julep/julep/agents-api/tests/test_workflow_routes.py:114:1: error: : Stray type comment: object [ignored-type-comment]
    1528:  #   type: object~~~~~~~~~~~~~~~
    1529:  #   type: object
    1530:  For more details, see https://google.github.io/pytype/errors.html#ignored-type-comment
    ...
    
    1541:  [223/301] check agents_api.common.exceptions.sessions
    1542:  [224/301] check tests.__init__
    1543:  [225/301] check agents_api.activities.logger
    1544:  [226/301] check tests.test_activities
    1545:  [227/301] check agents_api.common.protocol.__init__
    1546:  [228/301] check tests.sample_tasks.__init__
    1547:  [229/301] check agents_api.activities.__init__
    1548:  [230/301] check agents_api.routers.healthz.check_health
    1549:  FAILED: /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/routers/healthz/check_health.pyi 
    1550:  /home/runner/work/julep/julep/agents-api/.venv/bin/python -m pytype.main --disable pyi-error --imports_info /home/runner/work/julep/julep/agents-api/.pytype/imports/agents_api.routers.healthz.check_health.imports --module-name agents_api.routers.healthz.check_health --platform linux -V 3.12 -o /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/routers/healthz/check_health.pyi --analyze-annotated --nofail --none-is-not-bool --quick --strict-none-binding /home/runner/work/julep/julep/agents-api/agents_api/routers/healthz/check_health.py
    1551:  /home/runner/work/julep/julep/agents-api/agents_api/routers/healthz/check_health.py:5:1: error: in <module>: Can't find module 'router'. [import-error]
    1552:  from .router import router
    1553:  ~~~~~~~~~~~~~~~~~~~~~~~~~~
    1554:  For more details, see https://google.github.io/pytype/errors.html#import-error
    1555:  [231/301] check agents_api.routers.agents.create_agent_tool
    1556:  [232/301] check agents_api.rec_sum.trim
    1557:  ninja: build stopped: cannot make progress due to previous errors.
    1558:  Computing dependencies
    1559:  Generated API key since not set in the environment: 37161730073812595326584643816219
    1560:  Sentry DSN not found. Sentry will not be enabled.
    1561:  Analyzing 291 sources with 0 local dependencies
    1562:  Leaving directory '.pytype'
    1563:  ##[error]Process completed with exit code 1.
    

    ✨ 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.

    @whiterabbit1983 whiterabbit1983 marked this pull request as ready for review December 26, 2024 09:07
    @whiterabbit1983 whiterabbit1983 merged commit 4e1c975 into f/switch-to-pg Dec 26, 2024
    7 of 11 checks passed
    @whiterabbit1983 whiterabbit1983 deleted the f/executions-queries branch December 26, 2024 09:07
    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 e962645 in 33 seconds

    More details
    • Looked at 589 lines of code in 18 files
    • Skipped 0 files when reviewing.
    • Skipped posting 7 drafted comments based on config settings.
    1. agents-api/agents_api/queries/executions/count_executions.py:16
    • Draft comment:
      The removal of sqlglot for parsing SQL queries is fine if parsing is not needed. Ensure that this change is consistent across the codebase and update any related documentation or comments.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR removes the use of sqlglot for parsing SQL queries, which is a valid change if the parsing is not needed. However, the PR does not update the comments or documentation to reflect this change. Additionally, the PR removes the developer_id parameter from several functions, which seems intentional but should be verified for correctness.
    2. agents-api/agents_api/queries/executions/create_temporal_lookup.py:56
    • Draft comment:
      The removal of the developer_id parameter seems intentional. Ensure that this change is consistent across the codebase and does not affect any functionality.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR removes the developer_id parameter from several functions, which seems intentional but should be verified for correctness. This change should be consistent across the codebase.
    3. agents-api/agents_api/queries/executions/get_execution_transition.py:62
    • Draft comment:
      The removal of the developer_id parameter seems intentional. Ensure that this change is consistent across the codebase and does not affect any functionality.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR removes the developer_id parameter from several functions, which seems intentional but should be verified for correctness. This change should be consistent across the codebase.
    4. agents-api/agents_api/queries/executions/get_paused_execution_token.py:40
    • Draft comment:
      The removal of the developer_id parameter seems intentional. Ensure that this change is consistent across the codebase and does not affect any functionality.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR removes the developer_id parameter from several functions, which seems intentional but should be verified for correctness. This change should be consistent across the codebase.
    5. agents-api/agents_api/routers/tasks/create_task_execution.py:147
    • Draft comment:
      The removal of the developer_id parameter from create_temporal_lookup seems intentional. Ensure that this change is consistent across the codebase and does not affect any functionality.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR removes the developer_id parameter from several functions, which seems intentional but should be verified for correctness. This change should be consistent across the codebase.
    6. agents-api/tests/fixtures.py:279
    • Draft comment:
      The removal of the developer_id parameter from create_temporal_lookup seems intentional. Ensure that this change is consistent across the codebase and does not affect any functionality.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR removes the developer_id parameter from several functions, which seems intentional but should be verified for correctness. This change should be consistent across the codebase.
    7. agents-api/tests/test_execution_queries.py:48
    • Draft comment:
      The removal of the developer_id parameter from create_temporal_lookup seems intentional. Ensure that this change is consistent across the codebase and does not affect any functionality.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The PR removes the developer_id parameter from several functions, which seems intentional but should be verified for correctness. This change should be consistent across the codebase.

    Workflow ID: wflow_nOjvR4yL5EVLCzkQ


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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    SQL injection:
    The list_execution_transitions query uses string interpolation for sort parameters ($3, $4) which could allow SQL injection if the input values are not properly validated before being used in the query. The sort column and direction should be validated against a whitelist of allowed values.

    ⚡ Recommended focus areas for review

    SQL Injection

    The query uses string interpolation for sort column and direction parameters which could allow SQL injection if not properly validated

    def _transform(d):
        current_step = d.pop("current_step")
        next_step = d.pop("next_step", None)
    
        return {
    Data Consistency

    The change in default execution state from 'pending' to 'queued' may affect existing code that relies on the 'pending' state

    WHEN lt.type::text IS NULL THEN 'queued'

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add missing required fields in data transformation to ensure complete transition data

    Add missing fields in the _transform function to ensure all required fields are
    present in the transition response. The 'id' and 'updated_at' fields are essential
    for tracking and displaying transitions.

    agents-api/agents_api/queries/executions/list_execution_transitions.py [24-34]

     def _transform(d):
         current_step = d.pop("current_step")
         next_step = d.pop("next_step", None)
     
         return {
    +        "id": d["transition_id"],
    +        "updated_at": utcnow(),
             "current": {
                 "workflow": current_step[0],
                 "step": current_step[1],
             },
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds essential fields (id and updated_at) that are required for proper transition tracking and display. These fields are critical for the API response structure and data consistency.

    8
    General
    Simplify and optimize the ORDER BY clause by removing unnecessary sorting conditions

    Simplify the ORDER BY clause to only use created_at since updated_at is not needed
    for transitions ordering, and ensure proper handling of NULL values.

    agents-api/agents_api/queries/executions/list_execution_transitions.py [24-28]

     ORDER BY 
    -    CASE WHEN $3 = 'created_at' AND $4 = 'asc' THEN created_at END ASC NULLS LAST,
    -    CASE WHEN $3 = 'created_at' AND $4 = 'desc' THEN created_at END DESC NULLS LAST,
    -    CASE WHEN $3 = 'updated_at' AND $4 = 'asc' THEN updated_at END ASC NULLS LAST,
    -    CASE WHEN $3 = 'updated_at' AND $4 = 'desc' THEN updated_at END DESC NULLS LAST
    +    CASE WHEN $4 = 'created_at' AND $5 = 'asc' THEN created_at END ASC NULLS LAST,
    +    CASE WHEN $4 = 'created_at' AND $5 = 'desc' THEN created_at END DESC NULLS LAST
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion improves query performance and maintainability by removing redundant sorting conditions, while preserving the required functionality.

    5

    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