Skip to content

feat: Add option for custom retries#15

Merged
zvikagart merged 3 commits intocodeocean:mainfrom
jtyoung84:feat-12-retry-adapter
Oct 21, 2024
Merged

feat: Add option for custom retries#15
zvikagart merged 3 commits intocodeocean:mainfrom
jtyoung84:feat-12-retry-adapter

Conversation

@jtyoung84
Copy link
Contributor

Closes #12

  • Adds _optional_adapters field to CodeOcean data class. Default is empty tuple. Suppressed from class repr.
  • Mounts adapters in __post_init__

@zvikagart
Copy link
Contributor

@jtyoung84 Turns out you can only mount a single HTTPAdapter. See here.
I've suggested a change which allows passing in an optional retry object:

from urllib3.util import Retry

retries = Retry(
    total=3,
    backoff_factor=1,
    status_forcelist=[429, 500, 502, 503, 504],
)

client = CodeOcean(domain=..., token=..., retries=retries)

LMK what you think, and if OK I'll merge this.

@jtyoung84
Copy link
Contributor Author

@jtyoung84 Turns out you can only mount a single HTTPAdapter. See here. I've suggested a change which allows passing in an optional retry object:

from urllib3.util import Retry

retries = Retry(
    total=3,
    backoff_factor=1,
    status_forcelist=[429, 500, 502, 503, 504],
)

client = CodeOcean(domain=..., token=..., retries=retries)

LMK what you think, and if OK I'll merge this.

I tested it out and it works. Looks good to merge. Thanks!

@zvikagart zvikagart merged commit b903f85 into codeocean:main Oct 21, 2024
zvikagart pushed a commit that referenced this pull request Oct 21, 2024
@zvikagart zvikagart changed the title feat: adds option for custom adapters feat: Add option for custom retries Oct 21, 2024
@bjhardcastle
Copy link
Contributor

Sorry I didn't get a chance to look at this sooner.

This merge actually changes the default behavior from 0 retries to 3.

TCPKeepAliveAdapter(..., max_retries=None) gets passed up the MRO to requests.HTTPAdapter() and is handled here, calling Retry.from_int():
https://github.com/psf/requests/blob/7335bbf480adc8e6aa88feb2022797a549a00aa3/src/requests/adapters.py#L209-L212

Retry.from_int() handles None by defaulting to Retry.DEFAULT:
https://github.com/urllib3/urllib3/blob/59bbda7e1543d2e8b4b65c3d8075152d664c43ca/src/urllib3/util/retry.py#L278-L279

...and Retry.DEFAULT is Retry(3):
https://github.com/urllib3/urllib3/blob/59bbda7e1543d2e8b4b65c3d8075152d664c43ca/src/urllib3/util/retry.py#L533

And it looks like this is true in practice:
0.1.5

>>> aind_session.get_codeocean_client().session.adapters['https://codeocean.allenneuraldynamics.org'].max_retries
Retry(total=0, connect=None, read=False, redirect=None, status=None)

0.2.0

>>> aind_session.get_codeocean_client().session.adapters['https://codeocean.allenneuraldynamics.org'].max_retries
Retry(total=3, connect=None, read=None, redirect=None, status=None)

To get the previous behavior, the default should just be set to zero (tested and it works):

@dataclass
class CodeOcean:

    domain: str
    token: str
    retries: Optional[Retry | int] = 0

    ...

The type annotation should probably reflect that int is supported, regardless.

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.

Allow for retries in requests

3 participants