Revert "[worker] fix: create a new event loop if none exists when building rollouts"#3820
Conversation
…lding ro…" This reverts commit e0e352b.
There was a problem hiding this comment.
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.
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" (verl-project#3820) Reverts verl-project#3803
…lding rollouts" (verl-project#3820) Reverts verl-project#3803
…lding rollouts" (verl-project#3820) Reverts verl-project#3803
…lding rollouts" (verl-project#3820) Reverts verl-project#3803
…lding rollouts" (verl-project#3820) Reverts verl-project#3803
…lding rollouts" (verl-project#3820) Reverts verl-project#3803
…lding rollouts" (verl-project#3820) Reverts verl-project#3803
…lding rollouts" (verl-project#3820) Reverts verl-project#3803
Reverts #3803