Skip to content

Conversation

yinghai
Copy link
Contributor

@yinghai yinghai commented Mar 16, 2025

Motivation

Two improvement

  • Make Engine a context manager so that we don't forget to release the resource (e.g. dev memory) when we are done.
  • Make it optional to send SIGKILL to parent process when engine quit unexpectedly so that we can actually catch the error in pytest process.

Modifications

Checklist

@@ -246,7 +246,16 @@ def encode(

def shutdown(self):
"""Shutdown the engine"""
kill_process_tree(os.getpid(), include_parent=False)
Copy link
Contributor

@merrymercy merrymercy Mar 17, 2025

Choose a reason for hiding this comment

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

include_parent is already False by default, so it won't kill the parent process.
You changed it to True by default, and it failed many CI test. Is this still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I didn't notice the change. let me check

@yinghai yinghai requested a review from merrymercy March 17, 2025 02:32
@merrymercy merrymercy merged commit c614dbd into sgl-project:main Mar 17, 2025
18 of 21 checks passed
@yinghai yinghai deleted the yinghai/s branch March 17, 2025 18:07
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.

2 participants