Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

task v2 refactor part 2: observer schema py -> task_v2 schema py #1814

Merged
merged 2 commits into from
Feb 22, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 22, 2025

Important

Refactor to rename observers.py schema to task_v2.py and update all related imports across the codebase.

  • Schema Renaming:
    • Rename observers.py to task_v2.py.
    • Update imports in __init__.py, create_webvoyager_observer.py, and local.py to use task_v2 instead of observers.
  • Code Updates:
    • Replace ObserverTask, ObserverTaskRequest, ObserverTaskStatus, and ObserverThought imports from observers to task_v2 in api_handler_factory.py, models.py, and manager.py.
    • Update client.py and async_executor.py to use task_v2 schema.
    • Modify agent_protocol.py to reflect changes in schema imports.
    • Adjust workflow_runs.py to import ObserverThought from task_v2.
    • Update task_v2_service.py to import necessary classes from task_v2.
    • Change block.py and workflow.py to use task_v2 schema.
  • Miscellaneous:
    • Ensure all references to ObserverThoughtType and related classes are updated to the new schema path.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Refactor schema imports from `observers` to `task_v2` across the codebase, including renaming the `observers.py` file to `task_v2.py`.
>
>   - **Schema Refactor**:
>     - Rename import path from `skyvern.forge.sdk.schemas.observers` to `skyvern.forge.sdk.schemas.task_v2` for `ObserverTask`, `ObserverTaskRequest`, `ObserverTaskStatus`, `ObserverThought`, and `ObserverThoughtType`.
>     - Affected files include `cloud_async_executor.py`, `observer_cruises.py`, `run_local_observer.py`, `api_handler_factory.py`, and `models.py` among others.
>   - **File Rename**:
>     - Rename `skyvern/forge/sdk/schemas/observers.py` to `skyvern/forge/sdk/schemas/task_v2.py`.
>
> <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 f70a418064c2f15b69bead47fc7ef5a3c774be34. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Refactor schema imports from `observers` to `task_v2` across the codebase, including renaming the `observers.py` file to `task_v2.py`.
>
>   - **Schema Refactor**:
>     - Rename import path from `skyvern.forge.sdk.schemas.observers` to `skyvern.forge.sdk.schemas.task_v2` for `ObserverTask`, `ObserverTaskRequest`, `ObserverTaskStatus`, `ObserverThought`, and `ObserverThoughtType`.
>     - Affected files include `cloud_async_executor.py`, `observer_cruises.py`, `run_local_observer.py`, `api_handler_factory.py`, and `models.py` among others.
>   - **File Rename**:
>     - Rename `skyvern/forge/sdk/schemas/observers.py` to `skyvern/forge/sdk/schemas/task_v2.py`.
>
> <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 f70a418064c2f15b69bead47fc7ef5a3c774be34. 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 72b1117 in 58 seconds

More details
  • Looked at 26 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. evaluation/core/__init__.py:15
  • Draft comment:
    Update import: Use task_v2 module instead of observers; verify dependent code follows new schema.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
    The comment suggests updating an import and verifying dependent code. The suggestion to update the import is specific, but asking to verify dependent code is not allowed as it falls under asking the author to ensure compatibility, which violates the rules.
2. evaluation/script/create_webvoyager_observer.py:12
  • Draft comment:
    Update import: Use task_v2 module for ObserverTaskRequest instead of observers.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. evaluation/core/__init__.py:15
  • Draft comment:
    Ensure that the new import from task_v2 is fully compatible with previous usage of ObserverTask and ObserverTaskRequest.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. evaluation/script/create_webvoyager_observer.py:12
  • Draft comment:
    Confirm the new observer schema import from task_v2 matches our expected model structure.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_X3uf5ZgtBhBhMIAv


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 72b1117 in 2 minutes and 3 seconds

More details
  • Looked at 248 lines of code in 18 files
  • Skipped 0 files when reviewing.
  • Skipped posting 21 drafted comments based on config settings.
1. skyvern/forge/sdk/services/task_v2_service.py:1420
  • Draft comment:
    Consider reusing an httpx.AsyncClient instance (or using a context manager) instead of creating a new one in send_observer_task_webhook to avoid potential resource leaks.
  • 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.
2. skyvern/forge/sdk/services/task_v2_service.py:1169
  • Draft comment:
    Instead of seeding the global random module with os.urandom for generating random strings, consider using the secrets module (e.g. secrets.token_urlsafe) for cryptographically secure and isolated random values.
  • 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.
3. skyvern/forge/sdk/services/task_v2_service.py:1240
  • Draft comment:
    Review the exception handling in initialize_observer_task; more detailed logging and context can help diagnose failures in creating observer tasks and workflow runs.
  • 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.
4. skyvern/forge/sdk/workflow/models/block.py:172
  • Draft comment:
    When rendering templates in format_block_parameter_template_from_workflow_run_context, ensure that any user-controlled input is sanitized to prevent potential template injection vulnerabilities.
  • 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.
5. skyvern/forge/sdk/workflow/models/workflow.py:40
  • Draft comment:
    Consider integrating duplicate block label validation into a Pydantic validator (or custom init) rather than a separately called method. This would ensure the validation is automatic upon model instantiation.
  • 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.
6. skyvern/forge/sdk/workflow/models/workflow.py:90
  • Draft comment:
    Consider documenting the 'is_final' method in WorkflowRunStatus to clarify its purpose and the rationale for status membership.
  • Reason this comment was not posted:
    Confidence changes required: 40% <= threshold 50%
    None
7. skyvern/forge/sdk/workflow/models/workflow.py:15
  • Draft comment:
    Consider adding module- and class-level docstrings to enhance clarity and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
8. evaluation/core/__init__.py:125
  • Draft comment:
    Typo in error message: the assert message currently shows 'but got NONE' at line 125. It should use 'None' (proper case) to match Python's standard representation.
  • 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.
9. evaluation/script/create_webvoyager_observer.py:55
  • Draft comment:
    Typographical improvement: In the print statement on line 55, the sentence 'saving the records file in {record_file_path}' begins with a lowercase 'saving' after a period. Consider capitalizing it or rephrasing for clarity (e.g., 'Queued {cnt} cruises to launch webvoyager evaluation test. Saved the records file in {record_file_path}').
  • 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.
10. skyvern/agent/local.py:36
  • Draft comment:
    Typo: In the comment on line 36, 'OrganizationAutoToken' should be corrected to 'OrganizationAuthToken' for consistency with the token naming elsewhere in the file.
  • 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.
11. skyvern/forge/sdk/api/llm/models.py:87
  • Draft comment:
    There appears to be a typo in the parameter name 'observer_cruise' (line 88). Given its type is 'ObserverTask', it might be more consistent to rename it to 'observer_task' to avoid confusion.
  • 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.
12. skyvern/forge/sdk/artifact/storage/base.py:10
  • Draft comment:
    Typographical error: Rename 'FILE_EXTENTSION_MAP' to 'FILE_EXTENSION_MAP' to correct the spelling.
  • 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.
13. skyvern/forge/sdk/artifact/storage/local.py:11
  • Draft comment:
    Typo noticed: 'FILE_EXTENTSION_MAP' seems to be misspelled. Consider renaming it to 'FILE_EXTENSION_MAP' if this was unintentional for clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
14. skyvern/forge/sdk/artifact/storage/s3.py:16
  • Draft comment:
    Typographical error: The constant name FILE_EXTENTSION_MAP appears to be misspelled. Consider renaming it to FILE_EXTENSION_MAP for better clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
15. skyvern/forge/sdk/artifact/storage/s3.py:136
  • Draft comment:
    Typographical error: The variable name 'presigned_urils' seems to be misspelled. It should likely be 'presigned_urls' to reflect its content correctly.
  • 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.
16. skyvern/forge/sdk/db/client.py:2034
  • Draft comment:
    In get_totp_codes, the variable used to store the result of session.scalars is named 'totp_code' and then is reused in the list comprehension as the loop variable. This shadowing can be confusing for readers and might lead to errors. It is recommended to rename the loop variable (e.g., 'code') to improve clarity.
  • 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.
17. skyvern/forge/sdk/db/models.py:237
  • Draft comment:
    Typo in comment: 'workfow runs with parent_workflow_run_id' should be 'workflow runs with parent_workflow_run_id'. Please correct this small mistake.
  • 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.
18. skyvern/forge/sdk/services/task_v2_service.py:81
  • Draft comment:
    Typographical error: The word 'extrat' should be corrected to 'extract' in the description of the data extraction schema.
  • 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.
19. skyvern/forge/sdk/services/task_v2_service.py:187
  • Draft comment:
    Typographical error: In the comment, 'oserver cruise' should be corrected to 'observer cruise'.
  • 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.
20. skyvern/forge/sdk/services/task_v2_service.py:333
  • Draft comment:
    Typographical error: 'max_iterationss_override' in the log statement should be corrected to 'max_iterations_override'.
  • 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.
21. skyvern/forge/sdk/services/task_v2_service.py:515
  • Draft comment:
    Typographical error: In the comment, 'parse observer repsonse' should be corrected to 'parse observer response'.
  • 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_M0kdi4eSjjqOFRUZ


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

@wintonzheng wintonzheng merged commit 713005f into main Feb 22, 2025
7 checks passed
@wintonzheng wintonzheng deleted the shu/refactor_task_v2_part2 branch February 22, 2025 11:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant