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

feat(agents-api): ALL TESTS PASS!! :D #459

Merged
merged 5 commits into from
Aug 17, 2024

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Aug 17, 2024

  • fix(agents-api): Fix update_execution model
  • feat(agents-api): Add demo workflow for testing
  • fix(agents-api): Use a contextmanager instead of fixture for creating test temporal envs
  • wip
  • feat(agents-api): ALL TESTS PASS!! :D

🚀 This description was created by Ellipsis for commit 0194c73

Summary:

This PR adds a demo workflow, refactors activities for testing, updates environment configurations, and enhances test fixtures to ensure all tests pass in the agents-api module.

Key points:

  • Added agents-api/agents_api/activities/demo.py for demo activity with mock support.
  • Modified agents-api/agents_api/activities/embed_docs.py to include mock activity for testing.
  • Updated agents-api/agents_api/activities/task_steps to use mock activities when testing.
  • Changed agents-api/agents_api/env.py to print environment variables and warn if testing is enabled.
  • Refactored agents-api/agents_api/models/execution to fix transition validation and update logic.
  • Introduced agents-api/agents_api/workflows/demo.py for demo workflow execution.
  • Enhanced test fixtures and utilities in agents-api/tests/ to support testing with Temporal.
  • Ensured all tests pass by using a context manager for Temporal test environments.

Generated with ❤️ by ellipsis.dev

Diwank Tomer added 5 commits August 16, 2024 18:39
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 0194c73 in 1 minute and 36 seconds

More details
  • Looked at 1105 lines of code in 21 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. agents-api/agents_api/activities/demo.py:8
  • Draft comment:
    Ensure that the demo_activity is never called in production environments as it raises an exception. This is controlled by the testing flag, which should be properly set in non-production environments to avoid runtime errors.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The code in agents-api/agents_api/activities/demo.py is designed to switch between a real activity and a mock activity based on the testing flag. This is a common pattern in testing to avoid executing real code that could affect production systems. The demo_activity raises an exception if called in production, which is a safeguard against accidental execution in a non-testing environment. The mock_demo_activity simply returns the sum of two numbers, serving as a benign placeholder during tests.
2. agents-api/agents_api/activities/embed_docs.py:12
  • Draft comment:
    The embed_docs activity processes document snippets for embedding. Ensure that the embedder client and embed_snippets_query are correctly configured and that the testing flag is properly managed to switch between real and mock implementations.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The embed_docs activity in agents-api/agents_api/activities/embed_docs.py is designed to handle document embedding tasks. It uses the embedder client to generate embeddings for document snippets and stores these embeddings using the embed_snippets_query. The mock_embed_docs function is a no-op, used for testing to bypass the actual embedding process. The activity definition at the end of the file switches between the real and mock functions based on the testing flag.
3. agents-api/agents_api/activities/task_steps/evaluate_step.py:16
  • Draft comment:
    The evaluate_step activity evaluates expressions and returns the results. It uses the simple_eval_dict utility function to perform the evaluation. Ensure that the expressions and context are correctly handled and that the testing flag is properly managed.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The evaluate_step activity in agents-api/agents_api/activities/task_steps/evaluate_step.py evaluates expressions based on the context provided. The mock_evaluate_step is a direct reference to evaluate_step, indicating that no special mocking behavior is needed for testing this activity. The activity definition uses the testing flag to decide whether to use the real or mock function.
4. agents-api/agents_api/activities/task_steps/transition_step.py:13
  • Draft comment:
    The transition_step activity manages state transitions within task executions. It uses the create_execution_transition_query to record transitions. Ensure that the transition logic is correctly implemented and that the testing flag is properly managed to switch between real and mock implementations.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The transition_step activity in agents-api/agents_api/activities/task_steps/transition_step.py handles the logic for transitioning between different states in a workflow. It checks if a waiting step is needed and fetches the task token if so. The create_execution_transition_query is used to record the transition in the database. The mock_transition_step does nothing, serving as a placeholder during testing. The activity definition switches between the real and mock functions based on the testing flag.
5. agents-api/agents_api/activities/task_steps/yield_step.py:15
  • Draft comment:
    The yield_step activity manages control flow by evaluating expressions and determining the next step or workflow to transition to. Ensure that the expressions and transitions are correctly handled and that the testing flag is properly managed.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The yield_step activity in agents-api/agents_api/activities/task_steps/yield_step.py handles the logic for yielding control to another step or workflow based on the evaluation of expressions. It uses the simple_eval_dict utility function to evaluate expressions and determine the next step. The mock_yield_step is a direct reference to yield_step, indicating that no special mocking behavior is needed for testing this activity. The activity definition uses the testing flag to decide whether to use the real or mock function.
6. agents-api/agents_api/common/protocol/tasks.py:26
  • Draft comment:
    The valid_transitions dictionary is crucial for managing state transitions in task executions. Ensure that all transitions are correctly defined and that the state machine logic is robust and handles all expected and unexpected transitions appropriately.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The valid_transitions dictionary in agents-api/agents_api/common/protocol/tasks.py defines valid state transitions for task executions. It's crucial for ensuring that tasks transition through states in a controlled and predictable manner. The init state is noted as a placeholder for the start state of the state machine, which is a key piece of information for understanding the workflow logic.
7. agents-api/agents_api/env.py:91
  • Draft comment:
    Ensure that the debug and testing flags are never enabled in production environments as they could lead to the exposure of sensitive configuration details. The warning messages are a good practice but consider implementing additional safeguards to prevent these modes from being enabled in production.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is somewhat redundant because the code already includes a warning message for when debug or testing is enabled. Additionally, the suggestion to implement additional safeguards is vague and not actionable. The comment does not point out a specific code change that is required, which makes it less useful.
    I might be underestimating the importance of the comment's suggestion for additional safeguards, but without specific guidance, it remains vague. The comment could be seen as a reminder for best practices, but it doesn't align with the rules for actionable comments.
    While the suggestion for additional safeguards is a good practice, the lack of specificity makes it less actionable. The existing warning in the code already addresses the primary concern raised by the comment.
    Remove the comment as it is redundant and not actionable. The code already includes a warning for the issue mentioned, and the suggestion for additional safeguards is too vague.

Workflow ID: wflow_bUHsgDoG3b1NR0AP


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

@@ -120,11 +122,12 @@ def _(make_request=make_request, task=test_task):
assert response.status_code == 200


# FIXME: This test is failing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might happen because the transition created in test_transition does not belong to the execution created in test_execution

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is happening because of an invalid transition. It seems that at the time of running this fixture, there is already a transition with type "cancelled". And this fixture transition is of type "step".

Since "cancelled"->"step" is an invalid transition, this query fails

@whiterabbit1983 whiterabbit1983 merged commit 729a0aa into dev-tasks-disable-routes Aug 17, 2024
2 of 5 checks passed
@whiterabbit1983 whiterabbit1983 deleted the f/task-tests branch August 17, 2024 13:13
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