Skip to content

Conversation

@prassanna-ravishankar
Copy link
Member

CI was broken

@prassanna-ravishankar prassanna-ravishankar marked this pull request as ready for review September 25, 2025 09:23
@prassanna-ravishankar prassanna-ravishankar changed the title Fix DEFAULT_MAX_RETRIES value from 0 to 2 [Chore] Fix DEFAULT_MAX_RETRIES value from 0 to 2 Sep 25, 2025
@smoreinis
Copy link
Contributor

Can we just fix the test expectation here? I'm not sure if there was a reason why we wanted to actually reduce the default number of max retries to 0 cc @jasonyang101

@prassanna-ravishankar
Copy link
Member Author

Can we just fix the test expectation here? I'm not sure if there was a reason why we wanted to actually reduce the default number of max retries to 0 cc @jasonyang101

AFAIK the core issue is the default. I asked claude to do a bisect + analysis

  1. Retry behavior should be > 0 by default: A default of 0 retries means no resilience to transient network issues, which is poor UX for an SDK.
  2. Test expectations were more logical: The tests expecting 2 retries suggest this was the intended design.
  3. Industry standards: Most HTTP clients default to 2-3 retries for better reliability.
  4. The bisect revealed the issue: The assert self.client.max_retries == 2 assertions have been in the codebase for a long time, indicating this was always the intended behavior.

Copy link
Contributor

@smoreinis smoreinis left a comment

Choose a reason for hiding this comment

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

ok, I checked and it doesn't sound like anything's relying on this change having been made in https://github.com/scaleapi/agentex-python/pull/108/files so I'm ok with having it back at 2. thanks!

@prassanna-ravishankar prassanna-ravishankar merged commit a208510 into main Sep 25, 2025
9 of 10 checks passed
@prassanna-ravishankar prassanna-ravishankar deleted the fix/failing-test branch September 25, 2025 19:56
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.

3 participants