Skip to content

fix: Add resource cleanup mechanism in CodeAgent #1056

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: Add resource cleanup mechanism in CodeAgent #1056

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7d7cf8
fix: Add resource cleanup mechanism in CodeAgent to prevent Docker co…
kingdomad Mar 23, 2025
92e5770
simplify CodeAgent cleanup logic
kingdomad Mar 23, 2025
09db403
Remove redundant conditional check
kingdomad Mar 24, 2025
5042e84
Revert "Remove redundant conditional check"
kingdomad Mar 24, 2025
2b04606
Add test for CodeAgent Docker resource cleanup
kingdomad Mar 24, 2025
ba41fe6
use try-except for executor deletion in cleanup method
kingdomad Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/smolagents/agents.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,3 +1278,12 @@ def to_dict(self) -> dict[str, Any]:
agent_dict["executor_kwargs"] = self.executor_kwargs
agent_dict["max_print_outputs_length"] = self.max_print_outputs_length
return agent_dict

def cleanup(self):
"""Clean up resources used by the agent, especially Docker container resources."""
if hasattr(self, "python_executor") and self.executor_type == "docker":
Copy link
Member

Choose a reason for hiding this comment

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

I think the first condition is always True: self.python_executor is defined at instantiation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll remove the first condition.

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 found that the first condition is necessary because there are some cases where the program exits before self.python_executor is fully instantiated, and in those cases, CodeAgent doesn't have the python_executor attribute.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of understanding your argument: if the program exits during instantiation, then the instance is not created, so self does not exist, so you cannot delete a non-existing instance. 🤔

Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. If CodeAgent encounters an error during instantiation, the instance will still exist in an incomplete form and will still execute the logic in the __del__ method and cleanup method. For example, if an error occurs when instantiating the docker executor, the CodeAgent won't have a python_executor attribute, but it will still execute CodeAgent.cleanup. Without the if hasattr(self, "python_executor") check, it would raise another error. I've already tested this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Even if, in 99% of cases, the __del__ method will not be executed and only in 1% of cases it will, we should still take this 1% into account and add the condition if hasattr(self, "python_executor") to enhance the robustness of the program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an interactive console, several factors can affect the execution of the __del__ method:

  1. The Python interpreter may retain references to recently created objects, especially in an interactive environment.
  2. Even if the variable a goes out of scope, Python's garbage collector might not immediately reclaim the object. Garbage collection is typically triggered when the system needs more memory or after specific time intervals.
  3. In an interactive environment, the interpreter maintains special variables (such as _) that reference the results of recent expressions, potentially extending the object's lifecycle.

Copy link
Member

Choose a reason for hiding this comment

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

I see your point, @kingdomad, and I appreciate the discussion.

  • I agree that we should not ignore the 1% of cases where __del__ is reliably called. See my suggestion below.
  • At the same time, we also need to account for the other 99% of cases where __del__ may not be called, ensuring proper resource cleanup in those scenarios. How do you think we could best handle this?

For the case where __del__ is called, I would suggest following the "Easier to ask for forgiveness than permission" approach, by wrapping the cleanup in a try block:

try:
    self.python_executor.delete()
except AttributeError:
    pass

Let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You make a valid point, but I think you're making this more complicated than it needs to be. I've consulted both Claude 3.7 and GPT-4o, and they both agree with my approach to modifying the code! Which approach do you prefer? I'm happy to implement your suggestion if that's what you'd like!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@albertvillanova I've reconsidered, and your approach makes sense. I'll implement the change using the try-except pattern as you suggested.

self.python_executor.delete()

def __del__(self):
"""Ensure resources are cleaned up when the agent is garbage collected."""
self.cleanup()