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

feat(agents-api): Add retry policies to temporal workflows/activities #551

Merged
merged 12 commits into from
Oct 5, 2024

Conversation

HamadaSalhab
Copy link
Contributor

@HamadaSalhab HamadaSalhab commented Oct 2, 2024

Important

Introduces DEFAULT_RETRY_POLICY for consistent retry behavior in Temporal workflows and activities, updating workflows, activities, and tests accordingly.

  • Retry Policy:
    • Introduces DEFAULT_RETRY_POLICY in retry_policies.py with specific retry configurations.
    • Applies DEFAULT_RETRY_POLICY to run_task_execution_workflow() in temporal.py and run_embed_docs_task() in create_doc.py.
  • Workflows:
    • Adds retry_policy=DEFAULT_RETRY_POLICY to DemoWorkflow, EmbedDocsWorkflow, MemRatingWorkflow, SummarizationWorkflow, TruncationWorkflow.
    • Updates TaskExecutionWorkflow in task_execution/__init__.py to use DEFAULT_RETRY_POLICY for activities.
  • Activities:
    • Updates raise_complete_async() in raise_complete_async.py to use consistent string formatting.
    • Updates transition() in transition.py to use DEFAULT_RETRY_POLICY.
  • Tests:
    • Updates test_activities.py to use DEFAULT_RETRY_POLICY in workflow execution tests.

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

@HamadaSalhab HamadaSalhab requested a review from creatorrr October 2, 2024 18:55
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 578f473 in 13 seconds

More details
  • Looked at 353 lines of code in 14 files
  • Skipped 2 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/agents_api/common/retry_policies.py:8
  • Draft comment:
    Consider increasing maximum_attempts to a higher value to improve reliability in case of transient errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The retry policy is set to a maximum of 2 attempts, which might be too low for certain transient errors. Increasing the maximum attempts could improve reliability.

Workflow ID: wflow_oDpBmGrja17xGQsb


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! Incremental review on a1fb6aa in 8 seconds

More details
  • Looked at 41 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/agents_api/common/retry_policies.py:4
  • Draft comment:
    Ensure debug and testing are correctly set in all environments to avoid unintended retry behavior. Consider documenting their expected values and usage.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The retry policy is well-defined, but the use of debug and testing variables should be verified for correctness and potential side effects.

Workflow ID: wflow_paEEvU7QVBqzGmRU


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! Incremental review on ccd01db in 10 seconds

More details
  • Looked at 80 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/agents_api/common/retry_policies.py:13
  • Draft comment:
    Consider reviewing the list of non-retryable error types. Some exceptions like RuntimeError and ValueError might be too generic and could potentially mask issues that are transient and should be retried. Ensure that all included exceptions are truly non-retryable in the context of your application.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_dkRayuzjgCEX0nOV


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! Incremental review on 2d945d3 in 9 seconds

More details
  • Looked at 44 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/agents_api/workflows/task_execution/__init__.py:57
  • Draft comment:
    The import of DEFAULT_RETRY_POLICY was moved inside the workflow.unsafe.imports_passed_through() block, which is correct. Ensure all necessary imports are within this block for Temporal workflows.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The import of DEFAULT_RETRY_POLICY was moved inside the workflow.unsafe.imports_passed_through() block, which is a good practice for Temporal workflows to ensure all necessary imports are available when the workflow is executed. However, the import statement was removed from the top of the file, which is unnecessary since it is now correctly placed inside the block.

Workflow ID: wflow_cOyp7X4zLWDofvyL


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

@creatorrr
Copy link
Contributor

@HamadaSalhab can you confirm that this works? Because I see string names of the error classes both direct and namespaced ("WorkflowBlahError", "temporal.something.AnotherError"). Also- add beartype exceptions to the list as well please

@creatorrr creatorrr merged commit 2e0108e into dev Oct 5, 2024
3 checks passed
@creatorrr creatorrr deleted the f/retry-policy-improvement branch October 5, 2024 01:39
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.

2 participants