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

allow :rate-limiter/rate to be floats, to have floating points rates (0.5 => 1 req every 2 seconds) #61

Merged
merged 4 commits into from
Feb 16, 2024

Conversation

realgenekim
Copy link
Contributor

Thank you for such a wonderful library — I am using this library so often when I hit external API endpoints.

Recently, I started using it to query the OpenAI API, which has token limits that often push the rate limit below one request per second. (I use thread pools via claypoole library to fire off parallel requests)

I found that it was super-easy to specify rate limits of less then 1 req/sec, merely by specifying floats less then 1.0. All I had to do was relax the spec to take number? instead of int?.

PS: I haven't done any formalized or methodical testing, beyond just observation, but it works well enough for my purposes. (Before opening up this PR, I did some more measurements. A rate limit of 0.5 will fire 10 calls in 25 seconds. 0.33 will take 36 seconds. Good enough for me!)

@realgenekim
Copy link
Contributor Author

Oops. Forgot to modify documentation in README.md. (I hope I'm doing this correctly — I haven't done this very often!)

@sunng87
Copy link
Owner

sunng87 commented Jul 6, 2023

Thank you @realgenekim ! I didn't realize we can have such little rate :). Could you please add a test case in tests/diehar/rate_limiter_test.clj by replacing the current test with a float point rate so that we can verify this behaviour

@kevinmershon
Copy link

@realgenekim Are you going to finish this PR? This is a worthwhile addition to the lib

@realgenekim
Copy link
Contributor Author

Hi, @kevinmershon and @sunng87 — sorry for the delay on this. I updated the PR that includes a test. (My apologies in advance if I did something wrong. Just let me know, and I will fix.)

@sunng87
Copy link
Owner

sunng87 commented Feb 16, 2024

LGTM. Thank you for following up.

@sunng87 sunng87 merged commit 379424f into sunng87:master Feb 16, 2024
1 of 2 checks passed
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