-
Notifications
You must be signed in to change notification settings - Fork 777
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
Open
wintonzheng
wants to merge
1
commit into
main
Choose a base branch
from
shu/workflow_run_block_integration_part2_add_to_workflow_run
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
create and update workflow run block inworkflow execution #1419
wintonzheng
wants to merge
1
commit into
main
from
shu/workflow_run_block_integration_part2_add_to_workflow_run
+273
−52
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 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.
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
handling to workflow run blocks, including database 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 ae111a4. It will automatically update as commits are pushed.