-
Notifications
You must be signed in to change notification settings - Fork 823
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
create and update workflow run block inworkflow execution #1419
create and update workflow run block inworkflow execution #1419
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 ae111a4 in 1 minute and 54 seconds
More details
- Looked at
748
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. alembic/versions/2024_12_20_1637-5be249d8dc96_add_failure_reason_to_workflow_run_.py:24
- Draft comment:
The new columnfailure_reason
is nullable, which is appropriate since not all workflow run blocks will have a failure reason. The use ofsa.String()
is suitable for storing text. - Reason this comment was not posted:
Confidence changes required:0%
The PR introduces a new columnfailure_reason
to theworkflow_run_blocks
table. This column is nullable, which is appropriate since not all workflow run blocks will have a failure reason. The migration script correctly adds and removes this column in the upgrade and downgrade functions, respectively. The use ofsa.String()
for the column type is suitable for storing failure reasons as text.
2. skyvern/forge/sdk/db/client.py:2029
- Draft comment:
Theupdate_workflow_run_block
function now includesfailure_reason
,task_id
, andorganization_id
as optional parameters, allowing for more detailed updates to the workflow run block. - Reason this comment was not posted:
Confidence changes required:0%
Theupdate_workflow_run_block
function now includesfailure_reason
,task_id
, andorganization_id
as optional parameters. This allows for more detailed updates to the workflow run block, which is beneficial for tracking execution details. The function checks if these parameters are provided and updates the corresponding fields in the database model. This is a good practice to ensure that only the necessary fields are updated.
3. skyvern/forge/sdk/db/client.py:2057
- Draft comment:
Theget_workflow_run_block
function retrieves a workflow run block by its ID and organization ID, raising aNotFoundError
if not found. This is useful for fetching specific blocks and handling errors appropriately. - Reason this comment was not posted:
Confidence changes required:0%
Theget_workflow_run_block
function is a new addition that retrieves a workflow run block by its ID and organization ID. This function is useful for fetching specific workflow run blocks and checking their status or other details. The function raises aNotFoundError
if the block is not found, which is a good practice for error handling.
4. skyvern/forge/sdk/db/models.py:506
- Draft comment:
Thefailure_reason
column is added toWorkflowRunBlockModel
, allowing for storing failure reasons. This is useful for debugging and analysis, and the column is nullable, which is appropriate. - Reason this comment was not posted:
Confidence changes required:0%
TheWorkflowRunBlockModel
class now includes afailure_reason
column, which aligns with the migration script. This addition allows for storing the reason for a block's failure, which is useful for debugging and analysis. The column is nullable, which is appropriate since not all blocks will have a failure reason.
5. skyvern/forge/sdk/db/utils.py:389
- Draft comment:
Theconvert_to_workflow_run_block
function now includes thefailure_reason
field, ensuring consistency and data integrity when converting models to schemas. - Reason this comment was not posted:
Confidence changes required:0%
Theconvert_to_workflow_run_block
function now includes thefailure_reason
field from theWorkflowRunBlockModel
. This ensures that the failure reason is included when converting the model to a schema, which is important for consistency and data integrity.
6. skyvern/forge/sdk/schemas/workflow_runs.py:21
- Draft comment:
TheWorkflowRunBlock
schema now includes afailure_reason
field, allowing for capturing and displaying failure reasons, which is useful for debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:0%
TheWorkflowRunBlock
schema now includes afailure_reason
field, which aligns with the changes in the database model and migration script. This addition allows for capturing and displaying the reason for a block's failure, which is useful for debugging and user feedback.
7. skyvern/forge/sdk/workflow/models/block.py:144
- Draft comment:
Thebuild_block_result
method updates theworkflow_run_block
withfailure_reason
andstatus
, ensuring accurate recording of execution details for tracking and debugging. - Reason this comment was not posted:
Confidence changes required:0%
Thebuild_block_result
method in theBlock
class now updates theworkflow_run_block
with thefailure_reason
andstatus
. This ensures that the block's execution details are accurately recorded in the database, which is crucial for tracking and debugging workflow executions.
8. skyvern/forge/sdk/workflow/models/block.py:224
- Draft comment:
Theexecute_safe
method handles exceptions by recording the failure reason and updating the block's status, ensuring errors are logged and the workflow can continue or terminate gracefully. - Reason this comment was not posted:
Confidence changes required:0%
Theexecute_safe
method in theBlock
class now handles exceptions by recording the failure reason and updating the block's status. This is a good practice for ensuring that errors are logged and the workflow can continue or terminate gracefully based on the block's configuration.
Workflow ID: wflow_HpUz6BjxGp7zaqbo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
3cb49ac
to
f56e860
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 3cb49ac in 34 seconds
More details
- Looked at
38
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/block.py:870
- Draft comment:
Theexecute_safe()
method correctly handles exceptions and sets afailure_reason
, aligning with the PR description. - Reason this comment was not posted:
Confidence changes required:0%
The PR description mentions updating theexecute_safe()
method to managefailure_reason
. The current implementation in theexecute_safe()
method does handle exceptions and sets afailure_reason
when an exception occurs, which aligns with the PR description.
2. skyvern/forge/sdk/services/observer_service.py:329
- Draft comment:
Therun_observer_cruise()
function correctly handles block failures and sets afailure_reason
, aligning with the PR description. - Reason this comment was not posted:
Confidence changes required:0%
The PR description mentions updating theexecute_workflow()
method to handle block failure reasons. The current implementation in therun_observer_cruise()
function does handle block failures and sets afailure_reason
when a block fails, which aligns with the PR description.
Workflow ID: wflow_1oWFn0gRCGz04CDT
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 f56e860 in 29 seconds
More details
- Looked at
39
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/forge/sdk/services/observer_service.py:331
- Draft comment:
Thefailure_reason
should be set to the exception message to provide more context on the failure. Consider updating thefailure_reason
to include the exception message. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_VERRR2RMev5lgvBa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
f56e860
to
3abfb75
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 3abfb75 in 36 seconds
More details
- Looked at
54
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/block.py:870
- Draft comment:
Thefailure_reason
should be updated with the exception message when an exception occurs inexecute_safe()
. Consider usingstr(e)
to capture the exception message. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_NexnrG3MAo6V3Y67
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Add
failure_reason
handling to workflow run blocks, including schema updates and execution logic changes.failure_reason
column toworkflow_run_blocks
inalembic/versions/2024_12_20_1637-5be249d8dc96_add_failure_reason_to_workflow_run_.py
.WorkflowRunBlockModel
inmodels.py
to includefailure_reason
.WorkflowRunBlock
inschemas/workflow_runs.py
to includefailure_reason
.update_workflow_run_block()
inclient.py
to handlefailure_reason
.failure_reason
handling inbuild_block_result()
inblock.py
.execute_safe()
andexecute()
methods inblock.py
to managefailure_reason
during block execution.execute_workflow()
inservice.py
to handle block failure reasons.This description was created by for 3abfb75. It will automatically update as commits are pushed.