-
Notifications
You must be signed in to change notification settings - Fork 16
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
distributed-lazy: delete unneeded temporaries after execution #258
Conversation
Also move distributed stuff to distributed.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM subject to these minor fixes.
pytato/distributed.py
Outdated
# Keep a count on how often each input name is used, | ||
# in order to be able to free them. | ||
partition_input_names_refcount: Dict[str, int] = {} | ||
|
||
for pid in pids_to_execute: | ||
for name in partition.parts[pid].all_input_names(): | ||
if name in partition_input_names_refcount: | ||
partition_input_names_refcount[name] += 1 | ||
else: | ||
partition_input_names_refcount[name] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a @memoize_method
on the partition. No need to recompute on every evaluation IMO. (Make sure to not modify the cached dictionary in place!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 4b592fb
pytato/distributed.py
Outdated
@@ -668,6 +681,10 @@ def wait_for_some_recvs() -> None: | |||
for send_req in send_requests: | |||
send_req.Wait() | |||
|
|||
if __debug__: | |||
for _, v in partition_input_names_refcount.items(): | |||
assert v == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also assert that the values are gone from context
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5481125
Co-authored-by: Andreas Klöckner <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM! I'll aply the suggestion.
This should be able to land after inducer/pytools#118 merges. |
It will need a new pytools release+conda package I guess. |
Yep, you're totally right. 🤦 Doing that now. |
Add-on for #148.
Should help with illinois-ceesd/mirgecom#599