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

pika: Don't automatically import twisted and tornado. #1129

Merged
merged 2 commits into from Nov 4, 2018

Conversation

Projects
None yet
3 participants
@timabbott
Contributor

timabbott commented Oct 18, 2018

Previously, the pika.adapters convenience import logic would end up
importing Twisted, Tornado, and AsyncIO if they are present in the
current Python environment, regardless of whether the user intended to
use them.

While the convenience names of e.g. pika.TornadoConnection (rather
than the more verbose pika.adapters.TornadoConnection`) were nice,
this had the downside of making importing pika expensive in those
environments; on my system, twisted added 150ms and tornado 10ms to
the based ~40ms import time for pika.

We fix this by removing those convenience imports; now, a user needs
to explicitly import pika.adapters.tornado_connection (aka plan to
actually use the relevant adapter feature) in order to end up
importing those large third-party modules.

Fixes #1128.


Pull request is still work-in-progress; while it's possible I didn't make any mistakes, this is at this point untested. I opened a PR at this point in development just to get CI to run.

pika: Don't automatically import twisted and tornado.
Previously, the pika.adapters convenience import logic would end up
importing Twisted, Tornado, and AsyncIO if they are present in the
current Python environment, regardless of whether the user intended to
use them.

While the convenience names of e.g. `pika.TornadoConnection` (rather
than the more verbose pika.adapters.TornadoConnection`) were nice,
this had the downside of making importing pika expensive in those
environments; on my system, twisted added 150ms and tornado 10ms to
the based ~40ms import time for pika.

We fix this by removing those convenience imports; now, a user needs
to explicitly import pika.adapters.tornado_connection (aka plan to
actually use the relevant adapter feature) in order to end up
importing those large third-party modules.

Fixes #1128.
@timabbott

This comment has been minimized.

Contributor

timabbott commented Oct 18, 2018

OK, I've now tested that it indeed solves the intended problem in Zulip with this 2-commit branch: https://github.com/timabbott/zulip/tree/pika-testing; with that, importing pika is just 35ms on my system.

timabbott/zulip@014c261 is the part of that PR that is the migration for this change; the other commit is just reverting the hack I'd added to work around this issue in Zulip.

Still haven't seen whether CI passes here, so I'm leaving the WIP tag on this PR.

@timabbott timabbott changed the title from [WIP] pika: Don't automatically import twisted and tornado. to pika: Don't automatically import twisted and tornado. Oct 18, 2018

@timabbott

This comment has been minimized.

Contributor

timabbott commented Oct 18, 2018

OK, it looks like CI passed (the only failure looks like a coverage infrastructure flake), so this is ready for a review.

@lukebakken lukebakken added this to the 0.13.0 milestone Oct 25, 2018

@lukebakken lukebakken self-assigned this Oct 25, 2018

@lukebakken lukebakken modified the milestones: 0.13.0, 1.0.0 Oct 29, 2018

@michaelklishin michaelklishin merged commit e58f3a1 into pika:master Nov 4, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment