Skip to content

Added, renamed and consolidated methods of BaseAgent #107

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
Apr 21, 2025

Conversation

koichikawamura
Copy link

Hi @KennyVaneetvelde,

In accordance to the discussion at #100, I reformed BaseAgent class and now it has the following 4 agent running methods in this PR:
(I recreated PR with my private account since I no longer use @tf-koichi account.)

  • run
    synchronous batch run
  • run_stream
    synchronous streaming run
  • run_async
    asynchronous batch run
  • run_async_stream
    asynchronous streaming run

I sought a way to incorporate a transition mechanism to run_async for backward compatibility, but couldn't figure out a good one.

Also I made changes to type hints so that they make use of TypeVar.

Koichi

@koichikawamura
Copy link
Author

I don't know why Black Check is giving an error. It ran OK locally.

@koichikawamura
Copy link
Author

@KennyVaneetvelde

I noticed the CI/CD workflow is failing on the black formatting check with the following error:

The virtual environment found in /home/runner/work/atomic-agents/atomic-agents/.venv seems to be broken.
Recreating virtualenv atomic-agents in /home/runner/work/atomic-agents/atomic-agents/.venv
Command not found: black
Error: Process completed with exit code 1.

This appears to be an issue with the CI environment rather than the code itself. The workflow is trying to use black before it's properly installed in the recreated virtualenv.

The tests pass locally, and running poetry run black atomic-agents atomic-assembler atomic-examples atomic-forge on my machine causes no issues. The project's pyproject.toml correctly specifies black as a dev dependency (black = ">=24.8.0,<25.0.0").

To fix this in the CI workflow, I'd suggest updating it to:

  1. Completely remove any existing virtual environment before creating a new one
  2. Explicitly install dev dependencies with poetry install --with dev
  3. Then run the black formatting check

Something like this might help:

- name: Install dependencies
  run: |
    python -m pip install --upgrade pip
    python -m pip install poetry
    rm -rf .venv || true
    poetry install --with dev

Let me know if there's anything else I need to address on my end for this PR. Thanks!

@KennyVaneetvelde
Copy link
Member

KennyVaneetvelde commented Apr 18, 2025

@koichikawamura Heya, I'm aiming to merge the branch over the weekend

Been thinking about it as well, I don't think we can provide backward compatibility here

Semantic Versioning dictates that any breaking change must trigger a major version increase - no matter how small

Since I am finishing up on MCP support over the weekend as well, I suppose calling it Atomic Agents 2.0 would not be too bad, at least that way we don't break anything that anyone might be running in production

Since it is a "major release", though, I'll prepare a release branch later for this so that we can properly test and find any bugs or issues

I'll have a look at the Black thing, thanks for your suggestion!

Anyways, all that being said, I had an idea for a slight improvement to this implementation, @koichikawamura (sorry for it not being in the original issue, I only just thought of it) but I think it would be better if we would have a method run and run_async but have a boolean param stream to enable or disable streaming

For code cleanliness, we could still have 4 private functions __run, __run_async_stream, ... if that helps (your call) depending on how bloated the public functions otherwise get, but I think having the param would be a slightly nicer developer experience

EDIT: I fixed the issue with Black on the main branch and merged it onto this PR, thanks again!

@koichikawamura
Copy link
Author

Hi @KennyVaneetvelde ,

Actually, I've been thinking about an idea for some time. It may sound a little bold but now is a good timing to talk about it if you are thinking about a major version up.
The idea is to use type parameters, which is a cool new feature introduced in Python3.12, to specify input and output schemas of agents and tools instead of specifying them as ordinary parameters. How do you think? If it sounds interesting to you, I can partially implement it as a POC and make a PR within not too long time.

BTW, I imagine. that many people are migrating to Python3.12 because it is the standard version in Ubuntu 24.04LTS.

@KennyVaneetvelde
Copy link
Member

@koichikawamura Yeah I'd be curious to see what that would look like!

What did you think about my suggestion to make streaming a parameter of run and run_async? I think it makes more sense that way

Another thing I wanted to look at for a V2.0 was proper modules, currently imports are a bit awkward and long some times, directly reflecting the internal folder structure - this was handy during initial development but it makes usage a bit awkward and I noticed in some cases vscode does not enjoy it either & has trouble with auto-imports

I don't have much time today but I'll make an issue for it so we can keep track and I'll make a v2.0 label or something

@koichikawamura
Copy link
Author

koichikawamura commented Apr 18, 2025

Hi @KennyVaneetvelde,

I made the following PR to demonstrate the problem which arises from batch/streaming switch parameter:
#108

A method which returns AsyncGenerator fails if it has a return response line, and it doesn't matter whether the line is actually executed or not. So switching between batch and streaming with a boolean parameter doesn't work for asynchronous processing.

It doesn't cause any problem for synchronous processing. In fact, run_stream method can be used exactly the same way as run (probably with a slight additional overhead) because it has return full_response_content at the end, which I didn't really document. Anyway, having a streaming-switch parameter for a synchronous method and two segregated asynchronous methods is, in my opinion, confusing and is against my sense of beauty.

As the bottom line, I would stick to the four-method (run, run_stream, run_async, run_async_stream) implementation.

@KennyVaneetvelde
Copy link
Member

Decided to make MCP into a v1.1.0 release since there were no breaking changes, but I went ahead and made a v2.0 milestone, this PR looks good so I'll also merge it into the v2.0 branch

@KennyVaneetvelde KennyVaneetvelde changed the base branch from main to v2.0 April 21, 2025 12:14
@KennyVaneetvelde KennyVaneetvelde merged commit b458dc9 into BrainBlend-AI:v2.0 Apr 21, 2025
1 check 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