bpo-5001: More-informative multiprocessing error messages by drallensmith · Pull Request #3079 · python/cpython

drallensmith · 2017-08-12T18:59:12Z

This is a fix of some of the uninformative asserts in multiprocessing, including in multiprocessing.managers and multiprocessing.queues. Those in the queues.py module remain AssertionErrors, as do some in the managers.py module; others have been changed to more specific errors. In BaseManager's interpretation of method_to_typeid entries, checking for type(key or value) == str has been replaced with isinstance(key or value, str).

In pool.py, various uninformative asserts have been changed to other appropriate errors (generally ValueErrors with a few TypeErrors) and information added; one instance of checking of type (== int) was changed to isinstance([thing], int).

In util.py, two uninformative asserts have been changed to other appropriate errors (one TypeError and one ValueError) and information added; with the TypeError, one instance of checking of type (== int) was changed to isinstance([thing],int).

I have also added a couple of comments. (Git, BTW, seems somewhat confused in that merging from python/cpython as upstream doesn't tell it that some of the commits in question have already been done in the master version. I'm not sure what to do about that - I've tried rebase before, but run into problems with github with it.)

Also a question - is there some way that I should mark pull requests that might be suitable for cherry-picking for other versions of Python? My prior multiprocessing PR is such; the code was identical in 2.7.13 and 3.5.3, for instance (I'm not suggesting applying it to 2.7, as opposed to 3.5-dev). I am more uncertain about this one.

https://bugs.python.org/issue5001

Replace assertions in error-reporting code with more-informative version that doesn't cause confusion over where and what the error is.

@pitrou

As suggested in PR comment by @pitrou, changing from SystemError; TypeError appears appropriate.

…by additional work)

…base

pitrou · 2017-08-18T15:58:16Z

        pass

-    def debug_info(self, c):
+    def debug_info(self, c): # c is unused?


I don't know how multiprocessing managers work, but c seems to be an API convention (see number_of_objects below which also has an unused c parameter).

Yes. I was wondering whether it could be used for additional debugging information. Admittedly, anything that called it should have the connection anyway!

pitrou · 2017-08-18T16:00:28Z

        with self.mutex:
-            assert self.id_to_refcount[ident] >= 1
+            if self.id_to_refcount[ident] <= 0:
+                raise ProcessError(


I would keep it an assert (or an AssertionError), since this really seems to be testing an internal consistency thing.

OK; AssertionError, due to line length limits.

pitrou · 2017-08-18T16:01:43Z

            for key, value in list(method_to_typeid.items()):
-                assert type(key) is str, '%r is not a string' % key
-                assert type(value) is str, '%r is not a string' % value
+                assert isinstance(key, str), '%r is not a string' % key


I'm not sure there's much point in making this change.

Umm... OK. Someone was complaining that this wasn't done with the old 5001 patch...

pitrou · 2017-08-18T16:02:36Z

 def worker(inqueue, outqueue, initializer=None, initargs=(), maxtasks=None,
           wrap_exception=False):
-    assert maxtasks is None or (type(maxtasks) == int and maxtasks > 0)
+    if maxtasks is not None:


worker() is an internal API, I'm not sure it's worth maintaining this more complicated error reporting.

I've tried to simplify it.

pitrou · 2017-08-18T16:04:23Z

        Equivalent of `func(*args, **kwds)`.
        '''
-        assert self._state == RUN
+        if self._state != RUN:


apply_sync() seems to already have this check, so we can probably remove it here.

pitrou · 2017-08-18T16:05:52Z

        cls._help_stuff_finish(inqueue, task_handler, len(pool))

-        assert result_handler.is_alive() or len(cache) == 0
+        if not result_handler.is_alive():


I think this is also an internal error, which makes assert fine here.

Or AssertionError?

pitrou · 2017-08-18T16:06:51Z


    def put(self, obj, block=True, timeout=None):
-        assert not self._closed
+        assert not self._closed, "Queue {0!r} already closed".format(self)


"already" sounds unwanted, since put() isn't asking to close the queue.

Good point. Substituted "has been".

pitrou · 2017-08-18T16:08:46Z

@@ -0,0 +1,7 @@
+This is a fix of some of the uninformative asserts in `multiprocessing`,


I don't think three separate NEWS entries and such a detailed description are warranted for what is a small improvement :-)

Well, it's now 1 short NEWS entry and I fixed most of the asserts. (Of course, then I had a typo due to an editor save problem... sigh. Unfortunately and ironically, the multiprocessing tests overload my current machine. Everything else works...) I was going nuts trying to figure out why something I knew I'd changed wasn't there, so shut down the pull request so as to not overload travis with both my fork's and cpython's builds. (I'm not paying them anything so I try to be nice to them.)

drallensmith · 2017-08-19T14:55:37Z

This now fixes, as far as I can tell, all uninformative asserts in multiprocessing except for those in Windows-specific code (I am not sufficiently familiar with processes on Windows to feel confident writing informative error messages).

pitrou · 2017-08-27T17:48:48Z


        Returns a pair of fds (status_r, data_w).  The calling process can read
-        the child process's pid and (eventually) its returncode from status_r.
+        the child process\'s pid and (eventually) its returncode from status_r.


I don't know if you changed this because it tripped your text editor, but this escaping is unnecessary inside a triple-quoted string.

Your guess is correct - it made xemacs unable to tell what was quoted after it, which got especially inconvenient when trying to make sure everything was indented correctly.

pitrou · 2017-08-27T17:53:25Z

-            for key, value in list(method_to_typeid.items()):
-                assert type(key) is str, '%r is not a string' % key
-                assert type(value) is str, '%r is not a string' % value
+            for key, value in list(method_to_typeid.items()): # isinstance?


I think these 3 lines should not be changed.

pitrou · 2017-08-27T17:54:16Z

    def apply(self, func, args=(), kwds={}):
        '''
        Equivalent of `func(*args, **kwds)`.
+        self._state should be RUN.


Hmm. Comments about internal state do not have a place in the docstring of a public method.

"Pool must be running"?

That sounds ok, though I'm not sure it bears mentioning.

pitrou · 2017-08-27T17:55:29Z

-        assert result_handler.is_alive() or len(cache) == 0
+        if (not result_handler.is_alive()) and (len(cache) != 0):
+            raise AssertionError(
+                "Cannot have cache with result_hander not alive")


I'm not sure I get the point of adding a message here. The message will not be understandable for the average user, and whoever wants to debug it has to read the source code anyway.

How does one tell where the AssertionError is happening, if the pool is being used not by the primary process but by a subprocess (possibly on another machine via a manager)?

Thanks to the line number in the traceback, I guess?

I wish that worked in the circumstances I noted above - see here, noting that it's a "nightly" build so includes the earlier patch, for an example.

pitrou · 2017-08-27T17:57:18Z

        while self._woken_count.acquire(False):
            res = self._sleeping_count.acquire(False)
-            assert res
+            assert res, 'res is {:n}'.format(res)


Not really useful, as res will simply be False here.

pitrou · 2017-08-27T17:57:56Z

    def notify(self, n=1):
        assert self._lock._semlock._is_mine(), 'lock is not owned'
-        assert not self._wait_semaphore.acquire(False)
+        assert not self._wait_semaphore.acquire(False), 'waiting to acquire'


The message is rather ambiguous and unhelpful to the user.

Could you take a look and see if my alternative phrasing is better?

I don't think it's better. It's ok to call notify() even if there are no waiters. This seems to be testing an internal invariant rather than an user error.

…once

pitrou · 2017-08-28T08:29:18Z

        while self._woken_count.acquire(False):
            res = self._sleeping_count.acquire(False)
-            assert res, 'res is {:n}'.format(res)
+            assert res, 'Attempting to notify but no sleepers?'


Same comment as for the other assert above. I think the original assert is ok.

pitrou · 2017-08-29T21:45:05Z

    def notify(self, n=1):
        assert self._lock._semlock._is_mine(), 'lock is not owned'
-        assert not self._wait_semaphore.acquire(False)
+        assert not self._wait_semaphore.acquire(False), 'Nobody waiting?'


Perhaps this is an overlook, but the message here is still wrong (it's ok to have nobody waiting).

Hrm. So exactly what is the assertion testing for? Thanks for the clarification!

It allows waking up the exact desired number of waiters (see below). The logic is a bit difficult to understand, as the various semaphores don't have an obvious meaning, they are just used for careful synchronization between notifier and waiters.

"notify: Should not have succeeded at acquiring _wait_semaphore"?

Done (slightly shorter version).

pitrou · 2017-08-29T21:45:29Z

        while self._woken_count.acquire(False):
            res = self._sleeping_count.acquire(False)
-            assert res
+            assert res, 'Attempting to notify but no sleepers?'


And here too (it's more of a sophisticated internal detail that's been tested).

OK... but what would the assert failing indicate?

That the implementation has a bug :-) I don't think we can qualify it any further.

Heh. "notify: Bug in sleeping_count.acquire - res should not be False"?

Fair enough :-)

drallensmith · 2017-08-29T22:11:26Z

-familiarity), all asserts in `multiprocessing` are now more informative, and
-some error types have been changed to more specific ones. - Allen W. Smith
-(drallensmith on github)
+Many asserts in `multiprocessing` are now more informative, and some error types have been changed to more specific ones.


I thought I was supposed to sign it?

No, you don't need to. You can add an entry in Misc/ACKS, though (keeping alphabetical order).

Already done, but thank you.

drallensmith · 2017-08-29T22:16:18Z

@@ -1 +1 @@
-Many asserts in `multiprocessing` are now more informative, and some error types have been changed to more specific ones.
+Many asserts in `multiprocessing` are now more informative, and some error types have been changed to more specific ones. - Allen W. Smith (drallensmith on github)


I had thought I was supposed to sign it?

OK, never mind.

…ke possible to merge

pitrou · 2017-08-29T22:28:51Z

Ok, I'm waiting for the CI results and I'll probably merge afterwards.

drallensmith · 2017-08-29T22:42:46Z

Excellent; thanks for all of your help!

P.S. Sorry for my earlier impatience; I did try to make up for it by doing a (non-core, of course) review of someone else's PR...

pitrou · 2017-08-29T22:52:28Z

Thank your for contributing this!

@pitrou

* Make error message more informative Replace assertions in error-reporting code with more-informative version that doesn't cause confusion over where and what the error is. * Additional clarification + get travis to check * Change from SystemError to TypeError As suggested in PR comment by @pitrou, changing from SystemError; TypeError appears appropriate. * NEWS file installation; ACKS addition (will do my best to justify it by additional work) * Making current AssertionErrors in multiprocessing more informative * Blurb added re multiprocessing managers.py, queues.py cleanup * Further multiprocessing cleanup - went through pool.py * Fix two asserts in multiprocessing/util.py * Most asserts in multiprocessing more informative * Didn't save right version * Further work on multiprocessing error messages * Correct typo * Correct typo v2 * Blasted colon... serves me right for trying to work on two things at once * Simplify NEWS entry * Update 2017-08-18-17-16-38.bpo-5001.gwnthq.rst * Update 2017-08-18-17-16-38.bpo-5001.gwnthq.rst OK, never mind. * Corrected (thanks to pitrou) error messages for notify * Remove extraneous backslash in docstring.

drallensmith added 7 commits August 12, 2017 06:59

Make error message more informative

23f0559

Replace assertions in error-reporting code with more-informative version that doesn't cause confusion over where and what the error is.

Additional clarification + get travis to check

09e2391

Change from SystemError to TypeError

e8b45b6

As suggested in PR comment by @pitrou, changing from SystemError; TypeError appears appropriate.

NEWS file installation; ACKS addition (will do my best to justify it …

1fa907e

…by additional work)

Merge branch 'master' of https://github.com/python/cpython - avoid re…

6344adc

…base

Making current AssertionErrors in multiprocessing more informative

3251312

Blurb added re multiprocessing managers.py, queues.py cleanup

cfae761

the-knights-who-say-ni added the CLA signed label Aug 12, 2017

bedevere-bot added the awaiting review label Aug 12, 2017

Further multiprocessing cleanup - went through pool.py

c0fb430

drallensmith changed the title ~~bpo-5001: More-informative multiprocessing error messages (managers.py, queues.py)~~ bpo-5001: More-informative multiprocessing error messages (managers.py, queues.py, pool.py) Aug 17, 2017

Fix two asserts in multiprocessing/util.py

fa446ae

drallensmith changed the title ~~bpo-5001: More-informative multiprocessing error messages (managers.py, queues.py, pool.py)~~ bpo-5001: More-informative multiprocessing error messages (managers.py, queues.py, pool.py, util.py) Aug 18, 2017

pitrou reviewed Aug 18, 2017

View reviewed changes

Most asserts in multiprocessing more informative

f90e227

drallensmith closed this Aug 18, 2017

Didn't save right version

8220864

drallensmith reopened this Aug 18, 2017

bedevere-bot added awaiting core review and removed awaiting review labels Aug 18, 2017

drallensmith changed the title ~~bpo-5001: More-informative multiprocessing error messages (managers.py, queues.py, pool.py, util.py)~~ bpo-5001: More-informative multiprocessing error messages Aug 19, 2017

pitrou reviewed Aug 27, 2017

View reviewed changes

Further work on multiprocessing error messages

70cd790

drallensmith closed this Aug 28, 2017

drallensmith added 3 commits August 27, 2017 20:58

Correct typo

ae7a107

Correct typo v2

f29c97f

Blasted colon... serves me right for trying to work on two things at …

fb3fb93

…once

drallensmith reopened this Aug 28, 2017

pitrou reviewed Aug 28, 2017

View reviewed changes

pitrou reviewed Aug 29, 2017

View reviewed changes

pitrou and others added 2 commits August 29, 2017 23:58

Simplify NEWS entry

58a5a8e

Update 2017-08-18-17-16-38.bpo-5001.gwnthq.rst

4bda162

drallensmith commented Aug 29, 2017

View reviewed changes

drallensmith and others added 4 commits August 29, 2017 17:18

Update 2017-08-18-17-16-38.bpo-5001.gwnthq.rst

04f0a44

OK, never mind.

Corrected (thanks to pitrou) error messages for notify

c96980d

Merge branch 'master' of https://github.com/drallensmith/cpython - ma…

01f21fd

…ke possible to merge

Remove extraneous backslash in docstring.

4a7558b

pitrou merged commit bd73e72 into python:master Aug 29, 2017

Mariatta removed the awaiting core review label Oct 4, 2017

		@@ -0,0 +1,7 @@
		This is a fix of some of the uninformative asserts in `multiprocessing`,

		@@ -1 +1 @@
		Many asserts in `multiprocessing` are now more informative, and some error types have been changed to more specific ones.
		Many asserts in `multiprocessing` are now more informative, and some error types have been changed to more specific ones. - Allen W. Smith (drallensmith on github)

Uh oh!

Uh oh!

Conversation

drallensmith commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Aug 18, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drallensmith Aug 18, 2017 • 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!

drallensmith commented Aug 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drallensmith commented Aug 12, 2017 •

edited

Loading

pitrou Aug 18, 2017 •

edited

Loading

drallensmith Aug 18, 2017 •

edited

Loading