Skip to content
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

Feature: support ProcessPoolExecutor (Python >= 3.3 required) #31

Merged
merged 2 commits into from
Mar 27, 2016

Conversation

asfaltboy
Copy link
Contributor

Change Summary

  • include tests, some of which are skipped if on unsupported python version
  • use functools.partial instead of directly wrapping the request method,
    regardless of executor used - avoid code branching
  • use functools.partial(Session.request, self) instead of
    super(FuturesSession, self).request for pickling, again no code branching

Notes / Limitations

  • Python version 3.4 (actually >= 3.3.5) is required in order for us to pickle methods, due to python issue 9276. Though we may be able to use copy_reg to add this support to Python 2.
  • Python 3.5 is required to "create a session on demand", with lower versions an existing session is required. Probably due to one of the numerous pickle related issues fixed in 3.5.
  • Adding custom attributes to the response object in the callback is supported in ProcessPoolExecutor but only when the callback also modifies __attrs__ so it gets pickled and sent to parent process (see an example for that in the tests).
  • As mentioned, the background_callback MUST be a top-level (or otherwise importable) object. Additionally, since an instance of FuturesSession is passed to the callback (as self), it too must be importable, otherwise pickle will complain.

Conclusions

This relatively simple implementation fixes issue #11, however due to numerous caveats, I'd suggest we recommend it be used in Python 3.5 and above and explain the limitation of earlier versions. If earlier versions (like Python 2) are really required by someone they may need to implement a manual fix to the pickle implementation (e.g using copy_reg).

Personally, this PR solves my issue but I have yet to test it on production, hopefully, will do so in the near future.

* include tests, some of which are skipped if on unsupported python version
* use `functools.partial` instead of directly wrapping the `request` method,
  regardless of executor used - avoid code branching
* use `functools.partial(Session.request, self)` instead of
  `super(FuturesSession, self).request` for pickling, again no code branching

fixes ross#11
from os import environ
import unittest
import sys
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistically I prefer everything to be from X import Y, Z rather than import X... X.whatever

@ross
Copy link
Owner

ross commented Mar 12, 2016

Sorry for the slow response. Super busy at work as of late. Looks pretty good. Few small comments inline.

@asfaltboy
Copy link
Contributor Author

No problemo. I added an exception, some documentation and updated tests. Unfortunately, the README.rst does not render so well in GitHub, so feel free to style the doc to your liking.

@ross ross merged commit aa07c54 into ross:master Mar 27, 2016
@ross
Copy link
Owner

ross commented Mar 27, 2016

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants