Skip to content

FEAT: SelectorGroupChat could using stream inner select_prompt #6286

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

SongChiYoung
Copy link
Contributor

@SongChiYoung SongChiYoung commented Apr 12, 2025

Why are these changes needed?

This PR updates SelectorGroupChat to support streaming mode for select_speaker.
It introduces a streaming argument — when set to True, select_speaker will use create_streaming() instead of create().

Additional context

Some models (e.g., QwQ) only work properly in streaming mode.
To support them, the prompt selection step in SelectorGroupChat must also run with streaming=True.

Related issue number

Closes #6145

Checks

@SongChiYoung SongChiYoung changed the title FEAT: select group chat could using stream FEAT: SelectorGroupChat could using stream inner select_prompt Apr 12, 2025
@ekzhu
Copy link
Collaborator

ekzhu commented Apr 13, 2025

Can we address this first? #6161. Otherwise the streaming option won't actually stream to run_stream.

if self._streaming:
message: CreateResult | str = ""
async for _message in self._model_client.create_stream(messages=select_speaker_messages):
if isinstance(_message, LLMStreamEndEvent):
Copy link
Collaborator

Choose a reason for hiding this comment

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

_message actualy has the type str or CreateResult.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You’re right, that was my mistake.
I misunderstood how create_stream() works and thought the log message was being leaked.

But actually, the last message is always a CreateResult, and the if block just skips over LLMStreamEndEvent, so it worked by accident.
Thanks for pointing it out!

@SongChiYoung
Copy link
Contributor Author

SongChiYoung commented Apr 13, 2025

@ekzhu

Can we address this first? #6161. Otherwise the streaming option won't actually stream to run_stream.

I’ll take a look at #6161.
That said, in my case, I confirmed that stream was working by adding some print statements inside the create_stream() function at Openai Client

And, just change

            response = await self._model_client.create(messages=select_speaker_messages)

to

            if self._streaming:
                message: CreateResult | str = ""
                async for _message in self._model_client.create_stream(messages=select_speaker_messages):
                    message = _message
                if isinstance(message, CreateResult):
                    response = message
                else:
                    raise ValueError("Model failed to select a speaker.")
            else:
                response = await self._model_client.create(messages=select_speaker_messages)

So I don't think there would be any problem solving that issue —
-> well, except maybe some merge conflict later.

BTW, what kind of help do you expect for that issue?
There’s no code or draft linked yet.

Do you want me to take the lead on it?

@ekzhu
Copy link
Collaborator

ekzhu commented Apr 15, 2025

Yes, please address #6161 and then this one.

@SongChiYoung
Copy link
Contributor Author

@ekzhu I resolved merge conflict.

Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 77.52%. Comparing base (bb792b0) to head (ce0ac44).

Files with missing lines Patch % Lines
...gentchat/teams/_group_chat/_selector_group_chat.py 53.84% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6286      +/-   ##
==========================================
- Coverage   77.55%   77.52%   -0.03%     
==========================================
  Files         202      202              
  Lines       14759    14770      +11     
==========================================
+ Hits        11446    11451       +5     
- Misses       3313     3319       +6     
Flag Coverage Δ
unittests 77.52% <53.84%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

if self._streaming:
message: CreateResult | str = ""
async for _message in self._model_client.create_stream(messages=select_speaker_messages):
message = _message
Copy link
Collaborator

Choose a reason for hiding this comment

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

See how emitting event is done in BaseGroupChatManager:

await self._output_message_queue.put(ModelClientStreamingChunkEvent(content=_message)) to emit the chunk.

Copy link
Contributor Author

@SongChiYoung SongChiYoung Apr 17, 2025

Choose a reason for hiding this comment

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

Howabout, call _log_speaker_selection after makes total response?

Now I know about why do not use it.

like that case (adding [ and ] for more looks easy. look likely BaseGroupChatManager output) looks weird

---------- user ----------
say hello world
---------- SelectorGroupChatManager ----------
[hello]
---------- hello ----------
Hello, World!
---------- SelectorGroupChatManager ----------
Model failed to select a speaker after 3, using the previous speaker.
[hello][hello][,][ world][!][hello][,][ world][!]
---------- hello ----------
Hello, World!

Real three output is that
[hello] [hello, world!] [hello, world!]

So, howabout call await self._output_message_queue.put(ModelClientStreamingChunkEvent(content=_message)) after, set response?

Copy link
Contributor Author

@SongChiYoung SongChiYoung Apr 17, 2025

Choose a reason for hiding this comment

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

And... without that code,
output is like it

---------- user ----------
say hello world
---------- SelectorGroupChatManager ----------
['hello']
---------- hello ----------
Hello, World!
Model failed to select a speaker after 3, using the previous speaker.
---------- SelectorGroupChatManager ----------
['hello']
---------- hello ----------
Hello, World!

so... with that line, default _log_speaker_selection is looks not works.
(however, actually that function is called, just do not shown)

Clearly I need to your help for understand that funciton.

Copy link
Collaborator

Choose a reason for hiding this comment

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

_log_speaker_selection shouldn't be mixed in here. Keep it separate in the base class.

In this PR we are addressing the SelectorGroupChat-specific issue.

After every sequence of ModelClientStreamingEvent, it should be followed with a separate message which has the full content of the streamed token. This can be a SelectorEvent, for example.

Copy link
Contributor Author

@SongChiYoung SongChiYoung Apr 19, 2025

Choose a reason for hiding this comment

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

------ Delete that comment ------
Cause, now understand of your direction.

@SongChiYoung SongChiYoung force-pushed the feature/model_client_streaming_from_the_selector_of_selectorgroupchat_#6145 branch from ce0ac44 to 29f2c0f Compare April 17, 2025 09:16
@ekzhu
Copy link
Collaborator

ekzhu commented Apr 18, 2025

@SongChiYoung generally, I think it is more important to emit the inner SelectorEvent activities rather than just streaming the acitivies.

@SongChiYoung
Copy link
Contributor Author

@ekzhu
Thanks for all of your help.
I think, all of issue is resolved and changed from your direction.
and, I was add test case for this change.

Please check this PR.

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.

Model client streaming from the selector of SelectorGroupChat
2 participants