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

fail observer and workerflow runs if observer max iterations reached #1507

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Jan 6, 2025

Important

Fail observer and workflow runs if max iterations are reached, updating statuses and logging in observer_service.py.

  • Behavior:
    • Fail observer and workflow runs if max iterations are reached in run_observer_cruise_helper().
    • Log failure reason "Observer max iterations reached" and update status to failed.
  • Functions:
    • Add mark_observer_cruise_as_completed() to update observer cruise status to completed.
    • Modify run_observer_cruise() to check if observer cruise status is running before marking as completed.
  • Logging:
    • Add log message for max iterations reached in run_observer_cruise_helper().
    • Adjust logging in run_observer_cruise() to reflect status checks.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Fail observer and workflow runs if max iterations are reached, updating statuses and logging appropriately in `observer_service.py`.
>
>   - **Behavior**:
>     - Fail observer and workflow runs if max iterations are reached in `run_observer_cruise_helper()`.
>     - Log failure reason "Observer max iterations reached" and update status to failed.
>   - **Functions**:
>     - Add `mark_observer_cruise_as_completed()` to update observer cruise status to completed.
>     - Modify `run_observer_cruise()` to check if observer cruise status is running before marking as completed.
>   - **Logging**:
>     - Add log message for max iterations reached in `run_observer_cruise_helper()`.
>     - Adjust logging in `run_observer_cruise()` to reflect status checks.
>
> <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 3332efe216ab97214ea5bd1f86eaabeb6eb47c2b. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
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 c13aecf in 28 seconds

More details
  • Looked at 92 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern/forge/sdk/services/observer_service.py:190
  • Draft comment:
    Consider updating the observer cruise status to 'failed' when the observer cruise is not found, to maintain consistency with the behavior described in the PR.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
2. skyvern/forge/sdk/services/observer_service.py:224
  • Draft comment:
    The warning message 'Workflow or workflow run not found' is logged, but it might be more informative to include the observer cruise ID and organization ID in the log for better traceability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a change to fail observer and workflow runs if max iterations are reached. However, the code does not update the observer cruise status to 'failed' in the run_observer_cruise function when the observer cruise is not found. This is inconsistent with the behavior described in the PR description.
3. skyvern/forge/sdk/services/observer_service.py:548
  • Draft comment:
    The log message 'Observer cruise failed - run out of iterations' should be logged as an error instead of info, as it indicates a failure condition.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a change to fail observer and workflow runs if max iterations are reached. However, the code does not update the observer cruise status to 'failed' in the run_observer_cruise function when the observer cruise is not found. This is inconsistent with the behavior described in the PR description.

Workflow ID: wflow_1lUfzShi8BzWkw6Q


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! Reviewed everything up to c13aecf in 1 minute and 29 seconds

More details
  • Looked at 92 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern/forge/sdk/services/observer_service.py:541
  • Draft comment:
    Consider using mark_observer_cruise_as_completed instead of directly calling app.WORKFLOW_SERVICE.mark_workflow_run_as_completed for consistency.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. skyvern/forge/sdk/services/observer_service.py:553
  • Draft comment:
    Consider using mark_observer_cruise_as_failed instead of directly calling app.WORKFLOW_SERVICE.mark_workflow_run_as_failed for consistency.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_a8rYCsuTlwJjoxVZ


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

@wintonzheng wintonzheng merged commit 897579c into main Jan 7, 2025
2 checks passed
@wintonzheng wintonzheng deleted the shu/fail_observer_if_max_iterations_reached branch January 7, 2025 00:42
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.

1 participant