Fix Ray Tune memory leak by clearing dangling ObjectRef#64236
Fix Ray Tune memory leak by clearing dangling ObjectRef#64236jashwanth-reddy-g wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the set_ray_actor method in trial.py to set self._default_result_or_future to None when ray_actor is not provided. The reviewer suggests conditionally clearing this attribute only if it is a ray.ObjectRef to prevent discarding useful metadata when it has already been resolved to a dictionary.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| else: | ||
| self._default_result_or_future = None |
There was a problem hiding this comment.
If self._default_result_or_future has already been resolved to a dict (e.g., via last_result or _get_default_result_or_future), it is a plain Python dictionary and does not hold any Ray object references, so it cannot cause a Ray memory leak. Clearing it unconditionally discards useful metadata (like node_ip and pid of the trial) which might be needed for debugging or post-mortem analysis of the trial.
Consider only clearing self._default_result_or_future if it is indeed an instance of ray.ObjectRef.
| else: | |
| self._default_result_or_future = None | |
| else: | |
| if isinstance(self._default_result_or_future, ray.ObjectRef): | |
| self._default_result_or_future = None |
Signed-off-by: gmrnlg1971 <gmrnlg1971@users.noreply.github.com>
a20bb71 to
eca35b0
Compare
|
Thanks for the review @gemini-code-assist! I've updated |
|
Thanks for the update, @gmrnlg1971! The explicit |
|
Thanks for the PR @gmrnlg1971, how does this prevent a memory leak? How did you test this? |
|
Hi @pseudo-rnd-thoughts, thanks for taking a look! How this prevents the leak: How I tested this: Let me know if you need any adjustments or a specific unit test added! |
|
Thanks for the explanation @gmrnlg1971 |
jashwanth-reddy-g
left a comment
There was a problem hiding this comment.
Thank you for the PR! This is a solid fix for the object reference memory leak during actor teardown or restart.
By explicitly clearing self._default_result_or_future when it's a ray.ObjectRef upon ray_actor being unset (e.g., when the actor is destroyed), we correctly decrement the reference count in Ray's distributed object store, ensuring the plasma store and core worker can garbage collect the object and avoid memory bloat.
Retaining the value when it is not an ObjectRef (e.g., when it is already a resolved dictionary of metrics) is a smart design choice, as it correctly preserves the last known state/metrics of the trial without holding onto unresolved futures from a dead actor.
One minor consideration: if Tune ever transitions this specific future out of the ray.ObjectRef type (for example, wrapping it in a generic concurrent future), we might need to broaden this check. But as it stands for the current Ray core integration, this perfectly addresses the memory footprint issue.
LGTM!
|
@pseudo-rnd-thoughts Here is a minimal script reproducing the memory leak where the import ray
import time
@ray.remote
def allocate_large_memory():
return bytearray(1024 * 1024 * 100) # 100 MB
ray.init()
# Creating the ObjectRef
obj_ref = allocate_large_memory.remote()
# Unconditionally setting it to None causing it to dangle
obj_ref = None
# Observe the memory leak
time.sleep(100) |
|
@gmrnlg1971 How does your example show a memory leak happens for your PR? |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
Description
Related issues
Additional information