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 some test_execution_workflow tests #1009

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

Ahmad-mtos
Copy link
Contributor

@Ahmad-mtos Ahmad-mtos commented Jan 2, 2025

PR Type

Bug fix, Tests


Description

  • Fixed handling of None in prompt_step settings.

  • Corrected tool fetching in get_task query.

  • Added missing task ID assignment in create_task_execution.

  • Updated test case to include inherit_tools parameter.


Changes walkthrough 📝

Relevant files
Bug fix
prompt_step.py
Fix handling of `None` in `prompt_step` settings                 

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

  • Fixed handling of None in settings dictionary.
  • Ensured passed_settings updates correctly with fallback.
  • +1/-1     
    get_task.py
    Fix tool fetching in `get_task` query                                       

    agents-api/agents_api/queries/tasks/get_task.py

  • Corrected tool fetching by adding to_jsonb conversion.
  • Ensured filtering on tool_id is properly applied.
  • +1/-1     
    create_task_execution.py
    Add task ID assignment in `create_task_execution`               

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

  • Added assignment of task ID to execution_input.task.
  • Ensured task ID is properly set during execution creation.
  • +1/-0     
    Tests
    test_execution_workflow.py
    Update test case with `inherit_tools` parameter                   

    agents-api/tests/test_execution_workflow.py

  • Updated test case to include inherit_tools parameter.
  • Improved test coverage for execution workflow.
  • +1/-0     

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


    Important

    Fixes null handling, tool fetching, and task ID assignment in agents-api, and updates tests to include inherit_tools parameter.

    • Behavior:
      • Fix null handling in prompt_step() in prompt_step.py by using or {} to prevent errors when settings is None.
      • Ensure jsonb_agg in get_task.py only aggregates non-null tool_id by converting tl to JSONB.
      • Set execution_input.task.id to task.id in start_execution() in create_task_execution.py to ensure task ID is correctly assigned.
    • Tests:
      • Add inherit_tools=True in test_execution_workflow.py to ensure tools are inherited correctly in tests.

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

    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

    Null Safety

    The updated settings handling with or {} needs validation to ensure it properly handles all edge cases for None/empty settings

    passed_settings.update(passed_settings.pop("settings", {}) or {})
    Data Integrity

    The to_jsonb conversion for tools needs verification to ensure all tool fields are properly serialized and no data is lost

    jsonb_agg(to_jsonb(tl)) FILTER (WHERE tl.tool_id IS NOT NULL),

    Copy link
    Contributor

    qodo-merge-pro-for-open-source bot commented Jan 2, 2025

    CI Failure Feedback 🧐

    (Checks updated until commit d6db947)

    Action: Typecheck

    Failed stage: Typecheck [❌]

    Failed test name: agents_api.app

    Failure summary:

    The action failed due to multiple attribute errors in the file agents_api/app.py. The main issues
    are:

  • The code is trying to access a state attribute on a container object, but container appears to be a
    list type
  • There are 10 instances of this error in the lifespan function where the code attempts to access or
    modify container.state
  • Specific attributes being accessed that caused errors: postgres_pool and s3_client

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    1207:  [1/294] check agents_api.autogen.Common
    1208:  [2/294] check agents_api.autogen.Docs
    1209:  [3/294] check agents_api.env
    1210:  [4/294] check agents_api.autogen.Tools
    1211:  [5/294] check agents_api.clients.pg
    1212:  [6/294] check agents_api.autogen.Users
    1213:  [7/294] check agents_api.autogen.Sessions
    1214:  [8/294] check agents_api.app
    1215:  FAILED: /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/app.pyi 
    1216:  /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
    1217:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:29:24: error: in lifespan: No attribute 'state' on list [attribute-error]
    1218:  if not getattr(container.state, "postgres_pool", None):
    1219:  ~~~~~~~~~~~~~~~
    1220:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:30:13: error: in lifespan: No attribute 'state' on list [attribute-error]
    1221:  container.state.postgres_pool = await create_db_pool(pg_dsn)
    1222:  ~~~~~~~~~~~~~~~
    1223:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:38:24: error: in lifespan: No attribute 'state' on list [attribute-error]
    1224:  if not getattr(container.state, "s3_client", None):
    1225:  ~~~~~~~~~~~~~~~
    1226:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:40:13: error: in lifespan: No attribute 'state' on list [attribute-error]
    1227:  container.state.s3_client = await session.create_client(
    1228:  ~~~~~~~~~~~~~~~
    1229:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:52:24: error: in lifespan: No attribute 'state' on list [attribute-error]
    1230:  if getattr(container.state, "postgres_pool", None):
    1231:  ~~~~~~~~~~~~~~~
    1232:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:53:23: error: in lifespan: No attribute 'state' on list [attribute-error]
    1233:  await container.state.postgres_pool.close()
    1234:  ~~~~~~~~~~~~~~~
    1235:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:54:17: error: in lifespan: No attribute 'state' on list [attribute-error]
    1236:  container.state.postgres_pool = None
    1237:  ~~~~~~~~~~~~~~~
    1238:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:58:24: error: in lifespan: No attribute 'state' on list [attribute-error]
    1239:  if getattr(container.state, "s3_client", None):
    1240:  ~~~~~~~~~~~~~~~
    1241:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:59:23: error: in lifespan: No attribute 'state' on list [attribute-error]
    1242:  await container.state.s3_client.close()
    1243:  ~~~~~~~~~~~~~~~
    1244:  /home/runner/work/julep/julep/agents-api/agents_api/app.py:60:17: error: in lifespan: No attribute 'state' on list [attribute-error]
    1245:  container.state.s3_client = None
    1246:  ~~~~~~~~~~~~~~~
    1247:  For more details, see https://google.github.io/pytype/errors.html#attribute-error
    ...
    
    1320:  [81/294] check agents_api.common.__init__
    1321:  [82/294] check agents_api.common.utils.__init__
    1322:  [83/294] check tests.sample_tasks.test_find_selector
    1323:  [84/294] check agents_api.worker.__init__
    1324:  [85/294] check agents_api.model_registry
    1325:  [86/294] check agents_api.common.exceptions.agents
    1326:  [87/294] check agents_api.rec_sum.__init__
    1327:  [88/294] check agents_api.common.utils.json
    1328:  ninja: build stopped: cannot make progress due to previous errors.
    1329:  Computing dependencies
    1330:  Generated API key since not set in the environment: 76680160409045757867909489452703
    1331:  Sentry DSN not found. Sentry will not be enabled.
    1332:  Analyzing 284 sources with 0 local dependencies
    1333:  Leaving directory '.pytype'
    1334:  ##[error]Process completed with exit code 1.
    ...
    
    1336:  [command]/usr/bin/git version
    1337:  git version 2.47.1
    1338:  Temporarily overriding HOME='/home/runner/work/_temp/ccdbc1cb-8fab-480e-aa49-8aa7a0f90dcb' before making global git config changes
    1339:  Adding repository directory to the temporary git global config as a safe directory
    1340:  [command]/usr/bin/git config --global --add safe.directory /home/runner/work/julep/julep
    1341:  [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
    1342:  [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' || :"
    1343:  fatal: No url found for submodule path 'drafts/cozo' in .gitmodules
    1344:  ##[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.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add comprehensive null checking in SQL aggregation to prevent runtime errors

    Add null check for tl (tool list) before converting to jsonb to prevent potential
    SQL errors

    agents-api/agents_api/queries/tasks/get_task.py [31]

    -jsonb_agg(to_jsonb(tl)) FILTER (WHERE tl.tool_id IS NOT NULL)
    +jsonb_agg(to_jsonb(tl)) FILTER (WHERE tl IS NOT NULL AND tl.tool_id IS NOT NULL)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds an important safety check for NULL values in the SQL query. Adding 'tl IS NOT NULL' alongside 'tl.tool_id IS NOT NULL' provides more robust error prevention when dealing with potentially null records.

    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! Reviewed everything up to d6db947 in 57 seconds

    More details
    • Looked at 53 lines of code in 4 files
    • Skipped 0 files when reviewing.
    • Skipped posting 4 drafted comments based on config settings.
    1. agents-api/agents_api/queries/tasks/get_task.py:31
    • Draft comment:
      Ensure that to_jsonb(tl) is used consistently for JSONB conversion. This change aligns with best practices for handling JSON data in SQL.
    • Reason this comment was not posted:
      Confidence changes required: 10%
      The change in line 31 of get_task.py modifies the aggregation of tools to use to_jsonb(tl) instead of just tl. This ensures that the tools are properly converted to JSONB format, which is necessary for consistent data handling. This change is correct and aligns with best practices for handling JSON data in SQL.
    2. agents-api/agents_api/activities/task_steps/prompt_step.py:119
    • Draft comment:
      Using or {} ensures that None values are handled safely, preventing potential TypeError. This is a good defensive programming practice.
    • Reason this comment was not posted:
      Confidence changes required: 10%
      The change in line 119 of prompt_step.py ensures that if settings is None, an empty dictionary is used instead. This prevents potential TypeError when None is passed to update(). This is a good defensive programming practice.
    3. agents-api/tests/test_execution_workflow.py:668
    • Draft comment:
      Ensure that inherit_tools=True is used consistently where tool inheritance is required in tests. This aligns with the intended functionality of the test.
    • Reason this comment was not posted:
      Confidence changes required: 10%
      The addition of inherit_tools=True in the test case on line 668 of test_execution_workflow.py ensures that tools are inherited in the workflow. This aligns with the intended functionality of the test.
    4. agents-api/agents_api/routers/tasks/create_task_execution.py:76
    • Draft comment:
      Setting execution_input.task.id = task.id ensures task ID consistency and traceability during execution. This is a good practice for maintaining data integrity.
    • Reason this comment was not posted:
      Confidence changes required: 10%
      The addition of execution_input.task.id = task.id in create_task_execution.py ensures that the task ID is explicitly set in the execution input. This is important for maintaining consistency and traceability of tasks during execution.

    Workflow ID: wflow_9QBiIk5VLkVfrsRm


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

    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 4d9f18d in 19 seconds

    More details
    • Looked at 20 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/activities/utils.py:16
    • Draft comment:
      The import of reduce from functools is redundant as it is already imported on line 16. Please remove the duplicate import.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The import of 'reduce' from functools is redundant because it is already imported in line 16. This redundancy should be removed to clean up the code.

    Workflow ID: wflow_TCkb9DhgUYgmgx9m


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

    @Vedantsahai18 Vedantsahai18 merged commit 20a68bf into f/switch-to-pg Jan 2, 2025
    1 check passed
    @Vedantsahai18 Vedantsahai18 deleted the x/execution-workflow-tests branch January 2, 2025 17:11
    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