-
Notifications
You must be signed in to change notification settings - Fork 3k
Revert "[worker] fix: create a new event loop if none exists when building rollouts" #3820
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
Conversation
…lding ro…" This reverts commit e0e352b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request reverts a change related to asyncio event loop creation. While the revert might fix an issue with asyncio.run() being called from an already running event loop, it introduces a critical risk of a RuntimeError if no event loop is present. My review includes a suggestion to adopt a more robust pattern for handling the event loop, similar to what's used elsewhere in the same file, to prevent potential crashes.
| loop = asyncio.get_event_loop() | ||
| loop.run_until_complete(self.trainer_mode()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change reverts the use of asyncio.run(), likely because it can cause a RuntimeError if an event loop is already running. However, the new implementation using asyncio.get_event_loop() will raise a RuntimeError if no event loop is running, which can happen depending on the execution context.
A more robust approach is to handle both cases, as is done in the generate_sequences method within this same file (lines 911-915). This ensures an event loop is always available.
I suggest adopting that pattern here to prevent potential crashes.
| loop = asyncio.get_event_loop() | |
| loop.run_until_complete(self.trainer_mode()) | |
| try: | |
| loop = asyncio.get_event_loop() | |
| except RuntimeError: | |
| loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(loop) | |
| loop.run_until_complete(self.trainer_mode()) |
|
Related to this: sgl-project/sglang#5162 |
…lding rollouts" (volcengine#3820) Reverts volcengine#3803
…lding rollouts" (volcengine#3820) Reverts volcengine#3803
…lding rollouts" (volcengine#3820) Reverts volcengine#3803
…lding rollouts" (volcengine#3820) Reverts volcengine#3803
…lding rollouts" (volcengine#3820) Reverts volcengine#3803
…lding rollouts" (volcengine#3820) Reverts volcengine#3803
…lding rollouts" (volcengine#3820) Reverts volcengine#3803
Reverts #3803