workflow run block #1332

wintonzheng · 2024-12-06T00:19:49Z

Important

Add WorkflowRunBlockModel and migration for managing workflow run blocks with ID generation and foreign key constraints.

Models:
- Add WorkflowRunBlockModel to models.py with fields: workflow_run_block_id, workflow_run_id, parent_workflow_run_block_id, organization_id, task_id, label, block_type, status, output, continue_on_failure, created_at, modified_at.
- Includes index wfrb_org_wfr_index on organization_id and workflow_run_id.
Database Migration:
- New Alembic migration introduce_workflow_run_blocks.py to create workflow_run_blocks table with specified fields and index.
- Handles foreign key constraints for organization_id, parent_workflow_run_block_id, task_id, and workflow_run_id.
ID Generation:
- Add generate_workflow_run_block_id() in id.py for generating IDs with WORKFLOW_RUN_BLOCK_PREFIX.

^{This description was created by}^{for 91ed435. It will automatically update as commits are pushed.}

> [!IMPORTANT] > Add `WorkflowRunBlockModel` to manage workflow run blocks in `models.py`. > > - **Models**: > - Add `WorkflowRunBlockModel` to `models.py` for managing workflow run blocks. > - Includes fields: `workflow_run_block_id`, `organization_id`, `workflow_run_id`, `block_type`, `status`, `created_at`, `modified_at`. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 73494f8bdd823778774aad7cdc4e4f9c6aca0a03. It will automatically update as commits are pushed.</sup>

ellipsis-dev

❌ Changes requested. Incremental review on 8155053 in 53 seconds

More details

Looked at 29 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. skyvern/forge/sdk/db/models.py:491

Draft comment:
Consider using an Enum for the status field to ensure consistency and prevent invalid status values. This is applicable to other models with a status field as well.
Reason this comment was not posted:
Based on historical feedback, this comment is too similar to comments previously marked by users as bad.

Workflow ID: wflow_IXYYeQi8gbPKIikG

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2024-12-06T00:20:53Z

skyvern/forge/sdk/db/models.py

+    __tablename__ = "workflow_run_blocks"
+    __table_args__ = (Index("wfrb_org_wfr_index", "organization_id", "workflow_run_id"),)
+
+    workflow_run_block_id = Column(String, primary_key=True)


Consider adding a default value generator for workflow_run_block_id to ensure uniqueness and consistency, similar to other ID fields in the codebase.

ellipsis-dev · 2024-12-06T00:20:53Z

skyvern/forge/sdk/db/models.py

+    organization_id = Column(String, ForeignKey("organizations.organization_id"), nullable=True)
+    task_id = Column(String, ForeignKey("tasks.task_id"), nullable=True)
+    label = Column(String, nullable=False)
+    block_type = Column(String, nullable=False)


Consider using an Enum for the block_type field to ensure consistency and prevent invalid block type values.

ellipsis-dev

❌ Changes requested. Reviewed everything up to 8155053 in 55 seconds

More details

Looked at 29 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_I8AMhmEtNwMebun0

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2024-12-06T00:20:57Z

skyvern/forge/sdk/db/models.py

+    __tablename__ = "workflow_run_blocks"
+    __table_args__ = (Index("wfrb_org_wfr_index", "organization_id", "workflow_run_id"),)
+
+    workflow_run_block_id = Column(String, primary_key=True)


Consider adding a default value generator for workflow_run_block_id to ensure uniqueness and consistency.

ellipsis-dev · 2024-12-06T00:20:57Z

skyvern/forge/sdk/db/models.py

+    task_id = Column(String, ForeignKey("tasks.task_id"), nullable=True)
+    label = Column(String, nullable=False)
+    block_type = Column(String, nullable=False)
+    status = Column(String, nullable=False)


Consider using an Enum for the status field to ensure consistency and validation. This is applicable in other models as well where status is used as a String (e.g., TaskModel, StepModel, WorkflowRunModel).

ellipsis-dev · 2024-12-06T00:20:57Z

skyvern/forge/sdk/db/models.py

+    output = Column(JSON, nullable=True)
+    continue_on_failure = Column(Boolean, nullable=False, default=False)
+
+    created_at = Column(DateTime, default=datetime.datetime.utcnow, nullable=False)


Consider using timezone-aware datetime for created_at and modified_at fields for consistency and to avoid potential issues with timezones.

ellipsis-dev

❌ Changes requested. Incremental review on f781e15 in 40 seconds

More details

Looked at 70 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_S9NIChNYUCOLJZRE

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2024-12-06T00:50:06Z

alembic/versions/2024_12_06_0048-7581e473eaf1_introduce_workflow_run_blocks.py

+        sa.Column("workflow_run_block_id", sa.String(), nullable=False),
+        sa.Column("workflow_run_id", sa.String(), nullable=False),
+        sa.Column("parent_workflow_run_block_id", sa.String(), nullable=True),
+        sa.Column("organization_id", sa.String(), nullable=True),


Consider making 'organization_id' non-nullable or revising the index 'wfrb_org_wfr_index' to handle nullable columns appropriately.

ellipsis-dev

❌ Changes requested. Incremental review on dd33318 in 48 seconds

More details

Looked at 46 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_vzEpO2H4yLWOfBqC

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

skyvern/forge/sdk/db/id.py

ellipsis-dev

👍 Looks good to me! Incremental review on 41371b4 in 19 seconds

More details

Looked at 46 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. skyvern/forge/sdk/db/models.py:483

Draft comment:
Consider reviewing the nullable=True setting for organization_id. If every workflow run block should be associated with an organization, this should be nullable=False.
Reason this comment was not posted:
Confidence changes required: 50%
The PR introduces a new model WorkflowRunBlockModel with a default ID generation function. The ID generation function is correctly implemented and used as a default for the primary key. The model also includes appropriate foreign key constraints and nullable settings. However, the organization_id field is nullable, which might not be intended if every workflow run block should be associated with an organization. This should be clarified.

Workflow ID: wflow_V0P70xN7k76NTrpn

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 91ed435 in 24 seconds

More details

Looked at 49 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 2 drafted comments based on config settings.

1. alembic/versions/2024_12_06_0113-de0254717601_introduce_workflow_run_blocks.py:31

Draft comment:
The label field was changed to nullable=True. Ensure this change is intentional and consistent with the model definition.
Reason this comment was not posted:
Confidence changes required: 50%
The label field in the WorkflowRunBlockModel was changed from nullable=False to nullable=True in both the model and the migration file. This change should be consistent across both files.

2. skyvern/forge/sdk/db/models.py:491

Draft comment:
The label field was changed to nullable=True. Ensure this change is intentional and consistent with the migration file.
Reason this comment was not posted:
Confidence changes required: 50%
The label field in the WorkflowRunBlockModel was changed from nullable=False to nullable=True in both the model and the migration file. This change should be consistent across both files.

Workflow ID: wflow_Kf3Ryqmca57fWvcs

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

wintonzheng added the sync label Dec 6, 2024

ellipsis-dev bot reviewed Dec 6, 2024

View reviewed changes

introduce WorkflowRunBlockModel

f781e15

ellipsis-dev bot reviewed Dec 6, 2024

View reviewed changes

skyvern/forge/sdk/db/id.py Show resolved Hide resolved

generate_workflow_run_block_id

41371b4

wintonzheng force-pushed the shu/workflow_run_block branch from dd33318 to 41371b4 Compare December 6, 2024 01:06

ellipsis-dev bot reviewed Dec 6, 2024

View reviewed changes

label not required

91ed435

wintonzheng merged commit 01e9678 into main Dec 6, 2024
2 checks passed

wintonzheng deleted the shu/workflow_run_block branch December 6, 2024 01:14

ellipsis-dev bot reviewed Dec 6, 2024

View reviewed changes

satansdeer pushed a commit to satansdeer/skyvern that referenced this pull request Dec 12, 2024

workflow run block (Skyvern-AI#1332)

8bf863c

wintonzheng mentioned this pull request Dec 17, 2024

shu/maksimi/record logs new #1399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workflow run block #1332

workflow run block #1332

wintonzheng commented Dec 6, 2024 •

edited by ellipsis-dev bot

Loading

ellipsis-dev bot left a comment

ellipsis-dev bot Dec 6, 2024

ellipsis-dev bot Dec 6, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot Dec 6, 2024

ellipsis-dev bot Dec 6, 2024

ellipsis-dev bot Dec 6, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot Dec 6, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

workflow run block #1332

workflow run block #1332

Conversation

wintonzheng commented Dec 6, 2024 • edited by ellipsis-dev bot Loading

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Dec 6, 2024

Choose a reason for hiding this comment

ellipsis-dev bot Dec 6, 2024

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Dec 6, 2024

Choose a reason for hiding this comment

ellipsis-dev bot Dec 6, 2024

Choose a reason for hiding this comment

ellipsis-dev bot Dec 6, 2024

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Dec 6, 2024

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

wintonzheng commented Dec 6, 2024 •

edited by ellipsis-dev bot

Loading