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): All tests pass (again) #701

Merged
merged 6 commits into from
Oct 18, 2024
Merged

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Oct 18, 2024

  • fix(agents-api): Fix tests related to docs
  • fix(agents-api): Fix tests related to executions
  • fix(agents-api): All tests pass (again)

Important

Fix tests in agents-api by updating error handling, environment variables, and test logic to ensure all tests pass.

  • Error Handling:
    • Add CompleteAsyncError to exception handling in interceptors.py for execute_activity and execute_workflow.
  • Environment Variables:
    • Add use_blob_store_for_temporal, blob_store_bucket, blob_store_cutoff_kb, s3_endpoint, s3_access_key, and s3_secret_key to env.py.
  • Document Models:
    • Modify get_doc.py and list_docs.py to filter out None embeddings.
    • Update search_docs_by_text.py to handle empty fts_queries.
  • Execution Models:
    • Fix get_execution.py and list_executions.py to check if output is a dictionary before accessing OUTPUT_UNNEST_KEY.
  • Storage Handler:
    • Add checks for use_blob_store_for_temporal in storage_handler.py.
  • Tests:
    • Remove skip decorator from test_docs_queries.py and test_execution_workflow.py.
    • Add new test cases in test_workflow_routes.py for YAML task creation and execution.

This description was created by Ellipsis for 7f53f2c. 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.

❌ Changes requested. Reviewed everything up to 25f36bd in 1 minute and 1 seconds

More details
  • Looked at 377 lines of code in 16 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. agents-api/agents_api/common/nlp.py:44
  • Draft comment:
    Consider converting keywords to lowercase in the normalized list to ensure consistent frequency counting.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The extract_keywords function in nlp.py has a parameter clean that is intended to strip non-alphanumeric characters from keywords. However, the normalized list is created without converting keywords to lowercase, which might affect the frequency count if the same word appears in different cases. This could lead to incorrect keyword extraction results.
2. agents-api/agents_api/models/utils.py:188
  • Draft comment:
    Consider setting a clear default value for debug in the cozo_query decorator and ensure consistent control of debug prints.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The cozo_query decorator in utils.py has parameters debug and only_on_error that are not used effectively. The debug parameter is set to None by default, which might lead to confusion. It would be better to have a clear default value and ensure that the debug prints are controlled consistently.

Workflow ID: wflow_hmkWt6jJew8whXuz


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

agents-api/agents_api/routers/tasks/router.py Show resolved Hide resolved
Signed-off-by: Diwank Singh Tomer <[email protected]>
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 3053335 in 26 seconds

More details
  • Looked at 87 lines of code in 4 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. agents-api/agents_api/activities/execute_system.py:45
  • Draft comment:
    The FIXME comment indicates the need for refactoring. Consider addressing this to improve code maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The execute_system function in execute_system.py has a FIXME comment indicating the need for refactoring. This is a clear indication of technical debt that should be addressed.
2. agents-api/agents_api/common/protocol/remote.py:129
  • Draft comment:
    The pytype: disable=signature-mismatch comment indicates a potential type mismatch issue. Consider addressing this to ensure type safety.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In remote.py, the __getitem__, __setitem__, and __iter__ methods have a pytype: disable=signature-mismatch comment. This indicates a potential type mismatch issue that should be addressed.
3. agents-api/agents_api/common/protocol/remote.py:169
  • Draft comment:
    The pytype: disable=signature-mismatch comment indicates a potential type mismatch issue. Consider addressing this to ensure type safety.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In remote.py, the __getitem__, __setitem__, and __iter__ methods have a pytype: disable=signature-mismatch comment. This indicates a potential type mismatch issue that should be addressed.
4. agents-api/agents_api/common/protocol/remote.py:238
  • Draft comment:
    The pytype: disable=signature-mismatch comment indicates a potential type mismatch issue. Consider addressing this to ensure type safety.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In remote.py, the __getitem__, __setitem__, and __iter__ methods have a pytype: disable=signature-mismatch comment. This indicates a potential type mismatch issue that should be addressed.

Workflow ID: wflow_Mjgg6V5AGBxRI9J2


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

More details
  • Looked at 34 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/tests/fixtures.py:181
  • Draft comment:
    Avoid using time.sleep(0.1) in tests as it can lead to flaky tests. Consider using a more reliable synchronization mechanism.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment addresses a potential issue with the newly added time.sleep(0.1), which can indeed cause flaky tests. The suggestion to use a more reliable synchronization mechanism is actionable and relevant to the change made in the diff.
    The comment does not specify what synchronization mechanism should be used, which might make it less actionable. However, it still highlights a valid concern about test reliability.
    While the comment could be more specific, it still points out a legitimate issue with the use of time.sleep(0.1) in tests, which is a common source of flakiness.
    Keep the comment as it highlights a valid concern about the use of time.sleep(0.1) in tests, which can lead to flaky tests.

Workflow ID: wflow_vZSUpalxzYR1bEJX


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

@Vedantsahai18 Vedantsahai18 merged commit 3669ca5 into dev Oct 18, 2024
14 checks passed
@Vedantsahai18 Vedantsahai18 deleted the x/hackathon-test-fixes branch October 18, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants