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

[FEATURE] Add retries to the internal httpx.Client used by the SDK #5386

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

gabrielmbmb
Copy link
Member

Description

This PR adds a new argument retries that can be used to specify the number of times that an HTTP request performed by the internal httpx.Client used by the SDK should be retried before raising an exception.

This is useful as sometimes while using the dataset.records.log you can receive a ConnectionError or 5xx from the server and if you retry a few seconds later everything is fine.

Type of change

  • Improvement

How Has This Been Tested

Checklist

  • I added relevant documentation
  • I followed the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • I confirm My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@gabrielmbmb gabrielmbmb force-pushed the feature/client-retries branch from 49484dc to b5acc70 Compare August 6, 2024 16:21
@gabrielmbmb gabrielmbmb marked this pull request as ready for review August 12, 2024 07:47
@gabrielmbmb gabrielmbmb self-assigned this Aug 12, 2024
@gabrielmbmb gabrielmbmb added the type: improvement Indicates updates or improvements on existing features label Aug 12, 2024
@davidberenstein1957 davidberenstein1957 added this to the v2.1.0 milestone Aug 12, 2024
Copy link
Member

@davidberenstein1957 davidberenstein1957 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielmbmb should we define upon which failures we retry. I think it only makes sense for the 500 series and perhaps on request timeout or anything. https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses
if payload is invalid or something, we don't want to retry.

self.api_url = self.api_url
self.api_key = self.api_key
self.timeout = self.timeout or 60
timeout: int = 60

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielmbmb I think we can add this to the constants.py file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielmbmb The refactor of timeout and retries looks good to me. It's more consistent and avoids background passing with **client_args.

@davidberenstein1957 A few PRs have used constants in context lately. I think we should tackle this in a dedicated PR because using a single constant file is not universally good.

@burtenshaw
Copy link
Contributor

@gabrielmbmb Implementation looks good. What do you think about expanding the testing for this? I thought of these two but open to your suggestions:

  • One unit for retries attributes in here argilla/tests/unit/api/http/test_http_client.py.
  • One using pytest_httpx mock and checking the number of retries. Documented here

@frascuchon frascuchon modified the milestones: v2.1.0, v2.2.0 Sep 3, 2024
@burtenshaw
Copy link
Contributor

@frascuchon @davidberenstein1957 What's your view of this PR? I've added a test and the remaining comments are:

@davidberenstein1957
Copy link
Member

@frascuchon @davidberenstein1957 What's your view of this PR? I've added a test and the remaining comments are:

I think there are several error that can occur and will never be fixed by retries, like authentication or wrong payload, so for me it doesn't make sense to retry during those cases.

W.r.t. variables vs shared constants, I don't have a strict opinion besides having everything else in constants.py already.

@burtenshaw burtenshaw merged commit f5ff647 into develop Sep 13, 2024
7 checks passed
@burtenshaw burtenshaw deleted the feature/client-retries branch September 13, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement Indicates updates or improvements on existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants