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

move action type validate in workflow runtime #1235

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Nov 21, 2024

Important

Move action type validation from service.py to ActionBlock in block.py, centralizing logic and improving modularity.

  • Behavior:
    • Move action type validation logic from block_yaml_to_block() in service.py to execute() in ActionBlock in block.py.
    • ActionBlock now determines prompt_template based on ActionType and handles unsupported actions by raising an exception.
    • If LLM_API_HANDLER returns an error, ActionBlock raises FailedToParseActionInstruction.
  • Imports:
    • Add FailedToParseActionInstruction and ActionType imports to block.py.
    • Remove FailedToParseActionInstruction and ActionType imports from service.py.

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

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 a883a2a in 40 seconds

More details
  • Looked at 123 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/block.py:1303
  • Draft comment:
    Consider logging the action_type after it is determined. This can help in debugging and understanding the flow of execution.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR moves the action type validation logic from block_yaml_to_block in service.py to the execute method in block.py. This change is reflected in the removal of the validation logic from block_yaml_to_block. The logic is now encapsulated within the ActionBlock class, which is a more appropriate location for this functionality, as it pertains specifically to the execution of an action block.
2. skyvern/forge/sdk/workflow/models/block.py:1326
  • Draft comment:
    The exception message here is hardcoded. Consider defining a constant or using a configuration file for such messages to maintain consistency and ease of updates across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR moves the action type validation logic from block_yaml_to_block in service.py to the execute method in block.py. This change is reflected in the removal of the validation logic from block_yaml_to_block. The logic is now encapsulated within the ActionBlock class, which is a more appropriate location for this functionality, as it pertains specifically to the execution of an action block.
3. skyvern/forge/sdk/workflow/service.py:1372
  • Draft comment:
    The prompt_template assignment has been removed from here and moved to the ActionBlock class. Ensure that this change does not affect any other logic that might depend on prompt_template being set at this point.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR moves the action type validation logic from block_yaml_to_block in service.py to the execute method in block.py. This change is reflected in the removal of the validation logic from block_yaml_to_block. The logic is now encapsulated within the ActionBlock class, which is a more appropriate location for this functionality, as it pertains specifically to the execution of an action block.

Workflow ID: wflow_kwX9nP1LAL92mumH


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

@LawyZheng LawyZheng merged commit 90f0b25 into main Nov 21, 2024
2 checks passed
@LawyZheng LawyZheng deleted the lawy/move-action-validate-to-runtime branch November 21, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant