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

fix(agents-api): Fix tests for workflows #998

Merged
merged 1 commit into from
Dec 27, 2024

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Dec 27, 2024

User description

Signed-off-by: Diwank Singh Tomer [email protected]


PR Type

Enhancement, Tests


Description

  • Implemented dependency injection pattern using a container for state management
  • Added async database operations with connection pool management
  • Enhanced lifespan management with Protocol classes for type safety
  • Updated test suite to support async database operations
  • Removed caching in favor of container-based state management
  • Added proper connection pool handling across activities and task steps

Changes walkthrough 📝

Relevant files
Enhancement
container.py
Add state container for dependency injection                         

agents-api/agents_api/activities/container.py

  • Added new State and Container classes for state management
  • Created singleton container instance
  • +12/-0   
    execute_integration.py
    Add container integration and async database access           

    agents-api/agents_api/activities/execute_integration.py

  • Added lifespan decorator for container management
  • Updated tool args functions to be async and use postgres pool from
    container
  • Added connection pool parameter to metadata functions
  • +15/-4   
    execute_system.py
    Add container integration for system execution                     

    agents-api/agents_api/activities/execute_system.py

  • Added lifespan decorator for container management
  • Updated get_developer function to use postgres pool from container
  • +7/-1     
    pg_query_step.py
    Refactor database connection management                                   

    agents-api/agents_api/activities/task_steps/pg_query_step.py

  • Removed alru_cache and get_db_pool function
  • Added lifespan decorator for container management
  • Updated to use postgres pool from container
  • +4/-10   
    transition_step.py
    Add container integration for transition steps                     

    agents-api/agents_api/activities/task_steps/transition_step.py

  • Added lifespan decorator for container management
  • Added connection pool parameter to create_execution_transition
  • +4/-0     
    app.py
    Enhance lifespan management with protocols                             

    agents-api/agents_api/app.py

  • Added Protocol classes for type safety
  • Updated lifespan function to support both FastAPI and ObjectWithState
  • +10/-1   
    __main__.py
    Simplify worker initialization                                                     

    agents-api/agents_api/worker/main.py

  • Removed app and lifespan imports
  • Simplified worker initialization
  • +1/-3     
    Tests
    test_execution_workflow.py
    Update tests for async database operations                             

    agents-api/tests/test_execution_workflow.py

  • Updated test fixtures to use connection pool
  • Added async database operations
  • Added new test cases for workflow execution
  • +1429/-1381

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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    State Management

    The State class is empty and may need additional attributes/methods to properly manage application state. Consider documenting expected state properties.

    class State:
        pass
    Resource Management

    The worker is started without proper cleanup in a lifespan context manager, which could lead to resource leaks. Consider adding proper cleanup.

    client = await temporal.get_client_with_metrics()
    worker = create_worker(client)
    
    # Start the worker to listen for and process tasks
    await worker.run()

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for required parameters before accessing them to prevent runtime errors

    Add error handling for the case where developer_id is not present in arguments.
    Currently the code assumes it exists which could lead to KeyError.

    agents-api/agents_api/activities/execute_system.py [95-98]

    +developer_id = arguments.get("developer_id")
    +if not developer_id:
    +    raise ValueError("developer_id is required for chat operations")
     developer = await get_developer(
    -    developer_id=arguments["developer_id"],
    +    developer_id=developer_id,
         connection_pool=container.state.postgres_pool,
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential runtime error by adding validation for the required developer_id parameter. This is important for system stability and proper error handling.

    8
    Add proper resource cleanup and error handling for worker execution

    The worker is now running without proper resource cleanup. Add error handling and
    graceful shutdown using try/finally or async context manager.

    agents-api/agents_api/worker/main.py [39]

    -await worker.run()
    +try:
    +    await worker.run()
    +finally:
    +    await worker.shutdown()
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses an important issue by adding proper resource cleanup, which helps prevent resource leaks and ensures graceful shutdown of the worker process.

    8
    Validate integration execution results to ensure they are not None or invalid

    Add error handling for failed integration execution results before returning them to
    prevent potential None or invalid responses from propagating.

    agents-api/agents_api/activities/execute_integration.py [32-46]

     result = await integrations.execute(
         integration=integration,
         arguments=arguments,
         setup=merged_tool_setup,
     )
    +if result is None:
    +    raise IntegrationExecutionException("Integration execution returned no result")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by validating integration execution results, preventing potential issues with null or invalid responses from propagating through the system.

    7
    Add proper error handling for dynamic module and attribute lookups

    Add error handling for the case when query_name split operation fails or
    module/attribute lookups fail.

    agents-api/agents_api/activities/task_steps/pg_query_step.py [19-22]

    -(module_name, name) = query_name.split(".")
    -module = getattr(queries, module_name)
    -query = getattr(module, name)
    +try:
    +    module_name, name = query_name.split(".")
    +    module = getattr(queries, module_name)
    +    query = getattr(module, name)
    +except (ValueError, AttributeError) as e:
    +    raise ValueError(f"Invalid query name format or query not found: {query_name}") from e
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling for dynamic imports and attribute lookups, providing clearer error messages and making the code more robust against runtime failures.

    7
    General
    Follow Python conventions for handling unused variables to improve code readability

    Avoid using underscore prefix for the unused variable name. Instead, use a standard
    Python convention like using just '_' for unused variables.

    agents-api/agents_api/worker/worker.py [43]

    -_task_activity_names, task_activities = zip(*getmembers(task_steps, isfunction))
    +_, task_activities = zip(*getmembers(task_steps, isfunction))
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While the suggestion correctly identifies a minor style issue, using '_' vs '_task_activity_names' has minimal impact on code functionality or maintainability.

    3

    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 fbb33c4 in 1 minute and 24 seconds

    More details
    • Looked at 3085 lines of code in 9 files
    • Skipped 0 files when reviewing.
    • Skipped posting 5 drafted comments based on config settings.
    1. agents-api/agents_api/activities/container.py:2
    • Draft comment:
      The State class is currently empty. If it's intended for future use, consider adding a comment to indicate its purpose.
    • Reason this comment was not posted:
      Confidence changes required: 20%
      The State class is currently empty, which might be intentional for future expansion, but it's worth noting that it doesn't serve any purpose at the moment. If it's meant to be expanded later, a comment indicating that would be helpful for future developers.
    2. agents-api/agents_api/activities/execute_integration.py:16
    • Draft comment:
      Consider adding a comment or documentation explaining the purpose and behavior of the @lifespan(container) decorator, as it is used in multiple files and its role is not immediately clear.
    • Reason this comment was not posted:
      Confidence changes required: 30%
      The lifespan decorator is used in multiple files, but its purpose and behavior are not immediately clear. It would be beneficial to have a comment or documentation explaining its role, especially since it seems to be related to resource management.
    3. agents-api/agents_api/activities/execute_system.py:32
    • Draft comment:
      Consider adding a comment or documentation explaining the purpose and behavior of the @lifespan(container) decorator, as it is used in multiple files and its role is not immediately clear.
    • Reason this comment was not posted:
      Confidence changes required: 30%
      The lifespan decorator is used in multiple files, but its purpose and behavior are not immediately clear. It would be beneficial to have a comment or documentation explaining its role, especially since it seems to be related to resource management.
    4. agents-api/agents_api/activities/task_steps/pg_query_step.py:12
    • Draft comment:
      Consider adding a comment or documentation explaining the purpose and behavior of the @lifespan(container) decorator, as it is used in multiple files and its role is not immediately clear.
    • Reason this comment was not posted:
      Confidence changes required: 30%
      The lifespan decorator is used in multiple files, but its purpose and behavior are not immediately clear. It would be beneficial to have a comment or documentation explaining its role, especially since it seems to be related to resource management.
    5. agents-api/agents_api/activities/task_steps/transition_step.py:28
    • Draft comment:
      Consider adding a comment or documentation explaining the purpose and behavior of the @lifespan(container) decorator, as it is used in multiple files and its role is not immediately clear.
    • Reason this comment was not posted:
      Confidence changes required: 30%
      The lifespan decorator is used in multiple files, but its purpose and behavior are not immediately clear. It would be beneficial to have a comment or documentation explaining its role, especially since it seems to be related to resource management.

    Workflow ID: wflow_Aubz1reEE1471ok6


    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 fbb33c4)

    Action: Typecheck

    Failed stage: Typecheck [❌]

    Failed test name: agents_api.app

    Failure summary:

    The action failed due to two types of errors in the Python type checking:
    1. Attribute errors in
    app.py:
    - No attribute postgres_pool found on app.state
    - No attribute s3_client found on
    app.state
    2. Type comment errors in test_workflow_routes.py:
    - Stray type comments found with
    invalid "type: object" annotations

    The pytype type checker could not proceed due to these errors and exited with code 1.

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    1166:  [1/302] check agents_api.autogen.Common
    1167:  [2/302] check agents_api.autogen.Docs
    1168:  [3/302] check agents_api.env
    1169:  [4/302] check agents_api.autogen.Tools
    1170:  [5/302] check agents_api.clients.pg
    1171:  [6/302] check agents_api.autogen.Users
    1172:  [7/302] check agents_api.autogen.Sessions
    1173:  [8/302] check agents_api.app
    1174:  FAILED: /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/app.pyi 
    1175:  /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.app.imports --module-name agents_api.app --platform linux -V 3.12 -o /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/app.pyi --analyze-annotated --nofail --none-is-not-bool --quick --strict-none-binding /home/runner/work/julep/julep/agents-api/agents_api/app.py
    1176:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:50:19: error: in lifespan: No attribute 'postgres_pool' on Assignable [attribute-error]
    1177:  In Union[Any, Assignable]
    1178:  await app.state.postgres_pool.close()
    1179:  ~~~~~~~~~~~~~~~~~~~~~~~
    1180:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:55:19: error: in lifespan: No attribute 's3_client' on Assignable [attribute-error]
    1181:  In Union[Any, Assignable]
    1182:  await app.state.s3_client.close()
    1183:  ~~~~~~~~~~~~~~~~~~~
    1184:  For more details, see https://google.github.io/pytype/errors.html#attribute-error
    ...
    
    1248:  [72/302] check agents_api.common.utils.json
    1249:  [73/302] check tests.test_sessions
    1250:  [74/302] check tests.sample_tasks.__init__
    1251:  [75/302] check agents_api.clients.__init__
    1252:  [76/302] check agents_api.rec_sum.summarize
    1253:  [77/302] check agents_api.common.utils.__init__
    1254:  [78/302] check agents_api.clients.worker.__init__
    1255:  [79/302] check tests.test_workflow_routes
    1256:  /home/runner/work/julep/julep/agents-api/tests/test_workflow_routes.py:65:1: error: : Stray type comment: object [ignored-type-comment]
    1257:  #   type: object~~~~~~~~~~~~~~~
    1258:  #   type: object
    1259:  /home/runner/work/julep/julep/agents-api/tests/test_workflow_routes.py:114:1: error: : Stray type comment: object [ignored-type-comment]
    1260:  #   type: object~~~~~~~~~~~~~~~
    1261:  #   type: object
    1262:  For more details, see https://google.github.io/pytype/errors.html#ignored-type-comment
    ...
    
    1277:  [94/302] check agents_api.routers.sessions.exceptions
    1278:  [95/302] check agents_api.common.exceptions.users
    1279:  [96/302] check tests.__init__
    1280:  [97/302] check agents_api.routers.tasks.patch_execution
    1281:  [98/302] check tests.sample_tasks.test_find_selector
    1282:  [99/302] check agents_api.rec_sum.entities
    1283:  [100/302] check agents_api.workflows.__init__
    1284:  [101/302] check agents_api.common.exceptions.agents
    1285:  ninja: build stopped: cannot make progress due to previous errors.
    1286:  Computing dependencies
    1287:  Generated API key since not set in the environment: 21394769117187407539804816817385
    1288:  Sentry DSN not found. Sentry will not be enabled.
    1289:  Analyzing 292 sources with 0 local dependencies
    1290:  Leaving directory '.pytype'
    1291:  ##[error]Process completed with exit code 1.
    ...
    
    1293:  [command]/usr/bin/git version
    1294:  git version 2.47.1
    1295:  Temporarily overriding HOME='/home/runner/work/_temp/9ec055da-ac1e-49e9-8990-83594c7d429c' before making global git config changes
    1296:  Adding repository directory to the temporary git global config as a safe directory
    1297:  [command]/usr/bin/git config --global --add safe.directory /home/runner/work/julep/julep
    1298:  [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
    1299:  [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' || :"
    1300:  fatal: No url found for submodule path 'drafts/cozo' in .gitmodules
    1301:  ##[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.

    @whiterabbit1983 whiterabbit1983 merged commit d2cf26c into f/switch-to-pg Dec 27, 2024
    7 of 11 checks passed
    @whiterabbit1983 whiterabbit1983 deleted the x/fix-test-workflow branch December 27, 2024 09:31
    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.

    2 participants