-
Notifications
You must be signed in to change notification settings - Fork 789
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
Utilize all Workflow Run statuses #935
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.
❌ Changes requested. Reviewed everything up to 93a2c19 in 30 seconds
More details
- Looked at
407
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_yoJ8EU94aKNja8C9
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.
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 93a2c19 in 1 minute and 8 seconds
More details
- Looked at
407
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_jmRdvsH08a83jFMM
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.
Added & handled: - queued - timed_out - canceled Handled: - terminated <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > Enhance workflow run management by adding support for `queued`, `timed_out`, `canceled`, and `terminated` statuses across execution, database, and logging components. > > - **Workflow Run Statuses**: > - Added handling for `queued`, `timed_out`, `canceled`, and `terminated` statuses in `WorkflowRunStatus`. > - Updated `execute_workflow()` in `sqs_producer.py` to set status to `queued`. > - Added `mark_workflow_run_as_canceled()` in `service.py`. > - **Database**: > - Added `get_workflow_runs_stuck_in_status()` in `db_client.py` to query stuck workflow runs. > - Updated `update_stuck_workflow_runs_to_timed_out()` in `tasks.py` to handle new statuses. > - **Blocks and Execution**: > - Updated `BlockStatus` in `block.py` to include new statuses. > - Modified `execute()` in `block.py` to handle `canceled` and `terminated` statuses. > - Adjusted `execute_safe()` in `block.py` to log and handle new statuses. > - **Miscellaneous**: > - Updated constants in `config.py` for stuck workflow run timing. > > <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 32ed2bbfc16a991f2e820aae82fd71351c940c8d. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
93a2c19
to
aa7e9de
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 aa7e9de in 32 seconds
More details
- Looked at
412
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/workflow.py:65
- Draft comment:
The 'queued' status is introduced but not set anywhere in the code. Consider setting it appropriately when a workflow run is queued. - 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 to the changes made in the diff, specifically the addition of the 'queued' status. It suggests a potential oversight in the implementation where the 'queued' status is not being utilized. This is a valid point as it could affect the functionality of the workflow system if the status is not set appropriately.
The comment assumes that the 'queued' status should be set somewhere, but it doesn't provide specific guidance on where or how this should be done. It could be that the status is set in another part of the codebase not shown here.
While the comment doesn't specify where the 'queued' status should be set, it highlights a potential gap in the implementation that could lead to issues if not addressed. This makes it a useful comment to keep.
Keep the comment as it highlights a potential issue with the newly added 'queued' status not being set anywhere in the code.
2. skyvern/forge/sdk/workflow/models/workflow.py:70
- Draft comment:
The 'timed_out' status is introduced but not set anywhere in the code. Consider setting it appropriately when a workflow run times out. - 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 to the changes made in the diff, specifically the addition of the 'timed_out' status. It highlights a potential issue where this status might not be used correctly in the code, which could lead to incomplete functionality. This is a valid concern that requires attention.
The comment assumes that the 'timed_out' status should be set somewhere in the code, but it doesn't provide specific evidence that this is necessary or that it is missing. It could be that the status is handled elsewhere or is intended for future use.
While the comment doesn't provide specific evidence, it raises a valid point about ensuring that new statuses are integrated into the logic. It's better to address this potential oversight now rather than later.
Keep the comment as it highlights a potential issue with the integration of the new 'timed_out' status, which is directly related to the changes made in the diff.
Workflow ID: wflow_rAUvG8QpRV2COj8Q
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Added & handled:
Handled:
Important
Add support for
queued
,timed_out
,canceled
, andterminated
workflow run statuses across execution, database, and logging components.queued
,timed_out
,canceled
, andterminated
statuses toWorkflowRunStatus
inworkflow.py
.execute_workflow()
inservice.py
to handle new statuses.mark_workflow_run_as_canceled()
inservice.py
.get_workflow_runs_stuck_in_status()
indb_client.py
to query stuck workflow runs.update_stuck_workflow_runs_to_timed_out()
intasks.py
to handle new statuses.BlockStatus
inblock.py
to include new statuses.execute()
inblock.py
to handlecanceled
andterminated
statuses.execute_safe()
inblock.py
to log and handle new statuses.config.py
for stuck workflow run timing.This description was created by for aa7e9de. It will automatically update as commits are pushed.