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

Use new event loop instead of the current loop for pipeline #1352

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

AllentDan
Copy link
Collaborator

For some jupyter notebook users, the loop can not be used directly.

@@ -476,8 +475,8 @@ async def gather():
])
outputs.put(None)

proc = Thread(
target=lambda: self.loop.run_until_complete(gather()))
proc = Thread(target=lambda: asyncio.new_event_loop().
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you use thread instead of coroutine here?
asyncio.get_event_loop().create_task would give create a new coroutine task parallelized with the current coroutine. and asyncio.Queue is awaitable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do you use thread instead of coroutine here? asyncio.get_event_loop().create_task would give create a new coroutine task parallelized with the current coroutine. and asyncio.Queue is awaitable.

Just want to yield items without async. Do you have demo codes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thread -> asyncio.get_event_loop().create_task
Queue -> asyncio.queue

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How can I return a generator for stream infer?

try:
out = outputs.get(timeout=0.001)
if out is None:
break
yield out

Copy link
Collaborator

Choose a reason for hiding this comment

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

 try: 
	     out = await outputs.get() 
	     if out is None: 
	         break 
	     yield out 

Copy link
Collaborator Author

@AllentDan AllentDan Apr 2, 2024

Choose a reason for hiding this comment

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

 try: 
	     out = await outputs.get() 
	     if out is None: 
	         break 
	     yield out 

but this is not an async function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

def __call_async():

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid the performance drop, I used the original Thread and Queue.

Copy link
Collaborator

@RunningLeon RunningLeon left a comment

Choose a reason for hiding this comment

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

LGTM

@lvhan028
Copy link
Collaborator

lvhan028 commented Apr 3, 2024

@AllentDan what's the performance of llama-7b now?

@AllentDan
Copy link
Collaborator Author

@AllentDan what's the performance of llama-7b now?

It does not influence the api_server performance. As for the stream_infer function. I tested 1000 prompts and got 131.17s for the main branch and 165.23s for the PR.

@AllentDan
Copy link
Collaborator Author

@AllentDan what's the performance of llama-7b now?

It does not influence the api_server performance. As for the stream_infer function. I tested 1000 prompts and got 131.17s for the main branch and 165.23s for the PR.

Tested the batch_infer function, no performance drop.

@AllentDan
Copy link
Collaborator Author

@AllentDan what's the performance of llama-7b now?

After recovering the original implementation, performance remained the same.

@lvhan028
Copy link
Collaborator

lvhan028 commented Apr 3, 2024

@grimoire According to @AllentDan test result, coroutine slows down the performance.
Any idea about the reason?

@grimoire
Copy link
Collaborator

grimoire commented Apr 3, 2024

rollback

@lvhan028 lvhan028 merged commit 93a09af into InternLM:main Apr 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants