ENH: delegate runtime resource cleanup to resource_tracker by pierreglaser · Pull Request #228 · joblib/loky

pierreglaser · 2019-12-05T13:12:59Z

This PR proposes to make resource_tracker in charge of cleaning up shared resources between between Python processes AT RUNTIME (and not only during shutdown as it is currently done). It does so by reference counting objects each time a Python process demands it (e.g means a Python process sends a "REGISTER"/"UNREGISTER" request to the resource_tracker). If an object's refcount drops to 0, the resource_tracker cleans it up. Previously, Python processes themselves would clean up the shared objects, and the resource_tracker would only intervene at the end of the program when all other processes connected to it have exited.

This PR is principally motivated by joblib/joblib#806, that shows the need for a proper way to track shared memmaped files usage between processes, as well as clean them up as soon as possible (and not wait for the shutdown of the joblib worker pool).

The resource tracker is now in charge of cleaning up shared resources between between Python processes. It does so by reference counting objects each time a Python process demands it (this means the Python process sends a "REGISTER"/"UNREGISTER" request to the resource_tracker). If an object's refcount drops to 0, the resource tracker cleans it up. Previously, Python processes themselves would clean up the shared objects, and the resource_tracker would only intervene at the end of the program when all other processes connected to it have exited.

pierreglaser

Reviewing myself to not forget anything because I'm about to do something else afterwards. @ogrisel it's still WIP but feel free to take a look.

tests/test_resource_tracker.py

loky/backend/resource_tracker.py

tests/test_resource_tracker.py

ogrisel

Thinking about it I think we should still preserve the existing behavior with "at exit" cleanup of registered resources by default. Un-registering should not trigger cleanups but instead make sure that the resource tracker does not collect the resource at all.

So instead we should probably add a new method for handling the "decref" with optional cleanup when the refcounts drops to 0. This new method could be called def maybe_cleanup(self, name).

The incref part can still be achieved via the "register" method (no need to introduce a new method).

ogrisel · 2019-12-05T13:56:43Z

maybe_collect or even maybe_unlink instead of maybe_cleanup?

The module docstring uses the unlink verb everywhere. So let's stick to it.

ogrisel

Some comments on typos / wording improvements.

loky/backend/resource_tracker.py

ogrisel · 2019-12-12T18:05:37Z

loky/backend/resource_tracker.py

 _CLEANUP_FUNCS = {
-    'folder': shutil.rmtree
+    'folder': shutil.rmtree,
+    'file': os.unlink


Maybe we should wrap those functions to ignore OSError (no such file or directory) if the resource has already been unlinked by another process.

I agree It would make some test logs less verbose, but I'm not sure about this change yet because it is useful information to know when we overshoot the number of maybe_cleanup calls, especially as I am calibrating the reference counting process of shared numpy arrays in joblib.

pierreglaser · 2019-12-12T18:10:09Z

I still have to push the MAYBE_UNLINK feature that i'm testing locally now.

now that unregister conserves its previous behavior, there is no reason to change the semaphores finalizer.

ogrisel · 2019-12-12T18:41:29Z

loky/backend/resource_tracker.py

+            while True:
+                line = f.readline()
+                if line == b'':  # EOF
+                    break


Question: why no for loop anymore?

Using a for-loop, in Python 2, the resource_tracker would wait until it got EOF before iterating over the lines written inside the pipe, processing them all at once (during shutdown). This is a bug that is actually present in loky master (but only for Python 2). I have not looked whether this behavior was documented or not.

ogrisel

This is starting to look good. Here are some more comments. Don't forget to add an entry to the changelog.

loky/backend/resource_tracker.py

loky/backend/synchronize.py

tests/test_resource_tracker.py

ogrisel

Some more comments:

loky/backend/resource_tracker.py

ogrisel

New batch of review:

tests/test_resource_tracker.py

…fcounting

* also reduce test output verbosity

pierreglaser · 2020-02-19T10:04:11Z

tests/test_resource_tracker.py

+        )
+        p.wait()
+        out, err = p.communicate()
+        assert p.returncode == 0, err


If the test errors, using this syntax instead of check_output leads to a much cleaner error reporting.

pierreglaser · 2020-02-19T11:24:26Z

@ogrisel all tests are passing. Only coverage is complaining (for now)

ogrisel

The remarks I made in the top level descriptions were marked as resolved where not addressed if I am not mistaken.

loky/backend/resource_tracker.py

Co-Authored-By: Olivier Grisel <olivier.grisel@ensta.org>

pierreglaser · 2020-02-19T14:26:50Z

My bad.

loky/backend/resource_tracker.py

ogrisel · 2020-02-19T14:28:00Z

2 left: https://github.com/joblib/loky/pull/228/files

ogrisel

Overall LGTM, just a few suggestions for additional fixes / improvements:

ogrisel · 2020-02-19T14:33:57Z

tests/utils.py

+try:
+    FileNotFoundError = FileNotFoundError
+except NameError:  # FileNotFoundError is Python 3-only
+    from loky.backend.semlock import FileNotFoundError


Nit: this could be simplified by always importing:

from loky.backend.semlock import FileNotFoundError

I don't think so, because FileNotFoundError is not a global of loky.backend.semlock for Python 3, it is a item of the __builtins__. At least it does not work on my machine.

tests/utils.py

tests/test_resource_tracker.py

ogrisel · 2020-02-19T14:49:47Z

Can you try to merge master into this branch to check whether that can help codedoc? The report link is broken on this PR for some reason.

loky/backend/resource_tracker.py

ogrisel · 2020-02-19T14:54:13Z

Hum codecov is now green... Maybe that was just the PyPy report that was late.

Co-Authored-By: Olivier Grisel <olivier.grisel@ensta.org>

…rreglaser/loky into resource-tracker-refcounting

tests/utils.py

ogrisel · 2020-02-19T16:17:46Z

The pthread import needs to be protected on Windows:

ImportError: cannot import name 'pthread' from 'loky.backend.semlock' (D:\a\1\s\loky\backend\semlock.py

pierreglaser · 2020-02-19T16:21:11Z

~~For some reason my last commit (fixinng the windows error) are not showing up.~~
EDIT: github's back on.

ogrisel · 2020-02-19T16:43:29Z

Merged! Thank you very much @pierreglaser.

ogrisel · 2020-02-19T16:44:10Z

I forgot to ask you to document the new feature in the changelog.

ogrisel · 2020-02-19T16:45:52Z

@pierreglaser we should now target 2.7.0 in the changelog because this is a new feature.

pierreglaser added 3 commits December 5, 2019 14:06

TST test new resource_tracker responsabilities

93ac29f

DOC mention new resource tracker responsabilities

31dfcb4

pierreglaser mentioned this pull request Dec 5, 2019

Use the resource_tracker to unlink shared temporary memmaps joblib/joblib#966

Merged

4 tasks

pierreglaser commented Dec 5, 2019

View reviewed changes

tests/test_resource_tracker.py Outdated Show resolved Hide resolved

loky/backend/resource_tracker.py Outdated Show resolved Hide resolved

tests/test_resource_tracker.py Outdated Show resolved Hide resolved

ogrisel reviewed Dec 5, 2019

View reviewed changes

pierreglaser added 3 commits December 12, 2019 16:26

FIX resource_tracker streaming process for 2.7

d650c87

FIX escape backslashes in Windows filepath regex

53e2c75

FIX close leaked fd's

b01d55c

ogrisel reviewed Dec 12, 2019

View reviewed changes

pierreglaser added 3 commits December 12, 2019 19:13

ENH implement maybe_unlink

ecf5247

TST adapt tests to maybe_unlink

b015a6f

MNT revert semaphore finalizer change

f88ed6d

now that unregister conserves its previous behavior, there is no reason to change the semaphores finalizer.

ogrisel reviewed Dec 12, 2019

View reviewed changes

pierreglaser force-pushed the resource-tracker-refcounting branch from 3f3e410 to f88ed6d Compare December 12, 2019 20:32

pierreglaser added 2 commits December 19, 2019 13:30

MNT cosmetics, robustness, logs improvements

bbd7764

MNT remove pytest pragmas for verbose mode

18847ea

pierreglaser mentioned this pull request Dec 19, 2019

Refactor CI configuration to use tox and Azure Pipelines for everything #229

Merged

ENH delete folders after every other resource

9dbb68d

ogrisel reviewed Dec 23, 2019

View reviewed changes

loky/backend/resource_tracker.py Outdated Show resolved Hide resolved

loky/backend/resource_tracker.py Outdated Show resolved Hide resolved

MNT renamings

c3d383e

pierreglaser force-pushed the resource-tracker-refcounting branch from 608abff to c3d383e Compare January 14, 2020 16:51

ogrisel reviewed Jan 15, 2020

View reviewed changes

ogrisel and others added 4 commits January 24, 2020 13:52

Merge remote-tracking branch 'origin/master' into resource-tracker-re…

887cccc

…fcounting

cosmetics

f4f7703

simpler pattern matching

ef2817a

rely on multiprocessing logging utilities

79648d0

* also reduce test output verbosity

cleanups

4a943cf

pierreglaser commented Feb 19, 2020

View reviewed changes

pierreglaser added 2 commits February 19, 2020 11:27

linux/macos sempahore compatibility

f039e01

handle windows-specific errors

7de75dd

clean up typos, add some informative comments

f19cb9c

ogrisel reviewed Feb 19, 2020

View reviewed changes

loky/backend/resource_tracker.py Outdated Show resolved Hide resolved

loky/backend/resource_tracker.py Outdated Show resolved Hide resolved

Apply suggestions from code review

99632a0

Co-Authored-By: Olivier Grisel <olivier.grisel@ensta.org>

ogrisel reviewed Feb 19, 2020

View reviewed changes

loky/backend/resource_tracker.py Outdated Show resolved Hide resolved

typos/corrections in the comment

6b8aeb5

ogrisel approved these changes Feb 19, 2020

View reviewed changes

ogrisel reviewed Feb 19, 2020

View reviewed changes

loky/backend/resource_tracker.py Outdated Show resolved Hide resolved

pierreglaser and others added 4 commits February 19, 2020 15:55

Apply suggestions from code review

c1faaf8

Co-Authored-By: Olivier Grisel <olivier.grisel@ensta.org>

Update tests/test_resource_tracker.py

0ee88c2

Co-Authored-By: Olivier Grisel <olivier.grisel@ensta.org>

cleanup resource usage also in Linux

d030a8a

Merge branch 'resource-tracker-refcounting' of https://github.com/pie…

d71509a

…rreglaser/loky into resource-tracker-refcounting

ogrisel reviewed Feb 19, 2020

View reviewed changes

tests/utils.py Show resolved Hide resolved

pierreglaser added 2 commits February 19, 2020 16:21

fix windows compatibility issues

d352161

style changes

9ea3dff

trigger github with empty commit

816c5f7

ogrisel merged commit 74737d7 into joblib:master Feb 19, 2020

Conversation

pierreglaser commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierreglaser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogrisel Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

pierreglaser Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

pierreglaser commented Dec 12, 2019

Uh oh!

ogrisel Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

pierreglaser Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierreglaser Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierreglaser commented Feb 19, 2020

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pierreglaser commented Feb 19, 2020

Uh oh!

Uh oh!

ogrisel commented Feb 19, 2020

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

pierreglaser commented Dec 5, 2019 •

edited

Loading

pierreglaser left a comment •

edited

Loading

ogrisel commented Dec 5, 2019 •

edited

Loading

pierreglaser Dec 12, 2019 •

edited

Loading

ogrisel left a comment •

edited

Loading

pierreglaser Feb 19, 2020 •

edited

Loading

pierreglaser Feb 19, 2020 •

edited

Loading

pierreglaser commented Feb 19, 2020 •

edited

Loading

ogrisel commented Feb 19, 2020 •

edited

Loading