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

fix: close runtime after main completion #1860

Merged
merged 1 commit into from
May 17, 2024
Merged

fix: close runtime after main completion #1860

merged 1 commit into from
May 17, 2024

Conversation

xingyaoww
Copy link
Contributor

In the evaluation, I found that the number of browser processes will increase as the evaluation progresses -- until it brings the system to OOM.

I think This PR might fixes this by properly clean-up runtime resource when main finishes.

@yufansong
Copy link
Collaborator

I thought we close it here AgentManeger, and the AgentManager will closed automaticly by atexit. Does it not work correctly?

@li-boxuan
Copy link
Collaborator

the number of browser processes will increase as the evaluation progresses

BrowserEnv has atexit registered, so they should be closed if the program exits normally. Did you witness this when some evaluation process exits abnormally? Crash, SIGKILL, SIGTERM, etc.?

@xingyaoww
Copy link
Contributor Author

@yufansong @li-boxuan the thing is -- I'm running eval using multi-processing and the program won't terminate until all the eval instances finish running (so no atexit is trigger). I suspect this lead to several instances of OOM in the VM I'm using for eval..

Copy link
Collaborator

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

Interesting! I am not in favour of atexit package anyways so I am happy to see this merged.

Copy link
Collaborator

@yufansong yufansong left a comment

Choose a reason for hiding this comment

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

Get it. Let's merge it.

@xingyaoww xingyaoww merged commit 7ca5604 into main May 17, 2024
2 checks passed
@xingyaoww xingyaoww deleted the xw/close-runtime branch May 17, 2024 08:55
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