GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [DEV-12952] Add Configurable Retry Behavior by mawelborn · Pull Request #350 · IndicoDataSolutions/indico-client-python

Conversation

@mawelborn

@mawelborn mawelborn commented Jan 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Summary

This PR implements configurable, best-practices retry behavior for both IndicoClient and AsyncIndicoClient with consistent socket connect and read timeouts. This replaces the existing retry behavior that's only present on the async client and isn't configurable. IndicoConfig has new options to control retry behavior, with reasonable defaults:

IndicoConfig(
    retry_count: int = 4,  # Retry API calls this many times.
    retry_wait: float = 1,  # Wait this many seconds after the first error before retrying.
    retry_backoff: float = 4,  # Multiply the wait time by this amount for each additional error.
    retry_jitter: float = 1,  # Add a random amount of time (up to this percent as a decimal) to the wait time to prevent simultaneous retries.
)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • Jira story/task is put into PR title
  • Code has been self-reviewed
  • Hard-to-understand areas have been commented
  • Documentation has been updated
  • My changes generate no new warnings
  • Unit tests have been added for base functionality and known edge cases
  • Any dependent changes have been merged and published in downstream modules

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mawelborn mawelborn added the enhancement New feature or request label Jan 29, 2025
@mawelborn mawelborn self-assigned this Jan 29, 2025
@mawelborn
mawelborn requested a review from a team as a code owner January 29, 2025 16:38
@mawelborn
mawelborn requested review from Sung96kim, goatrocks, jacobmanderson, nicholas-lockhart and sihrc January 29, 2025 16:38
@mawelborn
mawelborn force-pushed the mawelborn/configurable-retry branch 6 times, most recently from f68f18f to 3a7f846 Compare October 16, 2025 21:03
mawelborn-indico added 2 commits October 16, 2025 16:17
@mawelborn-indico
Add configurable retry behavior with reasonable defaults for count, w…
32b1a8c
…ait, backoff, and jitter

This makes retry behavior consistent between sync and async clients, and
in its default configuration will get users past network interruptions.
@mawelborn-indico
Make socket connect and read timeouts consistent between sync and asy…
1f9ebc1
…nc clients
@mawelborn
mawelborn force-pushed the mawelborn/configurable-retry branch from 3a7f846 to 1f9ebc1 Compare October 16, 2025 21:18
@mawelborn

mawelborn commented Oct 16, 2025

Copy link
Copy Markdown
Author

Hi @IndicoDataSolutions/pr-be-indicodata-ai folks! I rebased this PR onto main and have all the checks passing. Would appreciate a review when you have a chance. This functionality will be super useful for ProServe and customers.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thearchitector
thearchitector reviewed Oct 16, 2025
View reviewed changes

@thearchitector thearchitector left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

if this is an urgent thing, this can probably wait, but on the platform side we've been converging on this library, which might be worth considering here instead of shipping our own? it has fancy timing, logging, and backoff stuff we could probably expose if we wanted for better script integrations

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread indico/config/config.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mawelborn-indico
Fix environment variable casing for retry arguments
ea96ef6
@mawelborn

mawelborn commented Nov 14, 2025

Copy link
Copy Markdown
Author

Hello again! Sorry, just now getting back to this.

I don't have strong opinions regarding the retry implementation--whether we ship our own or use a library like Tenacity. I used the retry implementation from the toolkit since it's a known quantity, and shipping our own was already being done for AsyncIndicoClient.

Do I need to swap out the implementation or make any other changes? Or is the current implementation good enough for now?

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thearchitector
thearchitector reviewed Nov 17, 2025
View reviewed changes
Comment thread indico/http/client.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

mawelborn-indico added 2 commits November 18, 2025 07:59
@mawelborn-indico
Simplify _handle_files's verify conditional in _make_request
0fbfc54
@mawelborn-indico
Replace method assignment with dynamic wrapper method
c58b697
thearchitector
thearchitector reviewed Nov 18, 2025
View reviewed changes
Comment thread indico/http/client.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mawelborn-indico
Make socket connect and read timeouts configurable
d6ec2b0
thearchitector
thearchitector reviewed Nov 18, 2025
View reviewed changes
Comment thread indico/http/retry.py Outdated
"""
def retry_decorator(
decorated: "Callable[ArgumentsType, InnerReturnType]",
) -> "Callable[ArgumentsType, Awaitable[InnerReturnType]] | Callable[ArgumentsType, InnerReturnType]": # noqa: E501

@thearchitector thearchitector Nov 18, 2025

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

we shouldn't need both inner and outer return type here i don't think? both the wrapper and the fn should be typed to return the same thing (since they do). that would probably alleviate the any typing problem too

also | isn't valid for <3.10. has to be Union[]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mawelborn mawelborn Nov 21, 2025

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah good catch! Those two return types are the same in this version.

It's valid on 3.9 when string escaped, like the other types that are only imported when type checking. I've run it and type checked it with mypy --strict on 3.9 without issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thearchitector reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@mawelborn-indico
Deduplicate retry return type TypeVar
f45cdda
thearchitector
thearchitector approved these changes Dec 5, 2025
View reviewed changes
@mawelborn

mawelborn commented Dec 8, 2025

Copy link
Copy Markdown
Author

@IndicoDataSolutions/pr-be-indicodata-ai it looks like one of you will have to approve and/or merge?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goatrocks goatrocks Awaiting requested review from goatrocks goatrocks was automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai
@sihrc sihrc Awaiting requested review from sihrc sihrc was automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai
@Sung96kim Sung96kim Awaiting requested review from Sung96kim Sung96kim was automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai
@jacobmanderson jacobmanderson Awaiting requested review from jacobmanderson jacobmanderson was automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai
@nicholas-lockhart nicholas-lockhart Awaiting requested review from nicholas-lockhart nicholas-lockhart was automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai
1 more reviewer
@thearchitector thearchitector thearchitector approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@mawelborn mawelborn

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@mawelborn @thearchitector @mawelborn-indico
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.