-
Notifications
You must be signed in to change notification settings - Fork 798
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
add workflow failure reason #1198
Merged
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
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 8e8aae4 in 1 minute and 10 seconds
More details
- Looked at
491
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. alembic/versions/2024_11_15_0251-1909715536dc_add_failure_reason_column_to_workflow_.py:24
- Draft comment:
Thefailure_reason
column is added as nullable, which is appropriate since not all workflow runs will have a failure reason. The use ofsa.String()
is suitable for storing text data. - Reason this comment was not posted:
Confidence changes required:0%
The PR introduces a new columnfailure_reason
to theworkflow_runs
table. This column is nullable, which is appropriate since not all workflow runs will have a failure reason. The upgrade and downgrade functions are correctly implemented to add and remove the column, respectively. The use ofsa.String()
for the column type is appropriate for storing failure reasons as text.
2. skyvern/forge/sdk/db/client.py:1087
- Draft comment:
Theupdate_workflow_run
method now includes afailure_reason
parameter, which is necessary to support the newfailure_reason
column in the database. This change is consistent with the handling offailure_reason
throughout the codebase. - Reason this comment was not posted:
Confidence changes required:0%
Theupdate_workflow_run
method has been updated to include thefailure_reason
parameter. This is a necessary change to support the newfailure_reason
column in the database. The method correctly updates thefailure_reason
field in theWorkflowRunModel
. This change is consistent with the rest of the codebase wherefailure_reason
is being handled.
3. skyvern/forge/sdk/db/models.py:214
- Draft comment:
Thefailure_reason
field is added to theWorkflowRunModel
class, which aligns with the database schema change. This allows storing and retrieving failure reasons for workflow runs. - Reason this comment was not posted:
Confidence changes required:0%
Thefailure_reason
field is added to theWorkflowRunModel
class. This is consistent with the database schema change and allows the application to store and retrieve failure reasons for workflow runs. The field is nullable, which is appropriate since not all workflow runs will have a failure reason.
4. skyvern/forge/sdk/db/utils.py:194
- Draft comment:
Theconvert_to_workflow_run
function now includes thefailure_reason
field, ensuring theWorkflowRun
object accurately reflects the database state. - Reason this comment was not posted:
Confidence changes required:0%
Theconvert_to_workflow_run
function has been updated to include thefailure_reason
field. This ensures that theWorkflowRun
object correctly reflects the database state, including any failure reasons. This change is consistent with the rest of the codebase wherefailure_reason
is being handled.
5. skyvern/forge/sdk/workflow/models/block.py:87
- Draft comment:
TheBlockResult
class now includes afailure_reason
field, allowing for more detailed information about block execution results, especially in failure scenarios. - Reason this comment was not posted:
Confidence changes required:0%
TheBlockResult
class now includes afailure_reason
field. This change is consistent with the rest of the codebase wherefailure_reason
is being handled. It allows for more detailed information about block execution results, particularly in failure scenarios.
6. skyvern/forge/sdk/workflow/models/block.py:120
- Draft comment:
Thebuild_block_result
method now includes afailure_reason
parameter, allowing for more detailed information about block execution results, especially in failure scenarios. - Reason this comment was not posted:
Confidence changes required:0%
Thebuild_block_result
method now includes afailure_reason
parameter. This change is consistent with the rest of the codebase wherefailure_reason
is being handled. It allows for more detailed information about block execution results, particularly in failure scenarios.
7. skyvern/forge/sdk/workflow/models/block.py:164
- Draft comment:
Theexecute_safe
method now captures exceptions and sets afailure_reason
, providing more detailed information about why a block execution failed. - Reason this comment was not posted:
Confidence changes required:0%
Theexecute_safe
method in theBlock
class now captures exceptions and sets afailure_reason
. This is consistent with the rest of the codebase wherefailure_reason
is being handled. It provides more detailed information about why a block execution failed.
8. skyvern/forge/sdk/workflow/models/workflow.py:93
- Draft comment:
TheWorkflowRun
class now includes afailure_reason
field, aligning with the database schema change to store and retrieve failure reasons for workflow runs. - Reason this comment was not posted:
Confidence changes required:0%
TheWorkflowRun
class now includes afailure_reason
field. This is consistent with the database schema change and allows the application to store and retrieve failure reasons for workflow runs. The field is nullable, which is appropriate since not all workflow runs will have a failure reason.
Workflow ID: wflow_nGR4wvIntMMC1DXq
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Important
Add
failure_reason
toworkflow_runs
and update related logic to capture and store failure reasons during workflow execution.failure_reason
column toworkflow_runs
table in Alembic migration2024_11_15_0251-1909715536dc_add_failure_reason_column_to_workflow_.py
.WorkflowRunModel
inmodels.py
to includefailure_reason
.update_workflow_run()
inclient.py
to acceptfailure_reason
.execute_safe()
inblock.py
to setfailure_reason
on exceptions.execute_workflow()
inservice.py
to logfailure_reason
when blocks fail.convert_to_workflow_run()
inutils.py
to includefailure_reason
.failure_reason
toWorkflowRun
inworkflow.py
.block.py
to handlefailure_reason
.failure_reason
inservice.py
.This description was created by for 8e8aae4. It will automatically update as commits are pushed.