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

workflow run streaming API #887

Merged
merged 2 commits into from
Sep 30, 2024
Merged

workflow run streaming API #887

merged 2 commits into from
Sep 30, 2024

Conversation

ykeremy
Copy link
Contributor

@ykeremy ykeremy commented Sep 30, 2024

Important

Adds workflow_run_streaming WebSocket endpoint in streaming.py for authenticated streaming of workflow run data with error handling.

  • New Feature:
    • Adds workflow_run_streaming WebSocket endpoint in streaming.py for streaming workflow run data.
  • Authentication:
    • Validates apikey or token for organization access.
    • Sends error message if credentials are invalid.
  • Streaming Logic:
    • Streams workflow run data if status is running.
    • Sends base64-encoded screenshot if available.
    • Closes connection if no activity for 5 minutes or if workflow run is in a final state.
  • Error Handling:
    • Handles ValidationError, WebSocketDisconnect, and ConnectionClosedOK exceptions with appropriate logging and messaging.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Adds a WebSocket endpoint for streaming workflow run data with authentication, streaming logic, and error handling in `streaming.py`.
>
>   - **New Feature**:
>     - Adds `workflow_run_streaming` WebSocket endpoint in `streaming.py` for streaming workflow run data.
>   - **Authentication**:
>     - Validates `apikey` or `token` for organization access.
>     - Sends error message if credentials are invalid.
>   - **Streaming Logic**:
>     - Streams workflow run data if status is `running`.
>     - Sends base64-encoded screenshot if available.
>     - Closes connection if no activity for 5 minutes or if workflow run is in a final state.
>   - **Error Handling**:
>     - Handles `ValidationError`, `WebSocketDisconnect`, and `ConnectionClosedOK` exceptions with appropriate logging and messaging.
>
> <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 5998bf39c47d0e092a3245e6ef732209c44a2636. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
@ykeremy ykeremy added the sync label Sep 30, 2024
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.

❌ Changes requested. Incremental review on d7b6cb4 in 18 seconds

More details
  • Looked at 147 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/streaming.py:135
  • Draft comment:
    Typo in log message: "WofklowRun" should be "WorkflowRun". This typo appears in multiple log messages.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The code has a typo in the log messages for 'WorkflowRun Streaming'.

Workflow ID: wflow_BGwfAg4tH7ngQ9oa


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.

)
return

if WorkflowRun.status == WorkflowRunStatus.running:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect status check. Use workflow_run.status instead of WorkflowRun.status. This issue appears in other parts of the code as well.

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.

❌ Changes requested. Reviewed everything up to d7b6cb4 in 20 seconds

More details
  • Looked at 147 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/streaming.py:135
  • Draft comment:
    Typo in log message: "WofklowRun" should be "WorkflowRun". This typo appears in multiple log messages throughout the function.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code has a typo in the log messages for "WorkflowRun" which is consistently misspelled as "WofklowRun". This should be corrected for clarity and consistency.

Workflow ID: wflow_6OsaomyFEW3O1yDP


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.

)
return

if WorkflowRun.status == WorkflowRunStatus.running:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logical error: Use workflow_run.status instead of WorkflowRun.status to check the status of the current workflow run instance.

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 e932ccc in 16 seconds

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

Workflow ID: wflow_eFAsveiB84K7epAq


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

@wintonzheng wintonzheng merged commit 5f4089e into main Sep 30, 2024
2 checks passed
@wintonzheng wintonzheng deleted the shu/workflow_streaming_api branch September 30, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants