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

[BUG] Commit #0d02bad may break srt_api by mis-use of asyncio #243

Open
2 tasks done
Maxwell-Lyu opened this issue Sep 11, 2024 · 4 comments
Open
2 tasks done

[BUG] Commit #0d02bad may break srt_api by mis-use of asyncio #243

Maxwell-Lyu opened this issue Sep 11, 2024 · 4 comments

Comments

@Maxwell-Lyu
Copy link

Maxwell-Lyu commented Sep 11, 2024

When you open an issue, please be sure to include the following

  • A descriptive title: [xxx] XXXX
  • A detailed description

Where's the problem

At generate_until in srt_api.py, at here

The author @kcz358 uses asyncio.as_completed in an order to send multiple requests to the server at the same time for faster evaluation, committed as 0d02bad.

What's happening

asyncio.as_completed is non-blocking, which means this iterator yield whatever request that has a response, regardless of their order of creation.
So, the order of responses in res may not be the order of the requests. In fact I should use "probably", because that probability is well above 50% whenever the CPU has more than 4 threads resulting in a num_processes of more than 2.
This make srt_api more like a rolling dice rather a valid evaluation, giving my colleage a evaluation score of non-sense.

How to fix

I'm no where near a professional python programmer, but I suggest using asyncio.gather instead, which is blocking thus preserves the order.
Or, if I mis-understood your code? Feel free to correct me and please forgive me if so.

Looking forward for your reply ;)

@kcz358
Copy link
Collaborator

kcz358 commented Sep 12, 2024

Yeah I think you are correct. I will try to fix this.

@kcz358
Copy link
Collaborator

kcz358 commented Sep 12, 2024

Hi @Maxwell-Lyu , I have created a PR in #244 to fix this issue. The fix is quite simple but there seems somehow some issues in current sglang that might causing the evaluation result to be inconsistent. I have tested that our result can be reproduced by the commit hash 2f1d92834f41df42e266ed6d7036b4add906d21f. I believe some changes after we PR have changed the behaviour of the model. I will try to check and fix it with the sgl team.

@Maxwell-Lyu
Copy link
Author

Thank you for your enthusiastic contribution to this project. Have a great day!

@kcz358
Copy link
Collaborator

kcz358 commented Sep 12, 2024

Hi @Maxwell-Lyu , I have fixed the bugs in the sglang in sgl-project/sglang#1402. Since this is already got merged, you no longer need to checkout from that specific commit hash.

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

No branches or pull requests

2 participants