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

add workflow failure reason #1198

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Nov 15, 2024

Important

Add failure_reason to workflow_runs and update related logic to capture and store failure reasons during workflow execution.

  • Database Changes:
    • Add failure_reason column to workflow_runs table in Alembic migration 2024_11_15_0251-1909715536dc_add_failure_reason_column_to_workflow_.py.
    • Update WorkflowRunModel in models.py to include failure_reason.
  • Functionality:
    • Modify update_workflow_run() in client.py to accept failure_reason.
    • Update execute_safe() in block.py to set failure_reason on exceptions.
    • Enhance execute_workflow() in service.py to log failure_reason when blocks fail.
  • Models and Conversions:
    • Update convert_to_workflow_run() in utils.py to include failure_reason.
    • Add failure_reason to WorkflowRun in workflow.py.
  • Miscellaneous:
    • Update various block execution methods in block.py to handle failure_reason.
    • Adjust logging and error handling to incorporate failure_reason in service.py.

This description was created by Ellipsis for 8e8aae4. 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 8e8aae4 in 1 minute and 10 seconds

More details
  • Looked at 491 lines of code in 7 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:
    The failure_reason column is added as nullable, which is appropriate since not all workflow runs will have a failure reason. The use of sa.String() is suitable for storing text data.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR introduces a new column failure_reason to the workflow_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 of sa.String() for the column type is appropriate for storing failure reasons as text.
2. skyvern/forge/sdk/db/client.py:1087
  • Draft comment:
    The update_workflow_run method now includes a failure_reason parameter, which is necessary to support the new failure_reason column in the database. This change is consistent with the handling of failure_reason throughout the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The update_workflow_run method has been updated to include the failure_reason parameter. This is a necessary change to support the new failure_reason column in the database. The method correctly updates the failure_reason field in the WorkflowRunModel. This change is consistent with the rest of the codebase where failure_reason is being handled.
3. skyvern/forge/sdk/db/models.py:214
  • Draft comment:
    The failure_reason field is added to the WorkflowRunModel 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%
    The failure_reason field is added to the WorkflowRunModel 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:
    The convert_to_workflow_run function now includes the failure_reason field, ensuring the WorkflowRun object accurately reflects the database state.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The convert_to_workflow_run function has been updated to include the failure_reason field. This ensures that the WorkflowRun object correctly reflects the database state, including any failure reasons. This change is consistent with the rest of the codebase where failure_reason is being handled.
5. skyvern/forge/sdk/workflow/models/block.py:87
  • Draft comment:
    The BlockResult class now includes a failure_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%
    The BlockResult class now includes a failure_reason field. This change is consistent with the rest of the codebase where failure_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:
    The build_block_result method now includes a failure_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%
    The build_block_result method now includes a failure_reason parameter. This change is consistent with the rest of the codebase where failure_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:
    The execute_safe method now captures exceptions and sets a failure_reason, providing more detailed information about why a block execution failed.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The execute_safe method in the Block class now captures exceptions and sets a failure_reason. This is consistent with the rest of the codebase where failure_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:
    The WorkflowRun class now includes a failure_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%
    The WorkflowRun class now includes a failure_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.

@LawyZheng LawyZheng merged commit e3aa583 into main Nov 15, 2024
2 checks passed
@LawyZheng LawyZheng deleted the lawy/add-workflow-failure-reason branch November 15, 2024 03:07
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