feat(agents-api): ALL TESTS PASS!! :D #459
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
August 16, 2024 18:39
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
… test temporal envs Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 0194c73 in 1 minute and 36 seconds
More details
- Looked at
1105lines of code in21files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. agents-api/agents_api/activities/demo.py:8
- Draft comment:
Ensure that thedemo_activityis never called in production environments as it raises an exception. This is controlled by thetestingflag, 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 inagents-api/agents_api/activities/demo.pyis designed to switch between a real activity and a mock activity based on thetestingflag. This is a common pattern in testing to avoid executing real code that could affect production systems. Thedemo_activityraises an exception if called in production, which is a safeguard against accidental execution in a non-testing environment. Themock_demo_activitysimply 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:
Theembed_docsactivity processes document snippets for embedding. Ensure that theembedderclient andembed_snippets_queryare correctly configured and that thetestingflag is properly managed to switch between real and mock implementations. - Reason this comment was not posted:
Confidence changes required:33%
Theembed_docsactivity inagents-api/agents_api/activities/embed_docs.pyis designed to handle document embedding tasks. It uses theembedderclient to generate embeddings for document snippets and stores these embeddings using theembed_snippets_query. Themock_embed_docsfunction 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 thetestingflag.
3. agents-api/agents_api/activities/task_steps/evaluate_step.py:16
- Draft comment:
Theevaluate_stepactivity evaluates expressions and returns the results. It uses thesimple_eval_dictutility function to perform the evaluation. Ensure that the expressions and context are correctly handled and that thetestingflag is properly managed. - Reason this comment was not posted:
Confidence changes required:33%
Theevaluate_stepactivity inagents-api/agents_api/activities/task_steps/evaluate_step.pyevaluates expressions based on the context provided. Themock_evaluate_stepis a direct reference toevaluate_step, indicating that no special mocking behavior is needed for testing this activity. The activity definition uses thetestingflag to decide whether to use the real or mock function.
4. agents-api/agents_api/activities/task_steps/transition_step.py:13
- Draft comment:
Thetransition_stepactivity manages state transitions within task executions. It uses thecreate_execution_transition_queryto record transitions. Ensure that the transition logic is correctly implemented and that thetestingflag is properly managed to switch between real and mock implementations. - Reason this comment was not posted:
Confidence changes required:33%
Thetransition_stepactivity inagents-api/agents_api/activities/task_steps/transition_step.pyhandles 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. Thecreate_execution_transition_queryis used to record the transition in the database. Themock_transition_stepdoes nothing, serving as a placeholder during testing. The activity definition switches between the real and mock functions based on thetestingflag.
5. agents-api/agents_api/activities/task_steps/yield_step.py:15
- Draft comment:
Theyield_stepactivity 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 thetestingflag is properly managed. - Reason this comment was not posted:
Confidence changes required:33%
Theyield_stepactivity inagents-api/agents_api/activities/task_steps/yield_step.pyhandles the logic for yielding control to another step or workflow based on the evaluation of expressions. It uses thesimple_eval_dictutility function to evaluate expressions and determine the next step. Themock_yield_stepis a direct reference toyield_step, indicating that no special mocking behavior is needed for testing this activity. The activity definition uses thetestingflag to decide whether to use the real or mock function.
6. agents-api/agents_api/common/protocol/tasks.py:26
- Draft comment:
Thevalid_transitionsdictionary 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%
Thevalid_transitionsdictionary inagents-api/agents_api/common/protocol/tasks.pydefines valid state transitions for task executions. It's crucial for ensuring that tasks transition through states in a controlled and predictable manner. Theinitstate 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 thedebugandtestingflags 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 whendebugortestingis 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 | |||
There was a problem hiding this comment.
That might happen because the transition created in test_transition does not belong to the execution created in test_execution
There was a problem hiding this comment.
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
merged commit 729a0aa
into
dev-tasks-disable-routes
2 of 5 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-apimodule.Key points:
agents-api/agents_api/activities/demo.pyfor demo activity with mock support.agents-api/agents_api/activities/embed_docs.pyto include mock activity for testing.agents-api/agents_api/activities/task_stepsto use mock activities when testing.agents-api/agents_api/env.pyto print environment variables and warn if testing is enabled.agents-api/agents_api/models/executionto fix transition validation and update logic.agents-api/agents_api/workflows/demo.pyfor demo workflow execution.agents-api/tests/to support testing with Temporal.Generated with ❤️ by ellipsis.dev