Skip to content
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

Merged

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Dec 20, 2024

Important

Add failure_reason handling to workflow run blocks, including schema updates and execution logic changes.

  • Database Schema:
    • Add failure_reason column to workflow_run_blocks in alembic/versions/2024_12_20_1637-5be249d8dc96_add_failure_reason_to_workflow_run_.py.
  • Models:
    • Update WorkflowRunBlockModel in models.py to include failure_reason.
    • Update WorkflowRunBlock in schemas/workflow_runs.py to include failure_reason.
  • Functions:
    • Modify update_workflow_run_block() in client.py to handle failure_reason.
    • Add failure_reason handling in build_block_result() in block.py.
  • Workflow Execution:
    • Update execute_safe() and execute() methods in block.py to manage failure_reason during block execution.
    • Update execute_workflow() in service.py to handle block failure reasons.

This description was created by Ellipsis for 3abfb75. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 7 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 column failure_reason is nullable, which is appropriate since not all workflow run blocks will have a failure reason. The use of sa.String() is suitable for storing text.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR introduces a new column failure_reason to the workflow_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 of sa.String() for the column type is suitable for storing failure reasons as text.
2. skyvern/forge/sdk/db/client.py:2029
  • Draft comment:
    The update_workflow_run_block function now includes failure_reason, task_id, and organization_id as optional parameters, allowing for more detailed updates to the workflow run block.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The update_workflow_run_block function now includes failure_reason, task_id, and organization_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:
    The get_workflow_run_block function retrieves a workflow run block by its ID and organization ID, raising a NotFoundError if not found. This is useful for fetching specific blocks and handling errors appropriately.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The get_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 a NotFoundError if the block is not found, which is a good practice for error handling.
4. skyvern/forge/sdk/db/models.py:506
  • Draft comment:
    The failure_reason column is added to WorkflowRunBlockModel, 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%
    The WorkflowRunBlockModel class now includes a failure_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:
    The convert_to_workflow_run_block function now includes the failure_reason field, ensuring consistency and data integrity when converting models to schemas.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The convert_to_workflow_run_block function now includes the failure_reason field from the WorkflowRunBlockModel. 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:
    The WorkflowRunBlock schema now includes a failure_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%
    The WorkflowRunBlock schema now includes a failure_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:
    The build_block_result method updates the workflow_run_block with failure_reason and status, ensuring accurate recording of execution details for tracking and debugging.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The build_block_result method in the Block class now updates the workflow_run_block with the failure_reason and status. 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:
    The execute_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%
    The execute_safe method in the Block 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.

@wintonzheng wintonzheng force-pushed the shu/workflow_run_block_integration_part2_add_to_workflow_run branch from 3cb49ac to f56e860 Compare December 22, 2024 19:13
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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:
    The execute_safe() method correctly handles exceptions and sets a failure_reason, aligning with the PR description.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR description mentions updating the execute_safe() method to manage failure_reason. The current implementation in the execute_safe() method does handle exceptions and sets a failure_reason when an exception occurs, which aligns with the PR description.
2. skyvern/forge/sdk/services/observer_service.py:329
  • Draft comment:
    The run_observer_cruise() function correctly handles block failures and sets a failure_reason, aligning with the PR description.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR description mentions updating the execute_workflow() method to handle block failure reasons. The current implementation in the run_observer_cruise() function does handle block failures and sets a failure_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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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:
    The failure_reason should be set to the exception message to provide more context on the failure. Consider updating the failure_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.

@wintonzheng wintonzheng force-pushed the shu/workflow_run_block_integration_part2_add_to_workflow_run branch from f56e860 to 3abfb75 Compare December 22, 2024 19:14
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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:
    The failure_reason should be updated with the exception message when an exception occurs in execute_safe(). Consider using str(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.

@wintonzheng wintonzheng merged commit b256bac into main Dec 22, 2024
2 checks passed
@wintonzheng wintonzheng deleted the shu/workflow_run_block_integration_part2_add_to_workflow_run branch December 22, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant