bindings/python: destroy future after use #2553

andre-merzky · 2019-11-22T10:17:42Z

Following a suggestion by @grondo, the submit_get_id-future sees
delayed / no garbage collection and causes a significant slowdowns due to
apparent limits in the future layer. This commit adds explicit
destruction after future completion, thus avoiding that issue.

I am not familiar with your PR policies: what branch to point to, what additional information to provide, etc. Please let me know this is off...

This closes #2549

garlick

Looks good and thanks for the PR!

We run python code through the "black" formatter in travis-ci and it's called out a need for this change:

--- src/bindings/python/flux/job.py	2019-11-22 10:18:42.824332 +0000
+++ src/bindings/python/flux/job.py	2019-11-22 10:18:56.523516 +0000
@@ -53,6 +53,5 @@
     future = submit_async(flux_handle, jobspec, priority, flags)
     jid = submit_get_id(future)
     future.destroy()
     return jid
 
-

Couple other minor nits if you don't mind:

fix misspelling of "slowdowns" in commit message
add "bindings/python:" prefix to commit title

For future reference, and for what it's worth, we put up these documents describing flux project practices:

grondo · 2019-11-22T14:18:23Z

Awesome! Thanks!

@grondo

Following a suggestion by @grondo, the `submit_get_id`-future sees delayed / no garbage collection and causes a significant slowdown due to apparent limits in the future layer. This commit adds explicit destruction after future completion, thus avoiding that issue.

garlick

Perfect, thanks again! I'll set the merge-when-passing label and then mergify will take care of the merge.

garlick · 2019-11-22T15:18:30Z

Uh oh, looks like one of the builders failed in t2206-job-manager-bulk-state.t with this message and a long backtrace:

*** Error in `python': corrupted size vs. prev_size: 0x0000000001e87360 ***

That runs a python script t/job-manager/bulk-state.py which uses job.submit() .

So we'd better run that down before this goes in (not putting that on you @andre-merzky)

garlick · 2019-11-22T15:23:42Z

Out of my depth here, but a66ead4 relates to the lifetimes of futures in the python bindings. Maybe @SteVwonder will have some thoughts on this pr?

Edit: fixed commit ref

SteVwonder

Thanks @andre-merzky for posting this fix. Some inline comments below. For the one suggested change to work, the __clear method in wrapper.py will need to be renamed to _clear (single underscore).

Using the _clear method should avoid the memory corruption we are seeing in Travis since the _clear method has some extra protections to ensure the C destructor is only called once.

SteVwonder · 2019-11-23T00:19:15Z

src/bindings/python/flux/future.py

+    def destroy(self):
+        self.pimpl.destroy()
+


I'd prefer to not include this public method. I previously attempted to make this work, but exposing it publically opens up all sorts of edge-cases. I'd prefer to leverage the _clear method in the Wrapper class.

SteVwonder · 2019-11-23T00:19:31Z

src/bindings/python/flux/job.py

@@ -51,4 +51,6 @@ def submit_get_id(future):

 def submit(flux_handle, jobspec, priority=lib.FLUX_JOB_PRIORITY_DEFAULT, flags=0):
    future = submit_async(flux_handle, jobspec, priority, flags)
-    return submit_get_id(future)
+    jid = submit_get_id(future)
+    future.destroy()


Suggested change

future.destroy()

future.pimpl._clear()

SteVwonder · 2019-11-23T00:25:08Z

Rename patch:

diff --git a/src/bindings/python/flux/wrapper.py b/src/bindings/python/flux/wrapper.py
index 4a58e7f12..a74d090f5 100644
--- a/src/bindings/python/flux/wrapper.py
+++ b/src/bindings/python/flux/wrapper.py
@@ -326,7 +326,7 @@ class Wrapper(WrapperBase):
         setattr(self, name, new_method)
         return new_method

-    def __clear(self):
+    def _clear(self):
         # avoid recursion
         if hasattr(self, "_handle") and self._handle is not None:
             handle = self._handle
@@ -354,14 +354,14 @@ class Wrapper(WrapperBase):
                     )
                 )
         if self._handle is not None:
-            self.__clear()
+            self._clear()
         self._handle = h

     def __del__(self):
-        self.__clear()
+        self._clear()

     def __enter__(self):
         return self

     def __exit__(self, type_arg, value, unused):
-        self.__clear()
+        self._clear()

garlick · 2019-11-25T14:39:44Z

I gave @SteVwonder's suggested modification to this PR a try this morning and it seems to both get past the 500 job hang and pass make check, at least in limited testing on my desktop.

Edit: although this pylint error pops up in travis:

src/bindings/python/flux/job.py:55:4: W0212: Access to a protected member _clear of a client class (protected-access)

SteVwonder · 2019-11-27T02:00:49Z

Thanks @garlick for giving it a spin. Adding # pylint: disable=protected-access to right above the offending line should make the pylint error go away for now.

garlick · 2019-11-27T02:14:38Z

Thanks @garlick for giving it a spin. Adding # pylint: disable=protected-access to right above the offending line should make the pylint error go away for now.

Oh cool. Is this the way to go or should we try to make your weakref thing in #2549 work?

SteVwonder · 2019-11-27T05:21:36Z

I think ultimately we need to fix the circular reference so that futures exposed to users aren’t leaked, but if this issue is blocking @andre-merzky, then I have no issue with this patch for now.

andre-merzky · 2019-11-27T06:27:39Z

I think ultimately we need to fix the circular reference so that futures exposed to users aren’t leaked, but if this issue is blocking @andre-merzky, then I have no issue with this patch for now.

Thanks for that! But I am still prototyping and quite a bit away from experiments or production, and don't mind waiting. I am using a version of this PR right now, so I am not stuck.

Allow future.pimpl._clear() to be called from other parts of the binding by dropping an underscore from its prefix. The _clear method has some extra protections to ensure the C destructor is only called once, thus future.pimpl._clear() is the preferred way to explicitly dispose of a future. This is a prereq for explicit cleanup of futures in the job() class. Originally proposed in flux-framework#2553.

@SteVwonder

Problem: job.submit() and job.wait() both seem to leak futures. The futures in these "synchronous" methods go out of scope and thus should be automatically destroyed, but due to a circular reference alluded to by @SteVwonder in flux-framework#2549, they persist. As a workaround, explicitly call _clear() on the futures in these methods. Credit goes to @andre-merzky for proposing the first version of this patch in flux-framework#2553, based on a suggestion by @grondo, with changes proposed by @SteVwonder. Group effort! Fixes flux-framework#2549.

Allow future.pimpl._clear() to be called from other parts of the binding by dropping an underscore from its prefix. The _clear method has some extra protections to ensure the C destructor is only called once, thus future.pimpl._clear() is the preferred way to explicitly dispose of a future. This is a prereq for explicit cleanup of futures in the job() class. Originally proposed in flux-framework#2553.

@SteVwonder

Problem: job.submit() and job.wait() both seem to leak futures. The futures in these "synchronous" methods go out of scope and thus should be automatically destroyed, but due to a circular reference alluded to by @SteVwonder in flux-framework#2549, they persist. As a workaround, explicitly call _clear() on the futures in these methods. Credit goes to @andre-merzky for proposing the first version of this patch in flux-framework#2553, based on a suggestion by @grondo, with changes proposed by @SteVwonder. Group effort! Fixes flux-framework#2549.

garlick · 2019-12-02T18:50:59Z

#2563 just went in and included bits gathered from this PR, so closing this one. Thanks @andre-merzky for running this down and proposing the initial fix!

garlick approved these changes Nov 22, 2019

View reviewed changes

andre-merzky force-pushed the fix/issue_321 branch 2 times, most recently from 85a2d42 to ae6ea79 Compare November 22, 2019 14:15

andre-merzky force-pushed the fix/issue_321 branch from 0d06d92 to 99687aa Compare November 22, 2019 14:26

andre-merzky changed the title ~~destroy future after use~~ bindings/python: destroy future after use Nov 22, 2019

andre-merzky requested a review from garlick November 22, 2019 14:33

garlick approved these changes Nov 22, 2019

View reviewed changes

garlick added merge-when-passing and removed merge-when-passing labels Nov 22, 2019

SteVwonder requested changes Nov 23, 2019

View reviewed changes

garlick mentioned this pull request Nov 25, 2019

job-manager: add flux_job_wait() #2546

Merged

garlick closed this Dec 2, 2019

andre-merzky deleted the fix/issue_321 branch December 2, 2019 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bindings/python: destroy future after use #2553

bindings/python: destroy future after use #2553

andre-merzky commented Nov 22, 2019

garlick left a comment

grondo commented Nov 22, 2019

garlick left a comment

garlick commented Nov 22, 2019 •

edited

Loading

garlick commented Nov 22, 2019 •

edited

Loading

SteVwonder left a comment

SteVwonder Nov 23, 2019

SteVwonder Nov 23, 2019

SteVwonder commented Nov 23, 2019

garlick commented Nov 25, 2019 •

edited

Loading

SteVwonder commented Nov 27, 2019

garlick commented Nov 27, 2019

SteVwonder commented Nov 27, 2019

andre-merzky commented Nov 27, 2019

garlick commented Dec 2, 2019

bindings/python: destroy future after use #2553

bindings/python: destroy future after use #2553

Conversation

andre-merzky commented Nov 22, 2019

garlick left a comment

Choose a reason for hiding this comment

grondo commented Nov 22, 2019

garlick left a comment

Choose a reason for hiding this comment

garlick commented Nov 22, 2019 • edited Loading

garlick commented Nov 22, 2019 • edited Loading

SteVwonder left a comment

Choose a reason for hiding this comment

SteVwonder Nov 23, 2019

Choose a reason for hiding this comment

SteVwonder Nov 23, 2019

Choose a reason for hiding this comment

SteVwonder commented Nov 23, 2019

garlick commented Nov 25, 2019 • edited Loading

SteVwonder commented Nov 27, 2019

garlick commented Nov 27, 2019

SteVwonder commented Nov 27, 2019

andre-merzky commented Nov 27, 2019

garlick commented Dec 2, 2019

garlick commented Nov 22, 2019 •

edited

Loading

garlick commented Nov 22, 2019 •

edited

Loading

garlick commented Nov 25, 2019 •

edited

Loading