msg235474 - (view) |
Author: Ole Streicher (olebole) |
Date: 2015-02-06 09:03 |
On Debian Hurd, there is no sem_open implementation. When I try there to create a Queue, I get different errors, depending on the Python version:
import multiprocessing
q = multiprocessing.Queue()
gives on Python 2.7.9:
maxsize = 0
def Queue(maxsize=0):
'''
Returns a queue object
'''
> from multiprocessing.queues import Queue
/usr/lib/python2.7/multiprocessing/__init__.py:217:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
__all__ = ['Queue', 'SimpleQueue', 'JoinableQueue']
import sys
import os
import threading
import collections
import time
import atexit
import weakref
from Queue import Empty, Full
import _multiprocessing
from multiprocessing import Pipe
> from multiprocessing.synchronize import Lock, BoundedSemaphore, Semaphore, Condition
/usr/lib/python2.7/multiprocessing/queues.py:48:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
'Lock', 'RLock', 'Semaphore', 'BoundedSemaphore', 'Condition', 'Event'
]
import threading
import os
import sys
from time import time as _time, sleep as _sleep
import _multiprocessing
from multiprocessing.process import current_process
from multiprocessing.util import Finalize, register_after_fork, debug
from multiprocessing.forking import assert_spawning, Popen
# Try to import the mp.synchronize module cleanly, if it fails
# raise ImportError for platforms lacking a working sem_open implementation.
# See issue 3770
try:
from _multiprocessing import SemLock
except (ImportError):
raise ImportError("This platform lacks a functioning sem_open" +
" implementation, therefore, the required" +
" synchronization primitives needed will not" +
> " function, see issue 3770.")
E ImportError: This platform lacks a functioning sem_open implementation, therefore, the required synchronization primitives needed will not function, see issue 3770.
/usr/lib/python2.7/multiprocessing/synchronize.py:59: ImportError
and on 3.4.2:
self = <multiprocessing.context.DefaultContext object at 0x1449b0c>, maxsize = 0
def Queue(self, maxsize=0):
'''Returns a queue object'''
from .queues import Queue
> return Queue(maxsize, ctx=self.get_context())
/usr/lib/python3.4/multiprocessing/context.py:101:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <multiprocessing.queues.Queue object at 0x511e8cc>, maxsize = 0
def __init__(self, maxsize=0, *, ctx):
if maxsize <= 0:
> maxsize = _multiprocessing.SemLock.SEM_VALUE_MAX
E AttributeError: 'module' object has no attribute 'SemLock'
/usr/lib/python3.4/multiprocessing/queues.py:38: AttributeError
Both traces are actually copied from Debian build logs:
2.7.9: https://buildd.debian.org/status/fetch.php?pkg=python-astropy&arch=hurd-i386&ver=1.0~rc1-1&stamp=1422724845
3.4.2: https://buildd.debian.org/status/fetch.php?pkg=python-astropy&arch=hurd-i386&ver=1.0~rc1-2&stamp=1423153281
Neither for 2.7.9 nor for 3.4.2 this behaviour is documented. Also, I would expect to have a NotImplementedError and not an ImportError or an AttributeError in such a case.
Please:
* document the behaviour of multiprocessing.Queue() in the case of an absent sem_open for both Python2 and Python3
* Consider raising NotImplementedError or being consistent in some other way.
|
msg235529 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-02-07 18:26 |
Having installed a fresh copy of Debian Hurd into a VM, I am able to reproduce the described issue using this 2-line snippet of code:
import multiprocessing
q = multiprocessing.Queue()
It was possible to reproduce the issue both using the builds of 2.7.9 and 3.4.2 that came with Debian Hurd and with clean builds of each from the Python source code for those releases.
Although already described in the original issue report, I'm taking the venerable astropy package out of the picture with this bare-bones provocation of the issue here. So running the above code snippet with 2.7.9 produces an ImportError and with 3.4.2 produces an AttributeError (details of error message are as previously described).
I agree that the behavior should be consistent between 2.7 and 3.x -- I'm leaning towards the consistent use of ImportError because that is what's used when threading support isn't available on a platform (i.e. 'import thread' or 'import threading' fails with an ImportError there).
As to where this should be documented in the documentation itself so that readers have a hope of finding it... that requires a bit more thought but I agree that it should be noted in the documentation. Thinking in pragmatic terms, most folks will probably not read the docs and instead notice it at the time of the failing import so the error message should be clear and consistent there.
It does not look like there is a working implementation of sem_open() on Debian Hurd yet -- is that correct? Otherwise there's another more attractive potential fix for this.
|
msg235530 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-02-07 20:35 |
Apologies -- it was already pointed out that there is no sem_open implementation on Hurd.
|
msg235665 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-02-10 05:11 |
Attached are proposed patches for default (3.5), 3.4, and 2.7 branches. (The patch for 3.4 is identical to that for 3.5 so there are only two files in total being attached.)
Regarding the exception being raised:
* An ImportError is now consistently being raised across Python versions. This is believed to be a more consistent behavior than using NotImplementedError but it is debatable whether all such situations might be switched to instead use NotImplementedError in the future.
* Implementation is done via a try-except around the problematic attempt to use _multiprocessing.SemLock inside the multiprocessing.queue module; it is believed this change has minimal risk.
* The text of the ImportError message differs only slightly from that used in the exception highlighted in issue 3770. In 2.7, the code raising this exception is encountered in both code execution paths (as described in issue 3770 and as described here) but in 3.x various changes have broken this shared execution path. After staring at it for quite some time, I believe a less-than-simple refactoring is required to get both execution paths to encounter the same exception -- I'm punting on that for now. Ultimately, I believe the goal was not to leave the error message inspired by issue 3770 but instead to clean it up and eliminate it as part of further code improvement efforts. I believe doing so is well beyond the scope of this issue but is still something that deserves addressing.
Regarding the documentation of the exception:
* A note has been added to the "Reference" section's "Pipes and Queues" subsection, describing the potential for and reasons behind an ImportError being encountered.
* The note that previously appeared in the "Introduction" section's "Synchronization between processes" subsection (introduced per issue 3770) has been relocated to similarly appear in the "Reference" section's "Synchronization primitives" subsection. Such a note appearing in an introduction section serves as a distraction to the reader who is hopefully learning about key concepts; it is better located in the formal documentation around the synchronization primitives where caveats and details should be conveyed. Making this change (relocation) keeps the two explanations of the ImportError arising from different places in the code better in parallel / symmetry with one another.
Running through the complete battery of tests on Debian Hurd was not possible as the sequence of tests could never complete (on the default/3.5 branch at least) -- various tests were observed to hang and never complete. While Debian Hurd may not be a mainstream supported platform for Python, given its lack of important functionality such as a working sem_open implementation, it is still rather interesting as a testing platform to see how things behave when the underlying system is unable to provide numerous chunks of key functionality to Python.
|
msg235667 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-02-10 05:22 |
To be clear, the changes to 2.7 are exclusively in the documentation. Changes to 3.4 and default (3.5) are in both documentation and code.
|
msg236010 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-02-14 23:34 |
Thanks for the patch, Davin. I left a couple of comments on Rietveld.
|
msg236191 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-02-18 20:29 |
Attaching revised patches with improved phrasing in the changes to the docs.
Thanks goes to Berker for reviewing and providing helpful feedback, especially paying close attention to detail in the docs.
|
msg237083 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-03-02 20:32 |
Attaching a further revised patch for 3.4 and 3.5/default, now with cleaned-up code style per the discussion in the review.
No changes are prompted for the patch to 2.7 as it only contained changes to the docs and nothing code-related.
Thanks again goes to Berker for the guidance.
|
msg237174 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-03-04 05:35 |
LGTM
|
msg237344 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-03-06 11:55 |
I'm not sure that this issue is worth to fix. AttributeError is not worse than ImportError, and users of 3.x expect AttributeError (if expect).
|
msg237346 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-03-06 12:09 |
But if this change is worth, I would write it as
- maxsize = _multiprocessing.SemLock.SEM_VALUE_MAX
+ # Can raise ImportError (see issues #3770 and #23400)
+ from .synchronize import SEM_VALUE_MAX as maxsize
This would avoid a duplication of the error message.
|
msg237370 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-03-06 18:48 |
Please keep in mind that this issue should only be encountered by people using Python 3.x on a platform like Hurd (an unsupported platform) that has no working sem_open implementation. If we try to imagine what Python 3.x users should expect, it's that they should see consistent behavior when importing or trying to use various pieces of multiprocessing on such an incomplete system. Or we should imagine Python 2.x users attempting to adopt Python 3.x and choosing to do so on such an incomplete system -- again, consistency is to be expected or at least desperately wished for. It is reasonable to believe that it is the exception that people like Ole have even seen this behavior and their reaction is that of a request for consistency. This is worth fixing.
Adding the "from .synchronize import SEM_VALUE_MAX as maxsize" as was suggested would indeed trigger the ImportError but it would also trigger this ImportError immediately upon simply doing an "import multiprocessing" which is not the current behavior on systems like Hurd. This would effectively cut off those Python 3.x users from other functionality in multiprocessing which does not depend upon a working sem_open implementation.
It'd make for a nice, tidy solution, that "from .synchronize ..." -- but I wasn't prepared to alter other expected behaviors. Hence the earlier comment about, "I believe a less-than-simple refactoring is required to get both execution paths to encounter the same exception".
If we don't do this for Hurd users (they need love too), we should do it for users like Ole who sometimes find themselves working on a cool package like astropy on a system like Hurd.
|
msg237388 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-03-06 22:29 |
> Please keep in mind that this issue should only be encountered by people
> using Python 3.x on a platform like Hurd (an unsupported platform) that has
> no working sem_open implementation.
Yes, and I compiled Python with disabled sem_open for testing.
Python not always specifies what exact type of exception is raised. When you
pass wrong type of argument to a function, it can raise TypeError or
AttributeError. If you request unsupported feature, it can raise ImportError,
AttributeError, or even NameError. Python and C implementations can raise
different errors (C implementation usually more strict). Not all such cases are
considered as bugs.
> Adding the "from .synchronize import SEM_VALUE_MAX as maxsize" as was
> suggested would indeed trigger the ImportError but it would also trigger
> this ImportError immediately upon simply doing an "import multiprocessing"
> which is not the current behavior on systems like Hurd.
No, I suggested to replace only the line that produces AttributeError now.
|
msg237400 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-03-06 23:36 |
> No, I suggested to replace only the line that produces AttributeError now.
Ah! Sorry, I misunderstood and incorrectly assumed you were imagining the import to happen at the top of the module.
I must confess I am hesitant about the idea of putting an import inside the Queue.__init__ (or any method) because I generally don't consider it a best practice -- that said, there are a number of places within (not only) the multiprocessing module where imports are performed dynamically as part of implementations for various methods, but it bothers me when I see those as well. Yet your solution is simple and offers the benefit of not having the same (or nearly the same) text appear twice in the code.
Berker: Do you have any strong feeling on this idea? If not, I am tempted to bend on my instinctive reaction here and go with Serhiy's style.
|
msg237415 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-03-07 04:44 |
> If not, I am tempted to bend on my instinctive reaction here and go with Serhiy's style.
+1 to Serhiy's suggestion.
|
msg237451 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-03-07 15:31 |
Attaching updated single patch for both default/3.5 and 3.4 to use Serhiy's insightful simplification.
This updated patch has been tested on current Debian Hurd (see earlier comments about full battery of tests there) and OS X 10.10 (full, verbose tests).
Note that the patch for 2.7 does not need a similar update as it contains purely documentation and no code changes.
Thanks goes to Serhiy and Berker for their thoughtful reviews and follow-ups.
|
msg240268 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-04-08 14:57 |
New changeset 749fd043de95 by Berker Peksag in branch '3.4':
Issue #23400: Raise same exception on both Python 2 and 3 if sem_open is not available.
https://hg.python.org/cpython/rev/749fd043de95
New changeset a49737bd6086 by Berker Peksag in branch 'default':
Issue #23400: Raise same exception on both Python 2 and 3 if sem_open is not available.
https://hg.python.org/cpython/rev/a49737bd6086
|
msg240270 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-04-08 15:12 |
New changeset c2f6b3677630 by Berker Peksag in branch '2.7':
Issue #23400: Add notes about the sem_open support of the host OS to
https://hg.python.org/cpython/rev/c2f6b3677630
|
msg240271 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-04-08 15:15 |
Fixed. Thank you all (and sorry for my late commit, Davin).
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:12 | admin | set | github: 67589 |
2015-04-08 15:15:17 | berker.peksag | set | status: open -> closed resolution: fixed messages:
+ msg240271
stage: commit review -> resolved |
2015-04-08 15:12:43 | python-dev | set | messages:
+ msg240270 |
2015-04-08 14:57:33 | python-dev | set | nosy:
+ python-dev messages:
+ msg240268
|
2015-03-07 15:31:16 | davin | set | files:
+ issue23400_py35_and_py34_furtherimproveddocsandstyle.patch
messages:
+ msg237451 |
2015-03-07 04:44:17 | berker.peksag | set | messages:
+ msg237415 |
2015-03-06 23:36:53 | davin | set | messages:
+ msg237400 |
2015-03-06 22:29:08 | serhiy.storchaka | set | messages:
+ msg237388 |
2015-03-06 18:48:57 | davin | set | messages:
+ msg237370 |
2015-03-06 12:09:53 | serhiy.storchaka | set | messages:
+ msg237346 |
2015-03-06 11:55:05 | serhiy.storchaka | set | nosy:
+ sbt, eric.snow, serhiy.storchaka, brett.cannon, ncoghlan, jnoller messages:
+ msg237344
|
2015-03-04 05:35:17 | berker.peksag | set | messages:
+ msg237174 stage: patch review -> commit review |
2015-03-02 20:32:42 | davin | set | files:
+ issue23400_py35_and_py34_improveddocsandstyle.patch
messages:
+ msg237083 |
2015-02-18 20:29:43 | davin | set | files:
+ issue23400_py27_improveddocs.patch |
2015-02-18 20:29:10 | davin | set | files:
+ issue23400_py35_and_py34_improveddocs.patch
messages:
+ msg236191 |
2015-02-14 23:34:03 | berker.peksag | set | nosy:
+ berker.peksag messages:
+ msg236010
|
2015-02-10 05:22:34 | davin | set | files:
+ issue23400_py27.patch
messages:
+ msg235667 |
2015-02-10 05:11:38 | davin | set | files:
+ issue23400_py35_and_py34.patch versions:
+ Python 3.5 messages:
+ msg235665
keywords:
+ patch stage: needs patch -> patch review |
2015-02-07 20:35:39 | davin | set | messages:
+ msg235530 |
2015-02-07 18:26:13 | davin | set | nosy:
+ davin
messages:
+ msg235529 stage: needs patch |
2015-02-06 22:40:33 | erik.bray | set | nosy:
+ erik.bray
|
2015-02-06 09:03:19 | olebole | create | |