Removed inheritance of TaskSpecifier, TaskPlanner and CriticAgent from ChatAgent

[Obs01ete]

Description

Removed inheritance of TaskSpecifier, TaskPlanner and CriticAgent from ChatAgent. Now we have 0 mypy errors in camel and test.

Motivation and Context

TaskSpecifier, TaskPlanner and CriticAgent do not follow the signature of step() of ChatAgent. They must be dis-inherited from ChatAgent and a ChatAgent instance used as a member.

Issue #163

This is a list of rules of overriding for python:
https://rules.sonarsource.com/python/RSPEC-2638/

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

• I have read the CONTRIBUTION guide. (required)
• My change requires a change to the documentation.
• I have updated the tests accordingly. (required for a bug fix or a new feature)
• I have updated the documentation accordingly.

[lightaime]

TBH, I do not know if introducing nested agents is a good idea or not. It makes the design very complicated. Renaming the step functions also makes it hard to scale if we introduce more agents in the future. The current inheritance is not ideal but we can find other ways to fix it. I would prefer not to introduce nested agents and keep unified interfaces.

This is an important change. It would be great to hear everyone's opinions @hammoudhasan @HaniItani @oserikov @zechengz @camel-ai/camel-intern-team.

[Obs01ete]

The current inheritance is non-existant. If you "inherit" just a name of a method but not its signature it is not inheritance. What do you mean by "nested" agents?

[Obs01ete]

I believe, you want to organize something like PyTorch dynamic graph. But it does not make much sense since the only purpose of pytorch modules is to keep track of the compute graph for backprop. We do not have backprop then we dont need Modules. Further, notice that PyTorch does dynamic check of matching arguments and can crash at any moment. This is fine for Pytorch since neural networks are fast (less than a second). With chat agents it may take minutes. You dont want to have a chat of 50 messages to have it crash when the last agent of summarization gets the wrong number of parameters. We want this statically checked by mypy.

[Obs01ete]

BTW if you wanted something like PyTorch modules, you could name the main function 'forward' to refer PyTorch's concept.

[zechengz]

IMO it depends on the functionality of TaskSpecifier, TaskPlanner and CriticAgent (compared to ChatAgent).
The benefit of current inheritance is that TaskSpecifier, TaskPlanner and CriticAgent functionality is quite similar to that of ChatAgent. Using inheritance helps to make code simpler and cleaner.
The benefit of removing inheritance could be that future TaskSpecifier, TaskPlanner and CriticAgent differ relative a lot from ChatAgent. In this case, making TaskSpecifier, TaskPlanner and CriticAgent in parallel with ChatAgent makes sense. (Though I don't know what is our current plan for these agents in the future)
In near term IMO we might at least keep step function (which makes function calling same way as ChatAgent and this makes our development / function calling simpler) If in the future the functionality differs a lot from ChatAgent we can then rename the function with another name.
WDYT @Obs01ete @lightaime ?

[Obs01ete]

I realize that playing with inheritance may make you look cool in the eyes of peers. But it is not cool if you (1) have mypy errors on your code, (2) do not use polymorphism which is the purpose of having inheritance, (3) do not follow the Gang of Four's "prefer composition over inheritance" principle. A good software engineering practice is to use inheritance purely for interface specification, having max one level of inheritance.

[Obs01ete]

@lightaime I got you point about the nested agents. My opinion is that this is the pattern that is the most natural: makes the job done with composition only. Why is it complicated? You don't have to flatten-out all agents. What is the use of a unified interface? You could do some dynamic routing of messages between agents but if the output signature is is incompatible with the input signature, the code will crash and that's it.

[oserikov]

Thank you, I'm good with this PR! @lightaime @Obs01ete could we meet and discuss the nested/non-nested agent thing? I believe this relates to the general positioning of the codebase

[oserikov]

Suggested change

	def specify_prompt(
	def expand_prompt(

I'd propose expand as a basic self-descriptive word for what happens here, same could work for the class name probably

[Obs01ete]

@lightaime Speaking of nested agents, in the EmbodiedAgent we already have a list of ToolAgents saved in a field.

camel/camel/agents/embodied_agent.py

Line 61 in abff2ea

self.action_space = action_space or default_action_space

This looks like nesting to me.

[Obs01ete]

Fixed in #206

[lightaime]

Closed since it is not an active PR.