Skip to content

Conversation

@vermouth1992
Copy link
Collaborator

Reverts #3803

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines +633 to +634
loop = asyncio.get_event_loop()
loop.run_until_complete(self.trainer_mode())
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

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.

Suggested change
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())

@vermouth1992
Copy link
Collaborator Author

Related to this: sgl-project/sglang#5162

@vermouth1992 vermouth1992 merged commit 8235425 into main Oct 20, 2025
37 of 38 checks passed
@vermouth1992 vermouth1992 deleted the revert-3803-fix/workers/async branch October 20, 2025 01:19
wangboxiong320 pushed a commit to wangboxiong320/verl that referenced this pull request Nov 1, 2025
NenoL2001 pushed a commit to NenoL2001/verl that referenced this pull request Nov 3, 2025
AlexJJ009 pushed a commit to AlexJJ009/verl that referenced this pull request Nov 5, 2025
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.

3 participants