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

Allow None for sender field in CoversableAgent.generate_reply #1725

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

RohitRathore1
Copy link
Contributor

@RohitRathore1 RohitRathore1 commented Feb 19, 2024

Why are these changes needed?

Currently, if we do:

agent = ConversableAgent("name", llm_config=False, human_input_mode="ALWAYS")
reply = agent.generate_reply(messages=[{"role": "user", "content": "hello"}])

We get

AttributeError: 'NoneType' object has no attribute 'name'

After fixing this bug it addresses potential issue with sender being None and using sender_name correctly throughout the method. This ensures that our method handles different scenarios robustly, including when sender might not be provided, and makes our method more resilient to various input conditions.

autogen@17295b29b93a:~/autogen$ python3
Python 3.11.8 (main, Feb 13 2024, 10:08:31) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from autogen.agentchat.conversable_agent import ConversableAgent
>>> agent = ConversableAgent("name", llm_config=False, human_input_mode="ALWAYS")
>>> reply = agent.generate_reply(messages=[{"role": "user", "content": "hello"}])
Provide feedback to the sender. Press enter to skip and use auto-reply, or type 'exit' to end the conversation: hello
>>> print(reply)
{'role': 'user', 'content': 'hello'}
>>> reply = agent.generate_reply(messages=[{"role": "user", "content": "hello"}])
Provide feedback to the sender. Press enter to skip and use auto-reply, or type 'exit' to end the conversation: exit
>>> print(reply)
None

Related issue number

Closes #1708

Checks

@RohitRathore1
Copy link
Contributor Author

@microsoft-github-policy-service agree

@sonichi
Copy link
Contributor

sonichi commented Feb 19, 2024

Please fix the code formatting error. You can use pre-commit to help.

@RohitRathore1
Copy link
Contributor Author

The fix looks good! Could you add a test about this? Thank you!

Thanks for the review! It seems test cases are already present in our test suite, which are verifying the scenario where messages and sender are None for both synchronous and asynchronous versions of generate_reply.

def test_generate_reply_raises_on_messages_and_sender_none(conversable_agent):
with pytest.raises(AssertionError):
conversable_agent.generate_reply(messages=None, sender=None)
@pytest.mark.asyncio
async def test_a_generate_reply_raises_on_messages_and_sender_none(conversable_agent):
with pytest.raises(AssertionError):
await conversable_agent.a_generate_reply(messages=None, sender=None)

Thanks! These tests are indeed relevant but I think we should add an additional test in which the message is provided and the sender is None.

Thanks, done!

@ekzhu
Copy link
Collaborator

ekzhu commented Feb 21, 2024

@RohitRathore1 What's your discord name? We can add you to an appropriate role.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 21, 2024
@qingyun-wu qingyun-wu added this pull request to the merge queue Feb 21, 2024
Merged via the queue into microsoft:main with commit 3c2c251 Feb 21, 2024
55 of 57 checks passed
@RohitRathore1
Copy link
Contributor Author

RohitRathore1 commented Feb 22, 2024

@RohitRathore1 What's your discord name? We can add you to an appropriate role.

Hi @ekzhu sure, my username is RohitRathore1. Let me know if you encounter any issues.

whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
…osoft#1725)

* Allow None for sender field

* Apply formatting fixes

* Add test for ConversableAgent generate_reply with message is provided and sender is None

---------

Co-authored-by: Chi Wang <wang.chi@microsoft.com>
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.

[Feature Request]: allow None for sender field in CoversableAgent.generate_reply
5 participants