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): Replace Retry Policies with Temporal Interceptors & Add more non-retryable errors #612

Merged
merged 7 commits into from
Oct 8, 2024

Conversation

HamadaSalhab
Copy link
Contributor

@HamadaSalhab HamadaSalhab commented Oct 8, 2024

Important

Replaces retry policies with Temporal interceptors to handle non-retryable errors and updates worker to use these interceptors.

  • Interceptors:
    • Add CustomActivityInterceptor and CustomWorkflowInterceptor in interceptors.py to handle non-retryable errors by raising ApplicationError.
    • CustomInterceptor combines both activity and workflow interceptors.
  • Non-Retryable Errors:
    • Define NON_RETRYABLE_ERROR_TYPES and is_non_retryable_error() in exceptions/tasks.py to identify non-retryable errors.
  • Retry Policy:
    • Set DEFAULT_RETRY_POLICY to None in retry_policies.py due to conflict with interceptors.
  • Worker:
    • Update create_worker() in worker.py to use CustomInterceptor.

This description was created by Ellipsis for 7a50b72. 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 7a50b72 in 16 seconds

More details
  • Looked at 314 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/agents_api/common/retry_policies.py:1
  • Draft comment:
    The import statements and DEFAULT_RETRY_POLICY are commented out, likely due to the FIXME note. Ensure this change doesn't affect other parts of the codebase relying on DEFAULT_RETRY_POLICY.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statements for timedelta and RetryPolicy are commented out, and the DEFAULT_RETRY_POLICY is set to None. This is likely intentional due to the FIXME comment indicating an issue with retry policies and interceptors. However, it's important to ensure that this change doesn't affect other parts of the codebase that might rely on DEFAULT_RETRY_POLICY.

Workflow ID: wflow_4zA7YJ6NiK4QLdxt


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

@creatorrr
Copy link
Contributor

@HamadaSalhab - actually how will we do maximum attempts etc without retry policies?

@HamadaSalhab
Copy link
Contributor Author

@HamadaSalhab - actually how will we do maximum attempts etc without retry policies?

It seems like the interceptors & retry policies do not work well when both are set. I left a FIXME regarding that.

@HamadaSalhab HamadaSalhab merged commit d2759b3 into dev Oct 8, 2024
8 of 10 checks passed
@HamadaSalhab HamadaSalhab deleted the f/tasks-error-handling branch October 8, 2024 18:08
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