new API to the workflow run page #1400
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Incremental review on d24d9ce in 58 seconds
More details
- Looked at
649lines of code in7files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/webeye/actions/parse_actions.py:124
- Draft comment:
Redundant check foraction_type == "null". This is already covered byaction_type == ActionType.NULL_ACTION. Consider removing this check. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to suggest a redundancy that doesn't exist in the code. There is noActionType.NULL_ACTIONin the code, so the check foraction_type == "null"is not redundant. The comment appears to be incorrect.
I might be missing some context about theActionTypeenum. IfActionType.NULL_ACTIONis defined elsewhere, the comment could be valid. However, based on the provided code, it seems incorrect.
Given the provided code, there is no evidence ofActionType.NULL_ACTION, so the comment is likely incorrect. IfActionType.NULL_ACTIONexisted, it would be imported or defined in the code.
The comment is incorrect because there is noActionType.NULL_ACTIONin the code, making the check foraction_type == "null"necessary. The comment should be deleted.
Workflow ID: wflow_WsGUVdYKKzMmxxCd
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.
| modified_at=thought.modified_at, | ||
| ) | ||
| ) | ||
| workflow_run_events.sort(key=lambda x: x.created_at, reverse=True) |
There was a problem hiding this comment.
Consider optimizing the retrieval and sorting of workflow run events to handle large datasets efficiently. Sorting in memory can be inefficient for large datasets.
| @@ -325,6 +325,25 @@ async def get_task_actions(self, task_id: str, organization_id: str | None = Non | |||
| LOG.error("UnexpectedError", exc_info=True) | |||
| raise | |||
|
|
|||
| async def get_tasks_actions(self, task_ids: list[str], organization_id: str | None = None) -> list[Action]: | |||
There was a problem hiding this comment.
Handle the case where task_ids is an empty list to avoid unnecessary database queries. This is applicable to similar methods as well.
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to d24d9ce in 1 minute and 9 seconds
More details
- Looked at
649lines of code in7files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:648
- Draft comment:
Consider adding a response model to theget_workflow_run_eventsfunction to ensure consistent API responses. - Reason this comment was not posted:
Confidence changes required:50%
Theget_workflow_run_eventsfunction inagent_protocol.pyis missing a response model. This can lead to inconsistencies in the API response format and make it harder to maintain and understand the API contract.
2. skyvern/forge/sdk/routes/agent_protocol.py:701
- Draft comment:
Usingdatetime.datetime.utcnow()as a fallback forcreated_atandmodified_atcan lead to inconsistencies. Consider ensuring these fields are always set correctly in the database. - Reason this comment was not posted:
Confidence changes required:50%
Theget_tasks_actionsmethod inclient.pyandget_workflow_run_eventsinagent_protocol.pyboth usedatetime.datetime.utcnow()as a fallback forcreated_atandmodified_at. This can lead to inconsistencies if the database already has a value for these fields.
3. skyvern/forge/sdk/db/client.py:1824
- Draft comment:
Consider adding error handling forNotFoundErrorin theget_observer_cruise_thoughtsmethod to handle cases where the observer cruise is not found. - Reason this comment was not posted:
Confidence changes required:50%
Theget_observer_cruise_thoughtsmethod inclient.pyis missing error handling forNotFoundError. This can lead to unhandled exceptions if the observer cruise is not found.
Workflow ID: wflow_rp0n3zMpJkFc7yGl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
wintonzheng
force-pushed
the
shu/workflow_endpoint_v2
branch
from
d24d9ce to
7027527
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7027527 in 16 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. alembic/versions/2024_12_17_0651-cf45479f484c_add_index_to_observerthoughtmodel_db_.py:20
- Draft comment:
Consider adding docstrings to theupgradeanddowngradefunctions for clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:33%
The Alembic migration script is missing a docstring for theupgradeanddowngradefunctions, which is a best practice for clarity and maintainability.
Workflow ID: wflow_rGcYw7TFanRclvN5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| type=WorkflowRunEventType.block, | ||
| block=WorkflowRunBlock( | ||
| workflow_run_id=workflow_run_id, | ||
| block_type=BlockType.TASK, |
There was a problem hiding this comment.
why always task? not navigation/action/...?
There was a problem hiding this comment.
There's currently no way to tell right now. We need to integrate WorkflowRunTask but that has not been built.
There was a problem hiding this comment.
oh i can use navigation_goal and extraction_goal to tell. good point
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Adds `get_workflow_run_events` API endpoint and database index for `ObserverThoughtModel`, and refactors `parse_actions` function. > > - **API**: > - Adds `get_workflow_run_events` endpoint in `agent_protocol.py` to retrieve events for a workflow run, including tasks, actions, and observer thoughts. > - **Database**: > - Adds index `observer_cruise_index` to `ObserverThoughtModel` in `models.py` and corresponding Alembic migration. > - **Refactoring**: > - Moves `parse_actions` function to `parse_actions.py` from `actions.py`. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 29096b0ad914a522589edb557c6ce8f3b44bbb40. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
wintonzheng
force-pushed
the
shu/workflow_endpoint_v2
branch
from
7027527 to
43aff9b
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 43aff9b in 37 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:666
- Draft comment:
Add a check to ensuretasksis not empty before iterating over it to prevent potential errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a change that is not necessary. Iterating over an empty list in Python is safe and does not cause errors. The code handles the case where 'tasks' is empty by simply not appending any events to 'workflow_run_events'. Therefore, the comment does not point out a required code change.
I might be overlooking a specific requirement or context where an empty 'tasks' list should be handled differently, but based on standard Python behavior, iterating over an empty list is not problematic.
The standard behavior of iterating over an empty list is well-understood and does not require additional checks unless specified by additional requirements, which are not evident here.
The comment is unnecessary because iterating over an empty list is safe and does not require additional checks. The comment should be deleted.
2. skyvern/forge/sdk/routes/agent_protocol.py:698
- Draft comment:
Add a check to ensureactionsis not empty before iterating over it to prevent potential errors. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_kBCuidtm4yIBbzOb
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 53166d5 in 17 seconds
More details
- Looked at
33lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:669
- Draft comment:
Consider using a dictionary to maptask.task_typetoBlockTypeto simplify theblock_typedetermination logic. This will make the code cleaner and easier to maintain. - Reason this comment was not posted:
Confidence changes required:50%
The code for determiningblock_typein theget_workflow_run_eventsfunction is repetitive and can be simplified by using a dictionary mappingtask.task_typetoBlockType. This will make the code cleaner and easier to maintain.
Workflow ID: wflow_uVYhjJsnNS5DDv89
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 1cd472c in 17 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:729
- Draft comment:
The change in sorting order from descending to ascending might affect the logic that relies on the order of events. Ensure this change aligns with the intended behavior. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_J48oD0o7eqSoAcgQ
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 24597e7 in 21 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:1822
- Draft comment:
Consider creating a utility function for ordering bycreated_atas this pattern is repeated in multiple functions (e.g.,get_task_steps,get_tasks_actions,get_artifacts_for_observer_cruise, etc.). - Reason this comment was not posted:
Confidence changes required:50%
The order_by clause in the get_observer_cruise_thoughts function is correct and aligns with the PR description. However, the same order_by clause is used in multiple functions, which could be optimized by creating a utility function for ordering by created_at.
Workflow ID: wflow_QPiz94oFReYJ4LlT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Adds
get_workflow_run_eventsAPI endpoint, a database index forObserverThoughtModel, and refactorsparse_actionsfunction.get_workflow_run_eventsendpoint inagent_protocol.pyto retrieve events for a workflow run, including tasks, actions, and observer thoughts.observer_cruise_indextoObserverThoughtModelinmodels.pyand corresponding Alembic migration.parse_actionsfunction toparse_actions.pyfromactions.py.This description was created by
for 24597e7. It will automatically update as commits are pushed.