-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: Add support for async modules and async bootstrap #161
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
Conversation
feat: support async bootstrap few shot
feat: Add support for Async Module
| # return new_copy | ||
| class AsyncModule(Module): | ||
| async def __call__(self, *args, **kwargs): | ||
| return await self.forward(*args, **kwargs) |
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.
Will this actually work and become truly async? The call to the LLM is still completely synchronous/blocking.
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.
Thanks for engaging! It depends on how you define forward(). The goal is just to allow you to do async ops, which could be LM-related or not (e.g. async db reads), in the module. For example,
class RAG(dspy.AsyncModule):
def __init__(self):
super().__init__()
async def forward(self, question):
search_entity_batches = await asyncio.gather(*[search_fn_1, search_fn_2, ...])
# or could be async ranking, async ensemble answer generation, etc.
....
You're right in that it doesn't solve async for the current Predict and Retrieve abstractions, which use a sync LLM completion api call. Those would require a larger refactor to make async, but I think this is a reasonable escape hatch in the meantime.
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.
This is very reasonable. I can merge a very small change like this for sure but right now it's affecting a lot of lines of code
|
Hey, thanks a lot for opening this PR! We appreciate it. I left some comments. Let me know your thoughts. |
feat: Add async lm
|
This is so cool, thanks for the new updates! @lawliet19189 @detaos is this something you'd be interested to test? |
|
@sjiang2019 There's still a lot of content that may be redundant tho, I left some comments on those earlier, do you have thoughts on that |
|
@okhat Thanks! I'm slowly working on introducing an async path for LM calls as well.
I can't seem to see your earlier comments for some reason (I only see the one about async), but will take a stab at reducing the redundancies. |
refactor: Remove redundant code
…c-lm-code refactor: redundant code in gpt3 and async gpt3
| from dspy.teleprompt.bootstrap import BootstrapFewShot | ||
|
|
||
|
|
||
| class AsyncBootstrapFewShot(BootstrapFewShot): |
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.
Hmm why would you define async bootstrap fewshot? Do you want to compile in parallel with other things basically?
|
|
||
| class BootstrapFewShot(Teleprompter): | ||
| def __init__(self, metric=None, teacher_settings={}, max_bootstrapped_demos=4, max_labeled_demos=16, max_rounds=1): | ||
| def __init__( |
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.
Did anything actually change in this file? Please don't rewrite the style of the file if there are no changes there.
| @@ -0,0 +1,125 @@ | |||
| import dsp | |||
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.
Do we need to duplicate the entire code of bootstrap fewshot to define this?
| # return new_copy | ||
| class AsyncModule(Module): | ||
| async def __call__(self, *args, **kwargs): | ||
| return await self.forward(*args, **kwargs) |
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.
This is very reasonable. I can merge a very small change like this for sure but right now it's affecting a lot of lines of code
|
@okhat is there some progress here? We need this feature and are thinking of redoing it ourselves and opening a new PR without changing the code that is not relevant. wdyt? |
|
@Vasilije1990 Feel free to go ahead and create a new PR for async support! (Don't think any progress has been made to this PR but feel free to push onto it or create a new one, whichever works best) |
|
Hey @arnavsinghvi11, is async support part of the development roadmap? I don't see any active or closed PRs about this. I would be happy to pitch in if there is interest. |
|
Any reasons why this PR is on hold? What's the plan moving forward (continue this work or make a new PR)? |
|
@okhat I wonder if there is a plan for DSPy to natively support AsyncOpenAI and AsyncAnthropic? I know we can use sync_to_async, but I guess it's not exactly same with the native async function. Thanks! |
|
@Vasilije1990 @vaibhavnayel @NumberChiffre @owen-deepskill See #1806 for |
|
@CyrusNuevoDia Awesome! Many thanks for the update! |
AsyncModulewhich inherits fromModuleAsyncBootstrapFewShotwhich inherits fromBootstrapFewShot