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

Async Agent Model #831

Closed
wants to merge 14 commits into from
Closed

Conversation

hkjeon13
Copy link

This library is very easy to integrate into existing services, making it an attractive option for production use. However, there is one issue: the library does not support asynchronous operations, which makes it challenging to implement in a real-time service environment. To address this, I modified parts of the code to enable asynchronous calls in the existing modules.

The modifications were made in the following files:

  • agents.py: AsyncMultiStepAgent, AsyncToolCallingAgent, AsyncCodeAgent
  • models.py: AzureOpenAIServerModel, AsyncAzureOpenAIServerModel
  • tools.py: AsyncTool

I would appreciate it if you could review these changes and let me know if they are worth implementing.

@hkjeon13 hkjeon13 force-pushed the feature/async-agent branch from 6d277b7 to 30ed35b Compare February 28, 2025 11:11
@rht
Copy link

rht commented Feb 28, 2025

I'm just a passerby, but my take:

  • There is a constraint: the lib would hardly be called "smolagents" if agents.py goes much longer than 1k lines of code. Maybe there could be another library that is built on top of smolagents that will have comprehensive features?
  • Is there a way to avoid code duplication by defining parent classes with common patterns?

@hkjeon13
Copy link
Author

I'm just a passerby, but my take:

  • There is a constraint: the lib would hardly be called "smolagents" if agents.py goes much longer than 1k lines of code. Maybe there could be another library that is built on top of smolagents that will have comprehensive features?
  • Is there a way to avoid code duplication by defining parent classes with common patterns?

I don't fully subscribe to the notion that 'smolagent' must maintain a small codebase, but I do agree that there is significant duplication in the current code that needs to be reduced. This version includes some duplication solely to implement asynchronous processing, but with additional effort, much of it could be minimized. However, this would involve a substantial overhaul of the internal architecture, and it seems more appropriate to address that after the asynchronous features have been integrated and most functionalities have stabilized (especially considering the noticeable differences between main and v1.9.2, which indicate that the library still requires considerable stabilization).

@maeste
Copy link
Contributor

maeste commented Mar 1, 2025

In general I agree with @rht there is space to optimize the code with parent class and in general a refactoring. Doing a refactoring during adding a new functionality can be weird, but also pretending to have a big refactory in one pass after adding all the functionality seems hard. @hkjeon13 What about adding a commit with the refactoring for having parent classes for synch and asynch in the same PR? I mean, keep the current commit (which LGTM), making easier to read the functionality added, but propose also with a separate commit the needed refactoring. It would help avoiding to add other code to refactor on the pile and a "smol" step forward on optimizing the code with inheritance.

That said, I think async addition would be a great plus.

Just my 2C to this discussion, from another passerby

@hkjeon13
Copy link
Author

hkjeon13 commented Mar 3, 2025

@maeste Thank you for your feedback. I've added a refactored version in #849.
But please note that there are limitations to the refactoring since we cannot arbitrarily change the existing functions.

@hkjeon13 hkjeon13 closed this Mar 3, 2025
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