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.

Sign up for GitHub

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

Jump to bottom

Removed all derived classes of BaseMessage and its role field #177

Merged
merged 7 commits into from
Jun 28, 2023
Merged

Removed all derived classes of BaseMessage and its role field #177

merged 7 commits into from
Jun 28, 2023

Conversation

Obs01ete
Copy link
Collaborator

@Obs01ete Obs01ete commented Jun 20, 2023 •
edited
Loading

Description

  • The following classes have no pracrical use and must be scrapped:
ChatMessage, AssistantChatMessage, UserChatMessage,
SystemMessage, AssistantSystemMessage, UserSystemMessage
  • role field of BaseMessage must go. Instead it should be managed by ChatAgent internally.

This is manifestation of KISS: Keep it short, simple. The tests pass meaning all that was removed was not needed in the first place.

Motivation and Context

This started from #163 and somewhat exploded.

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring

Checklist

  • 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.

@Obs01ete Obs01ete self-assigned this Jun 20, 2023
@Obs01ete Obs01ete added Agent Related to camel agents Refactor API Modifies the API labels Jun 20, 2023
@Obs01ete Obs01ete marked this pull request as ready for review June 20, 2023 15:57
@Obs01ete Obs01ete changed the title Removed role field from BaseMessage and all its derived classes Removed all derived classes of BaseMessage and its role field Jun 20, 2023
lightaime
lightaime reviewed Jun 24, 2023
View reviewed changes
Copy link
Member

@lightaime lightaime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

examples/code/role_playing_multiprocess.py Outdated Show resolved Hide resolved
examples/code/role_playing_multiprocess.py Outdated Show resolved Hide resolved
camel/agents/chat_agent.py Outdated Show resolved Hide resolved
camel/agents/chat_agent.py Outdated Show resolved Hide resolved
camel/messages/base.py Show resolved Hide resolved
camel/messages/base.py Outdated Show resolved Hide resolved
camel/societies/role_playing.py Outdated Show resolved Hide resolved
camel/societies/role_playing.py Outdated Show resolved Hide resolved
oserikov
oserikov reviewed Jun 26, 2023
View reviewed changes
Copy link

@oserikov oserikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR, @Obs01ete ! I totally agree with the replacement of Asst and Usr classes with make_asst and make_usr methods.

On the other hand one thing confuses me. Why are _critic_ renamings introduced in this PR (or update_messages altered)? they're not related to the base message logic, but globally to the camel pipeline.

NB I haven't run tests locally.

camel/messages/base.py Show resolved Hide resolved
camel/messages/base.py Show resolved Hide resolved
camel/messages/base.py Outdated Show resolved Hide resolved
camel/societies/role_playing.py Outdated Show resolved Hide resolved
camel/societies/role_playing.py Outdated Show resolved Hide resolved
test/agents/test_chat_agent.py Show resolved Hide resolved
@Obs01ete
Copy link
Collaborator Author

Great PR, @Obs01ete ! I totally agree with the replacement of Asst and Usr classes with make_asst and make_usr methods.

On the other hand one thing confuses me. Why are _critic_ renamings introduced in this PR (or update_messages altered)? they're not related to the base message logic, but globally to the camel pipeline.

NB I haven't run tests locally.

Maybe this rename is out of the scope of this PR. It was a bit natural to these renames. I do not want to make too thin salami slicing of changes at this stage.

oserikov
oserikov approved these changes Jun 27, 2023
View reviewed changes
Copy link

@oserikov oserikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. @Obs01ete could you still answer the user/assistant comment? Just for me to better understand the thing.

camel/agents/chat_agent.py Outdated Show resolved Hide resolved
examples/code/role_playing_multiprocess.py
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the submit/reduce naming !

lightaime
lightaime reviewed Jun 27, 2023
View reviewed changes
Copy link
Member

@lightaime lightaime left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Left some comments

camel/agents/chat_agent.py Outdated Show resolved Hide resolved
camel/societies/role_playing.py Show resolved Hide resolved
camel/agents/chat_agent.py Outdated Show resolved Hide resolved
lightaime
lightaime approved these changes Jun 28, 2023
View reviewed changes
Copy link
Member

@lightaime lightaime left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!!

@Obs01ete Obs01ete merged commit 16c1ac3 into master Jun 28, 2023
@Obs01ete Obs01ete deleted the remove_role_field branch June 28, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lightaime lightaime lightaime approved these changes

@oserikov oserikov Awaiting requested review from oserikov

Assignees

@Obs01ete Obs01ete

Labels
Agent Related to camel agents API Modifies the API Refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Obs01ete @lightaime @oserikov