-
Notifications
You must be signed in to change notification settings - Fork 34
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
Allow using dill for pickling #36
Conversation
@unittest.skipIf( | ||
"multiprocess.util" in str(util), | ||
"concurrent.futures is not yet supported by uqfoundation " | ||
"(https://github.com/uqfoundation/pathos/issues/90)" | ||
) |
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.
sub-optimal to skip this test... especially for people that are using concurrent.futures.ProcessPoolExecutor
in combination with aioprocessing
, in an environment where multiprocess
happens to be installed. As the author stated in uqfoundation/pathos#90, it should be an easy PR. I've tried figuring it out multiple times as I keep running into multiprocessing problems in async contexts, but repeatedly stranded.
Here's the traceback for this test case, caused by ProcessPoolExecutor internally using multiprocessing
instead of multiprocess
:
test_executor (tests.queue_test.ManagerQueueTest) ... Process SpawnProcess-1:
Traceback (most recent call last):
File "~/.pyenv/versions/3.8.6/lib/python3.8/multiprocessing/process.py", line 315, in _bootstrap
self.run()
File "~/.pyenv/versions/3.8.6/lib/python3.8/multiprocessing/process.py", line 108, in run
self._target(*self._args, **self._kwargs)
File "~/.pyenv/versions/3.8.6/lib/python3.8/concurrent/futures/process.py", line 233, in _process_worker
call_item = call_queue.get(block=True)
File "~/.pyenv/versions/3.8.6/lib/python3.8/multiprocessing/queues.py", line 116, in get
return _ForkingPickler.loads(res)
File "~/.pyenv/versions/pi/lib/python3.8/site-packages/multiprocess/managers.py", line 959, in RebuildProxy
return func(token, serializer, incref=incref, **kwds)
File "~/.pyenv/versions/pi/lib/python3.8/site-packages/multiprocess/managers.py", line 809, in __init__
self._incref()
File "~/.pyenv/versions/pi/lib/python3.8/site-packages/multiprocess/managers.py", line 863, in _incref
conn = self._Client(self._token.address, authkey=self._authkey)
File "~/.pyenv/versions/pi/lib/python3.8/site-packages/multiprocess/connection.py", line 511, in Client
answer_challenge(c, authkey)
File "~/.pyenv/versions/pi/lib/python3.8/site-packages/multiprocess/connection.py", line 762, in answer_challenge
raise AuthenticationError('digest sent was rejected')
multiprocess.context.AuthenticationError: digest sent was rejected
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.
note that even if uqfoundation would add compatibility, and expose pathos.futures.ProcessPoolExecutor
or so, this error would still persist for people trying to combine the concurrent.futures.ProcessPoolExecutor
with aioprocessing[dill]
.
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.
I think there should be a note in the readme about this incompatibility when using dill, as well as documenting the workaround that forces usage of multiprocessing
. People upgrading to this version could suddenly have their code break, so it should be very clear how to fix it.
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.
Sure thing, added. Assuming semver and this being a potentially breaking change ^ I went ahead and bumped to 2.0.0 🤙
I like the idea - my main concern is that there's no way to opt out of using |
how about an env var |
aioprocessing/managers.py
Outdated
@@ -1,29 +1,22 @@ | |||
import asyncio | |||
from multiprocessing.util import register_after_fork | |||
from queue import Queue | |||
from threading import ( |
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.
These imports should still come from threading
. They're getting proxied, so we don't need the multiprocessing versions.
@unittest.skipIf( | ||
"multiprocess.util" in str(util), | ||
"concurrent.futures is not yet supported by uqfoundation " | ||
"(https://github.com/uqfoundation/pathos/issues/90)" | ||
) |
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.
I think there should be a note in the readme about this incompatibility when using dill, as well as documenting the workaround that forces usage of multiprocessing
. People upgrading to this version could suddenly have their code break, so it should be very clear how to fix it.
Yes, I think that is fine. |
Thanks for the contribution, @ddelange! |
Hi @dano, are there any blockers for the 2.0.0 release? Are there other things you wanna do/I can help with before releasing? |
@ddelange Just inertia 🙂 I will try to do it sometime today or this weekend. |
@ddelange Done! |
Hello 👋
This PR does not change any behaviour of the library, apart from when the
multiprocess
module is installed in the current environment (installable withpip install multiprocess
orpip install aioprocessing[dill]
).dill
allows for pickling locally defined functions, lambdas, and all sorts of other exotic cases where stdlib will complain.In this gist, I solved a case of
PicklingError: Can't pickle <function test4 at 0x118bb4940>: it's not the same object as __main__.test4
by monkey patchingaioprocessing
, and then thought 'why not PR' :)If you have reasons to constitutionally dislike this PR, feel free to close it, otherwise I'm happy to take on suggestions!