Skip to content

Add support for optional max concurrency #643

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

Merged
merged 6 commits into from
Feb 28, 2024

Conversation

joy13975
Copy link
Contributor

@joy13975 joy13975 commented Feb 20, 2024

Added optional Semaphore-based concurrency control for #642
As for the default value for max_concurrency, I don't know the ratio of API users vs. local LLM users, so the proposed default is an opinionated value of 16

  • I think more people use OpenAI API for now vs. local LLMs, thus default is not -1 (no limit)
  • 16 seems to be reasonably fast and doesn't seem to hit throughput limit in my experience

Tests
Embedding for 1k documents finished in <2min and subsequent Testset generation for test_size=1000 proceeding without getting stuck:
image

another 30s passes:
image

@joy13975 joy13975 force-pushed the add_max_concurrency branch from f926e94 to a9dfa76 Compare February 20, 2024 19:11
@joy13975
Copy link
Contributor Author

joy13975 commented Feb 21, 2024

Note: currently have test_size=500 done without problem (at 1k docs).
test_size=1000 still takes over an hour but does properly proceed rather than getting stuck at any %.

@wangyiran33
Copy link

Thanks! try to run but encounter a problemAs of 3.10, the *loop* parameter was removed from Semaphore() since it is no longer necessary

@jjmachan
Copy link
Member

I love this PR - thank you so much @joy13975 ❤️ - really! thank you for sharing your fix.

I just have one commend though. So I was thinking about the RunConfig and wondering it would be a good place to add this one too. the goal of that abstraction is that anything a user wants to configure for running - they can configure it through that and it will be our responsibility to make sure it gets used in the right place.

so as a user would you prefer it to be in RunConfig is that an overkill?

That was the only question I had but otherwise, I'm merging this in!

@joy13975
Copy link
Contributor Author

joy13975 commented Feb 23, 2024

@jjmachan
Totally overlooked putting it in RunConfig, but I think that might be neater than this. I’ll sort that out and put up another PR.

@wangyiran33 thanks for the report and l’ll make it compatible with 3.10 as well in the next PR update.

@jjmachan
Copy link
Member

makes perfect sense @joy13975 - there seems to be a small type error - if you fix that I'll merge this in

I can fix it too if you want but it will take some time for me to get to this sadly but the PR looks really good so would be awesome to merge it as soon as we can right.

@jjmachan
Copy link
Member

jjmachan commented Feb 23, 2024

or not - because if we don't have support for 3.10 that is a deal breaker - thanks for reporting that @wangyiran33
we might have to refer to the stuff they did

would love to hear your thoughts @joy13975

@joy13975
Copy link
Contributor Author

@jjmachan yeah I’ll fix all the above points once I get time. Should be up this weekend.

@jjmachan
Copy link
Member

sounds good - thanks a lot again
lets get this bug fixed 🔥

Also renamed max_concurrency to max_workers to be consistent with
convention.
@joy13975 joy13975 force-pushed the add_max_concurrency branch 2 times, most recently from b34341a to fe3ed68 Compare February 23, 2024 12:36
@joy13975 joy13975 force-pushed the add_max_concurrency branch from fe3ed68 to 0cebcc1 Compare February 23, 2024 12:39
@joy13975
Copy link
Contributor Author

joy13975 commented Feb 23, 2024

@jjmachan Just pushed an update to address all the above. @wangyiran33 Can you please help test on Python 3.10+?
Won't be able to increase my OpenAI account usage limit for a week..

@wangyiran33
Copy link

@jjmachan Just pushed an update to address all the above. @wangyiran33 Can you please help test on Python 3.10+? Won't be able to increase my OpenAI account usage limit for a week..

of course! I have tested and it works for py3.10, really appreciate that. Love this PR! A big Thanks for you! @joy13975

@jjmachan
Copy link
Member

jjmachan commented Feb 28, 2024

awesome - finished my round of testing and it is a massive improvement ❤️

thanks again for this @joy13975 for helping us all and contributing this fix 🙂

going out in today's release

@jjmachan jjmachan merged commit 12ad190 into explodinggradients:main Feb 28, 2024
@joy13975 joy13975 deleted the add_max_concurrency branch February 28, 2024 14:16
joy13975 added a commit to joy13975/ragas that referenced this pull request Feb 28, 2024
**Added optional Semaphore-based concurrency control for explodinggradients#642** 
As for the default value for `max_concurrency`, I don't know the ratio
of API users vs. local LLM users, so the proposed default is an
opinionated value of `16`
* I *think* more people use OpenAI API for now vs. local LLMs, thus
default is not `-1` (no limit)
* `16` seems to be reasonably fast and doesn't seem to hit throughput
limit in my experience

**Tests**
Embedding for 1k documents finished in <2min and subsequent Testset
generation for `test_size=1000` proceeding without getting stuck:
<img width="693" alt="image"
src="https://github.com/explodinggradients/ragas/assets/6729737/d83fecc8-a815-43ee-a3b0-3395d7a9d244">

another 30s passes:
<img width="725" alt="image"
src="https://github.com/explodinggradients/ragas/assets/6729737/d4ab08ba-5a79-45f6-84b1-e563f107d682">

---------

Co-authored-by: Jithin James <[email protected]>
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