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

fix max_iterations_override #1733

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 6, 2025

Important

Update x_max_iterations_override type in observer_task to accept int, str, or None.

  • Behavior:
    • Update x_max_iterations_override type in observer_task to accept int, str, or None.
  • Logging:
    • Logs max iterations override value in observer_task if provided.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Allow `max_iterations_override` to be `int`, `str`, or `None` in `cloud_async_executor.py`, `agent_protocol.py`, and `temporal_workflow_parameters.py`.
>
>   - **Behavior**:
>     - `max_iterations_override` parameter in `execute_cruise()` in `cloud_async_executor.py` now accepts `int`, `str`, or `None`.
>     - `x_max_iterations_override` in `observer_task()` in `agent_protocol.py` updated to accept `int`, `str`, or `None`.
>   - **Parameters**:
>     - `RunObserverCruiseParams` in `temporal_workflow_parameters.py` updated to allow `max_iterations_override` as `int`, `str`, or `None`.
>   - **Misc**:
>     - Converts `max_iterations_override` to string in `execute_cruise()` if not `None`.
>
> <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 af7d96e106b68cabe37978d512777db1d99da274. 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.

❌ Changes requested. Reviewed everything up to 2c7155d in 51 seconds

More details
  • Looked at 13 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/agent_protocol.py:1196
  • Draft comment:
    Accepting both int and str is fine if intentional, but consider converting a str to int before use. Without conversion, a string header might lead to unexpected behavior when used as numeric override.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_6K1cyeL5XKm36xMz


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.

@@ -1193,7 +1193,7 @@ async def observer_task(
background_tasks: BackgroundTasks,
data: ObserverTaskRequest,
organization: Organization = Depends(org_auth_service.get_current_org),
x_max_iterations_override: Annotated[int | None, Header()] = None,
x_max_iterations_override: Annotated[int | str | None, Header()] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing x_max_iterations_override to be int | str may lead to inconsistent behavior. If an integer override is expected, consider converting the header value to int explicitly and using an explicit None check (e.g. 'if x_max_iterations_override is not None') to avoid misinterpreting 0. Also, align its type with similar parameters (like x_max_steps_override).

@wintonzheng wintonzheng merged commit b0d9242 into main Feb 6, 2025
6 of 7 checks passed
@wintonzheng wintonzheng deleted the shu/fix_observer_max_iterations_override branch February 6, 2025 04:08
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 2c7155d in 56 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:1196
  • Draft comment:
    Consider converting string header input to int. Allowing int|str is fine for flexibility, but later usage may expect numeric value. If a string is received, explicitly cast it to int or validate it to avoid unexpected behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The comment raises a valid concern about type safety. However, I don't see strong evidence that this will cause issues - the value is only used for logging and passed through to another function. Without seeing how execute_cruise() uses this value, I can't be certain that string values would cause problems. The type change to allow strings may have been intentional.
    I don't have visibility into how execute_cruise() uses this value. The type change to allow strings may have been done deliberately to support string inputs.
    While type safety is good, without seeing the full context of how this value is used, I can't be confident that requiring int conversion is the right approach.
    Delete the comment since we don't have strong evidence that string values would cause problems, and the type change may have been intentional.
2. skyvern/forge/sdk/routes/agent_protocol.py:1196
  • Draft comment:
    Allowing x_max_iterations_override to be a string is useful since HTTP headers are strings, but ensure that if an integer is required downstream, you validate and convert this value appropriately.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_DlJJ7dmWjUYqlWN9


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

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