-
Notifications
You must be signed in to change notification settings - Fork 907
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): Set/get steps based on workflow state #484
Conversation
There was a problem hiding this 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 cb8ce3e in 1 minute and 12 seconds
More details
- Looked at
2148
lines of code in38
files - Skipped
2
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/set_value_step.py:26
- Draft comment:
Consider raising the exception after logging it to ensure the caller is aware of the failure. - Reason this comment was not posted:
Marked as duplicate.
2. agents-api/agents_api/workflows/task_execution/__init__.py:141
- Draft comment:
Consider raising the exception after logging it to ensure the caller is aware of the failure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code already raises an exception after logging, which is exactly what the comment suggests. Therefore, the comment is redundant and not needed.
I might be overlooking a subtlety in the comment's suggestion, such as a specific type of exception or additional context that should be included.
The comment is generic and does not specify any additional context or exception type, so it seems redundant given the current code implementation.
The comment is unnecessary because the code already raises an exception after logging, fulfilling the comment's suggestion. The comment should be removed.
3. agents-api/agents_api/worker/codec.py:41
- Draft comment:
Remove or address the commented-out code for type checking to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%
Thefrom_payload_data
function incodec.py
has commented-out code for type checking. This should be addressed or removed to avoid confusion.
4. agents-api/agents_api/routers/tasks/create_or_update_task.py:31
- Draft comment:
Address the FIXME comment indicating a known bug to ensure the function works as intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a FIXME note, which is a clear indication of a code issue that needs to be addressed. This aligns with the rule that allows comments suggesting actionable code quality refactors. The comment is not asking for verification or explanation, but rather pointing out a necessary action.
The comment might be seen as redundant since the FIXME already indicates a known issue. However, it emphasizes the importance of addressing the bug, which could be useful for prioritization.
While the FIXME note already indicates an issue, the comment serves as a reminder to address it, which can be beneficial in ensuring the bug is not overlooked.
Keep the comment as it highlights the need to address a known bug, which is relevant to the changes made in the PR.
5. agents-api/agents_api/routers/sessions/chat.py:64
- Draft comment:
Address the FIXME comment indicating unimplemented features to ensure the function works as intended. - Reason this comment was not posted:
Marked as duplicate.
6. agents-api/agents_api/routers/tasks/stream_transitions_events.py:20
- Draft comment:
Address the FIXME comment indicating missing support for page tokens to ensure the function works as intended. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_OeRLe8VXSADvPhIM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this 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 fdc7bfc in 25 seconds
More details
- Looked at
173
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. agents-api/agents_api/workflows/task_execution/__init__.py:579
- Draft comment:
Avoid using print statements for debugging. Use logging with appropriate log levels instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because the diff includes print statements that are likely used for debugging. The suggestion to use logging instead is a valid code quality improvement. The comment is actionable and clear, as it suggests a specific change to improve the code.
The comment might be considered obvious to experienced developers, but it is still a valid suggestion for improving code quality. The print statements are indeed part of the changes in the diff, so the comment is relevant.
Even if the suggestion is somewhat obvious, it is still a useful reminder to maintain good coding practices. The comment is relevant to the changes made in the diff, so it should be kept.
The comment is relevant and suggests a valid code quality improvement related to the changes in the diff. It should be kept.
2. agents-api/agents_api/clients/temporal.py:57
- Draft comment:
TODO: Consider implementing search_attributes for queryability to enhance workflow querying capabilities. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comments indicate areas that need further attention or implementation. It's important to track these and ensure they are addressed in future updates.
3. agents-api/agents_api/workflows/task_execution/__init__.py:159
- Draft comment:
TODO: Consider implementing search_attributes for queryability to enhance workflow querying capabilities. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comments indicate areas that need further attention or implementation. It's important to track these and ensure they are addressed in future updates.
4. agents-api/agents_api/workflows/task_execution/__init__.py:611
- Draft comment:
TODO: Consider using a continue_as_new workflow if history grows too large to manage workflow history size effectively. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comments indicate areas that need further attention or implementation. It's important to track these and ensure they are addressed in future updates.
Workflow ID: wflow_kusWMHgykNkAZrRK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
8079f53
to
511842f
Compare
There was a problem hiding this 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 511842f in 26 seconds
More details
- Looked at
192
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/workflows/task_execution/__init__.py:580
- Draft comment:
Avoid using print statements for debugging. Consider using a logging framework instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because it addresses the use of print statements in the code, which is a change made in the diff. The rest of the file uses a logging framework, so the comment is suggesting a consistent approach to logging. This is a valid code quality suggestion.
The comment might be considered obvious since using logging frameworks is a common practice, but it is still a valid suggestion for code quality improvement.
Even if the suggestion is somewhat obvious, it is still a valid point for improving code consistency and quality.
The comment is relevant to the changes made in the diff and provides a valid suggestion for improving code quality by using a logging framework instead of print statements.
Workflow ID: wflow_G5HszCYdcukXZcs6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
511842f
to
85fccc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on 85fccc4 in 29 seconds
More details
- Looked at
312
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. agents-api/agents_api/common/utils/template.py:44
- Draft comment:
The TODO comment indicates an issue with the regex not working. Consider addressing this or removing the comment if it's no longer relevant. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comment indicates a known issue with the regex not working as expected. This should be addressed or removed if no longer relevant.
2. agents-api/agents_api/clients/temporal.py:57
- Draft comment:
Consider implementing search attributes for queryability as suggested by the TODO comment. - Reason this comment was not posted:
Confidence changes required:30%
The TODO comment suggests adding search attributes for queryability, which is a potential improvement for the workflow execution.
3. agents-api/agents_api/workflows/task_execution/__init__.py:122
- Draft comment:
Consider implementing search attributes for queryability as suggested by the TODO comment. - Reason this comment was not posted:
Confidence changes required:30%
The TODO comment suggests adding search attributes for queryability, which is a potential improvement for the workflow execution.
4. agents-api/agents_api/workflows/task_execution/__init__.py:126
- Draft comment:
Consider reviewing and optimizing the current user state storage method for efficiency as suggested by the TODO comment. - Reason this comment was not posted:
Confidence changes required:30%
The TODO comment suggests reviewing the current user state storage method for efficiency. This is a potential area for improvement.
5. agents-api/agents_api/workflows/task_execution/__init__.py:135
- Draft comment:
Consider adding endpoints for getting and setting user state as suggested by the TODO comment. - Reason this comment was not posted:
Confidence changes required:30%
The TODO comment suggests adding endpoints for getting and setting user state, which could enhance functionality.
6. agents-api/agents_api/workflows/task_execution/__init__.py:577
- Draft comment:
Consider using a continue_as_new workflow if history grows too large as suggested by the TODO comment. - Reason this comment was not posted:
Confidence changes required:30%
The TODO comment suggests using a continue_as_new workflow if history grows too large, which is a potential improvement for handling large histories.
Workflow ID: wflow_WBpYtmOosEHL73W2
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.
85fccc4
to
3b933b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on 3b933b5 in 26 seconds
More details
- Looked at
328
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. agents-api/tests/test_set_get_workflow.py:1
- Draft comment:
The file is empty. It should contain tests for the new set/get workflow functionality introduced in this PR. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because it addresses the fact that the new file added in the diff is empty and should contain tests. This is a valid point since the purpose of the file is to test new functionality introduced in the PR.
The comment could be seen as obvious since an empty test file is clearly not useful. However, it is still a valid point that needs to be addressed.
Even though the issue might seem obvious, it is important to highlight it to ensure that the necessary tests are added.
The comment should be kept because it highlights a valid issue with the new file added in the diff, which is currently empty and should contain tests.
2. agents-api/agents_api/common/utils/template.py:44
- Draft comment:
The TODO comment is vague. Consider providing more context or details about the issue with the regex. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comment indicates a known issue with the regex, but it doesn't provide any actionable information or context. It would be more helpful to describe the specific problem or remove the comment if it's not useful.
Workflow ID: wflow_TQClmZ30RplXLIml
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.
Very inefficient. A better implementation would be to create activities that get a patent workflow handle from child workflows and then do temporal queries and signals to it |
Summary:
This PR introduces
set_value_step
andget_value_step
in workflows, updates models, and adds tests for new functionality.Key points:
set_value_step
andget_value_step
in workflows.base_evaluate
for mutable values usingBox
withconversion_box=True
.set_value_step
into workflow execution logic inagents-api/agents_api/workflows/task_execution/__init__.py
.SetStep
model to use a dictionary for expressions.output
field in transitions optional.SetStep
.agents-api/tests/test_set_get_workflow.py
to cover new functionality.Generated with ❤️ by ellipsis.dev