feat(agents-api): Transitions stream SSE endpoint #485
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
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh 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.
❌ Changes requested. Reviewed everything up to ed4d2ec in 1 minute and 6 seconds
More details
- Looked at
1346lines of code in29files - Skipped
2files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/set_value_step.py:35
- Draft comment:
Reassigningset_value_stepto a new definition withactivity.defnis unnecessary and can be confusing. Consider defining the function once and usingactivity.defndirectly on it.
@activity.defn(name="set_value_step")
async def set_value_step(
- 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 change made in the diff, specifically the reassignment ofset_value_step. The suggestion to useactivity.defndirectly on the function is a valid code quality improvement, as it simplifies the code and reduces potential confusion.
The reassignment might have a specific purpose, such as conditional logic for testing, which the comment does not acknowledge. The comment could be missing context about why the reassignment is done this way.
Even if there is a specific reason for the reassignment, the comment still highlights a potential area for simplification and clarity, which is valuable feedback.
The comment is about a change made in the diff and provides a valid suggestion for code improvement. It should be kept.
2. agents-api/agents_api/workflows/task_execution/transition.py:21
- Draft comment:
The match-case statement for settingstate.typecan be simplified. Consider using a more concise conditional approach.
if context.is_last_step:
state.type = "finish" if context.cursor.workflow == "main" else "finish_branch"
else:
state.type = "step"
- Reason this comment was not posted:
Confidence changes required:50%
Thetransitionfunction intransition.pyuses a match-case statement that could be simplified. The match-case is used to determine the type of transition based on the context, but it could be more concise by directly setting the state type based on conditions.
3. agents-api/agents_api/models/execution/create_temporal_lookup.py:38
- Draft comment:
ConvertingUUIDtostrfordeveloper_idandexecution_idmay be redundant if these values are already strings or if the function can handleUUIDtypes directly. Consider removing unnecessary conversions. - Reason this comment was not posted:
Confidence changes required:40%
Thecreate_temporal_lookupfunction increate_temporal_lookup.pyhas a redundant conversion ofUUIDtostrfordeveloper_idandexecution_id. This conversion is unnecessary if these values are already strings or if the function that uses them can handleUUIDtypes.
Workflow ID: wflow_aRcH7zdrWaH109pL
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.
| @beartype | ||
| async def set_value_step( | ||
| context: StepContext, | ||
| additional_values: dict[str, Any] = {}, |
There was a problem hiding this comment.
Using mutable default arguments like {} for additional_values can lead to unexpected behavior. Consider using None as the default value and initializing it inside the function.
Suggested change
| additional_values: dict[str, Any] = {}, | |
| additional_values: dict[str, Any] = None, |
agents-api/agents_api/routers/tasks/stream_transitions_events.py
Outdated
Show resolved
Hide resolved
creatorrr
force-pushed
the
f/transition-stream
branch
from
ed4d2ec
to
ab427f0
Compare
Signed-off-by: Diwank Singh Tomer <[email protected]>
creatorrr
force-pushed
the
f/transition-stream
branch
from
ab427f0
to
93b4bb1
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ab427f0 in 44 seconds
More details
- Looked at
436lines of code in11files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. agents-api/agents_api/clients/temporal.py:61
- Draft comment:
Consider adding type hints for the parameters and return type of this function for better readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
Theget_workflow_handlefunction intemporal.pyis missing type hints for its parameters and return type. Adding these will improve code readability and maintainability.
2. agents-api/agents_api/models/execution/create_temporal_lookup.py:32
- Draft comment:
Ensure that the@cozo_querydecorator is properly defined and imported, as it is not shown in the provided code. This might lead to runtime errors if not handled correctly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative and not directly related to any changes made in the diff. It suggests verifying something that is not part of the code changes, which violates the rules for review comments. The decorator's presence in the unchanged code indicates it is likely already handled.
I might be overlooking the possibility that the decorator could be incorrectly defined elsewhere, but that is outside the scope of this diff review.
The focus should remain on the changes made in the diff, and the comment does not address any of those changes.
The comment should be deleted as it is speculative and not related to the changes made in the diff.
3. agents-api/agents_api/models/execution/lookup_temporal_data.py:30
- Draft comment:
Ensure that the@cozo_querydecorator is properly defined and imported, as it is not shown in the provided code. This might lead to runtime errors if not handled correctly. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/routers/tasks/create_task_execution.py:124
- Draft comment:
Consider catching more specific exceptions instead of a generalExceptionto avoid masking unexpected errors. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_task_executionfunction increate_task_execution.pyhas a try-except block that catches a generalException. It's a good practice to catch more specific exceptions to avoid masking unexpected errors.
5. agents-api/agents_api/routers/tasks/stream_transitions_events.py:100
- Draft comment:
Ensure thatnext_page_tokenis always a valid base64 string to prevent runtime errors during decoding. - Reason this comment was not posted:
Confidence changes required:50%
Thestream_transitions_eventsfunction instream_transitions_events.pyuses anext_page_tokenparameter that is base64 decoded. Ensure that the input is always a valid base64 string to prevent runtime errors.
Workflow ID: wflow_FWNS6DmNCkI0GVg3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 93b4bb1 in 33 seconds
More details
- Looked at
436lines of code in11files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. agents-api/agents_api/routers/tasks/stream_transitions_events.py:1
- Draft comment:
Thetest_set_get_workflow.pyfile is empty and should be removed if not needed. - Reason this comment was not posted:
Confidence changes required:50%
Thetest_set_get_workflow.pyfile is empty and should be removed if not needed.
2. agents-api/agents_api/models/execution/create_temporal_lookup.py:2
- Draft comment:
Theuuid4import is unused and should be removed. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_temporal_lookupfunction has an unused importuuid4which should be removed.
3. agents-api/agents_api/models/execution/create_temporal_lookup.py:1
- Draft comment:
TheTypeVarimport is unused and should be removed. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_temporal_lookupfunction has an unused importTypeVarwhich should be removed.
4. agents-api/agents_api/models/execution/lookup_temporal_data.py:1
- Draft comment:
TheAnyimport is unused and should be removed. - Reason this comment was not posted:
Confidence changes required:50%
Thelookup_temporal_datafunction has an unused importAnywhich should be removed.
5. agents-api/agents_api/models/execution/lookup_temporal_data.py:18
- Draft comment:
TheModelTimport is unused and should be removed. - Reason this comment was not posted:
Confidence changes required:50%
Thelookup_temporal_datafunction has an unused importModelTwhich should be removed.
Workflow ID: wflow_HdQm5nWXvqbSAoVW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
creatorrr
changed the title
creatorrr
changed the title
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 Transitions stream SSE endpoint, implements set/get steps in workflows, updates temporal data handling, and modifies the TypeScript SDK and tests.
Key points:
agents-api/agents_api/routers/tasks/stream_transitions_events.py.agents-api/agents_api/workflows/task_execution/__init__.py.lookup_temporal_datainagents-api/agents_api/models/execution/lookup_temporal_data.py.create_temporal_lookupto use execution ID inagents-api/agents_api/models/execution/create_temporal_lookup.py.set_value_stepfor setting values in workflows.TaskExecutionWorkflowto handleSetStepandGetStep.SetStep.agents-api/tests/test_execution_queries.py.Generated with ❤️ by ellipsis.dev