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 select agent llm handler #1713

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 4, 2025

Important

Add SELECT_AGENT_LLM_API_HANDLER for dropdown select actions with fallback to SECONDARY_LLM_API_HANDLER.

  • Behavior:
    • Introduces SELECT_AGENT_LLM_API_HANDLER in app.py to handle select actions in dropdowns.
    • Updates select_from_dropdown and normal_select in handler.py to use SELECT_AGENT_LLM_API_HANDLER.
  • Configuration:
    • Adds SELECT_AGENT_LLM_KEY to Settings in config.py for configuring the new LLM handler.
  • Fallback Logic:
    • Uses SECONDARY_LLM_API_HANDLER as fallback if SELECT_AGENT_LLM_KEY is not set in app.py.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add `SELECT_AGENT_LLM_KEY` for select agent LLM operations, updating configurations and handlers for Azure O3 Mini support.
>
>   - **Behavior**:
>     - Introduces `SELECT_AGENT_LLM_KEY` in `config.py` for handling select agent operations.
>     - Updates `app.py` to use `SELECT_AGENT_LLM_API_HANDLER` for LLM operations, defaulting to `SECONDARY_LLM_API_HANDLER` if not set.
>     - Modifies `handler.py` to use `SELECT_AGENT_LLM_API_HANDLER` for `select_from_dropdown()` and `normal_select()` functions.
>   - **Infrastructure**:
>     - Adds `SELECT_AGENT_LLM_KEY` and related Azure O3 Mini configurations to `job-definition-production.json`, `job-definition-staging.json`, `task-definition-production.json`, `task-definition-staging.json`, `worker-staging-terraform/temporal_worker.tf`, `worker-terraform/temporal_worker.tf`, `workflow-job-definition-production.json`, and `workflow-job-definition-staging.json`.
>     - Configures Azure O3 Mini deployment, API base, and version in the above files.
>   - **Misc**:
>     - Ensures consistent environment variable setup across production and staging configurations.
>
> <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 301d344e40147877efbb5ef4466d22ea836eb303. 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 93a91e6 in 1 minute and 1 seconds

More details
  • Looked at 55 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. skyvern/forge/app.py:38
  • Draft comment:
    Good use of a fallback: SELECT_AGENT_LLM_API_HANDLER falls back to SECONDARY_LLM_API_HANDLER when SELECT_AGENT_LLM_KEY is not set. Consider adding a brief comment to document this behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is about newly added code (lines 38-42). While the comment correctly identifies the fallback behavior, suggesting documentation for such a simple and clear conditional assignment is not particularly valuable. The code is self-documenting through its clear variable names and straightforward conditional logic.
    The fallback behavior might be part of a larger system design that new developers need to understand. Documentation could help explain the hierarchy of LLM handlers.
    The code is already very clear and follows the same pattern as SECONDARY_LLM_API_HANDLER above it. Adding a comment would be redundant and could become outdated.
    Delete this comment as it suggests adding unnecessary documentation to already clear and self-documenting code.
2. skyvern/webeye/actions/handler.py:2164
  • Draft comment:
    Using SELECT_AGENT_LLM_API_HANDLER here seems aligned with the new feature. Verify that the JSON response from this handler includes the expected keys ('id', 'value', 'action_type') for downstream logic.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. skyvern/config.py:96
  • Draft comment:
    New SELECT_AGENT_LLM_KEY config added. Consider adding a doc comment to explain its purpose.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The section header "ACTIVE LLM PROVIDER" provides context. The variable follows the same pattern as SECONDARY_LLM_KEY which doesn't have a doc comment. The naming is clear and self-explanatory. Adding a doc comment here would be inconsistent with the rest of the file's style where individual config variables don't have doc comments.
    The variable's purpose might not be immediately obvious to new contributors. Documentation could help explain the relationship between the three LLM key variables.
    The file's established pattern is to use section headers for documentation rather than individual variable comments. Breaking this pattern would be inconsistent.
    The comment should be deleted as it suggests breaking the file's established documentation pattern and the variable's purpose is sufficiently clear from context.
4. skyvern/forge/app.py:35
  • Draft comment:
    Defines SELECT_AGENT_LLM_API_HANDLER with fallback to SECONDARY_LLM_API_HANDLER. Confirm that this default behavior is as intended when SELECT_AGENT_LLM_KEY is not set.
  • Reason this comment was not posted:
    Marked as duplicate.
5. skyvern/webeye/actions/handler.py:215
  • Draft comment:
    Custom select functions now always use SELECT_AGENT_LLM_API_HANDLER (removing the previous date-related branch). Verify that this refactoring aligns with intended behavior for both date-related and regular selections.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_QuM8iODxCRgotOVS


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 93a91e6 in 1 minute and 5 seconds

More details
  • Looked at 55 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. skyvern/config.py:97
  • Draft comment:
    Added SELECT_AGENT_LLM_KEY field. Consider adding comments or updating env sample files/documentation to explain its usage.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    None
2. skyvern/forge/app.py:38
  • Draft comment:
    New SELECT_AGENT_LLM_API_HANDLER created. Ensure fallback logic (using SECONDARY_LLM_API_HANDLER when SELECT_AGENT key is missing) works as intended.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
3. skyvern/webeye/actions/handler.py:2160
  • Draft comment:
    Replaced conditional LLM handler call with a unified SELECT_AGENT_LLM_API_HANDLER. Verify that this change covers all cases (including date-related inputs) as intended.
  • Reason this comment was not posted:
    Confidence changes required: 40%
    None
4. skyvern/config.py:96
  • Draft comment:
    Document the new SELECT_AGENT_LLM_KEY setting to explain its purpose and usage.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    Looking at the file, there are many config settings and most don't have detailed documentation. The purpose can be reasonably inferred from the name and context. The setting follows established patterns in the file. Adding documentation here but not elsewhere would be inconsistent.
    The setting's exact purpose might not be immediately clear to new developers. Documentation could help explain when to use this vs the other LLM keys.
    The file consistently uses self-documenting names without detailed comments. If this setting needs docs, then many others do too - this would be a larger cleanup effort.
    Delete the comment as it goes against the file's established pattern of minimal documentation and the setting's purpose can be reasonably inferred from context.
5. skyvern/forge/app.py:36
  • Draft comment:
    Verify that falling back to SECONDARY_LLM_API_HANDLER when SELECT_AGENT_LLM_KEY is not set is intentional. Consider logging which LLM handler is active for easier debugging.
  • Reason this comment was not posted:
    Comment did not seem useful.
6. skyvern/webeye/actions/handler.py:2159
  • Draft comment:
    Using SELECT_AGENT_LLM_API_HANDLER for custom select actions centralizes LLM calls. Ensure this change covers all cases (e.g. date-related pickers) previously handled differently.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. skyvern/webeye/actions/handler.py:2616
  • Draft comment:
    In normal_select, the call now uses SELECT_AGENT_LLM_API_HANDLER. Confirm that the response structure and subsequent handling work as expected with this dedicated handler.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_1ShO10FpCqtDfM0b


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

@LawyZheng LawyZheng merged commit 430c6b7 into main Feb 4, 2025
7 checks passed
@LawyZheng LawyZheng deleted the lawy/add-select-agent-llm-handler branch February 4, 2025 11:16
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