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

Gevent compatability layer #647

Open
wants to merge 25 commits into
base: v3.0.0-rc
from

Conversation

@daviskirk
Copy link
Contributor

daviskirk commented Jul 8, 2019

This is a first (raw) draft implementing some of the ideas mentioned here: https://www.notion.so/Support-concurrency-backends-other-than-Eventlet-aefdf261f4344a05859321967dcdf42f

This code implements gevent (which is quite similar to eventlet but more widely used and better supported).

My thoughts are that it is a better starting point for discussing some Questions instead of on a completely white page.

A few Questions I had:

  • How should the compatability layer be implemented?
    • Currently the backend is chosen at import time. Is this ok or do we need to be able to swap back and forth during tests (I would have no idea how to do this given, both eventlet and gevent rely on monkeypatching).
    • Conditional importing makes the nameko.concurrency module home to the correct backend. The disadvantage here is that everything has to be imported (wsgi, greenpools and so on) when the backend is initialized. Possibly there is a way of having two parallel packages that are aliased correctly at import time, but I'm not sure how to do this (some sort of magical modification of the python path??). Also, it might be easier to have the concurrency primitives of nameko core in a single module instead of "rebuilding" an entire module structure like this:
    concurrency/
    |- __init__.py
    |- gevent/
       |- __init__.py
       |- wsgi/
          |- ...
       |- event/
          |- ... 
    |- eventlet/
       |- ...    
    
    • The gevent compatability module tried to emulate what eventlet does (less code changes in nameko core). However, one could discuss if using the gevent api (more used?) or a simpler nameko specific api (less intricate than eventlet/gevent, but would also mean learning yet another variant) is nicer.
    • I did not implement websockets with gevent yet. I'm sure it's possible but it seems like the eventlet websocket handler is more "builtin" and so building the same in gevent might not be as simple as the event/rpc/web translation.
daviskirk added 4 commits Jul 2, 2019
@daviskirk daviskirk force-pushed the daviskirk:gevent branch from 239cf86 to c8f11fd Jul 8, 2019
daviskirk added 2 commits Jul 8, 2019
@daviskirk daviskirk force-pushed the daviskirk:gevent branch from f80fff0 to f6ad276 Jul 8, 2019
daviskirk added 2 commits Jul 8, 2019
@daviskirk daviskirk force-pushed the daviskirk:gevent branch from 38b180d to 6bbba42 Jul 8, 2019
@mattbennett

This comment has been minimized.

Copy link
Member

mattbennett commented Jul 9, 2019

Holy cow @daviskirk! This is a huge step forwards 🙌

How should the compatibility layer be implemented? Currently the backend is chosen at import time.

I think it should be possible to specify the concurrency backend with the configuration file. I had imagined that we'd have Nameko primitives in nameko.concurrency that delegated to the chosen backend at runtime. This may or may not be possible, but I think it's a good target and we can see if we can get there.

It might be easier to have the concurrency primitives of Nameko core in a single module

In line with above, this is what I imagined

One could discuss if using the gevent api or a simpler nameko specific api is nicer

Good point. I don't particularly mind whether the Nameko concurrency primitives look more like eventlet, more like gevent, or forge their own path. The latter may be nicest. Ultimately I don't think Nameko has particularly complicated concurrency requirements, so simpler is probably better.

I did not implement websockets with gevent yet

I think this is fine. I would be happy with websockets only being supported with the eventlet backend honestly.

Tangential point -- I would love to see extensions for an ASGI server like https://gitlab.com/pgjones/hypercorn available for Nameko. Hypercorn in particular is based on h11 and h2, which are sans-io libraries and therefore easy to use with any concurrency backend.

daviskirk added 3 commits Jul 9, 2019
- Add configuration options for nameko
@daviskirk daviskirk force-pushed the daviskirk:gevent branch from d22052b to 1a8c7c1 Jul 20, 2019
- Rename eventlet specific GreenPool to more abstract Pool
- Fix server test where test could pass without ever hitting route
- Fix imports and pylint
@daviskirk daviskirk force-pushed the daviskirk:gevent branch 3 times, most recently from ef33d7f to 4634e6c Jul 21, 2019
@daviskirk daviskirk force-pushed the daviskirk:gevent branch from 4634e6c to 7cd2f8f Jul 21, 2019
- python 2.7 gives syntax error
@daviskirk

This comment has been minimized.

Copy link
Contributor Author

daviskirk commented Jul 21, 2019

Ok, I think I'm ready for the next round. Thanks @mattbennett for your answers. Here a few comments:

Tests are now all passing for both gevent and eventlet 🎉. I'm not quite sure what to do with the "requirements" in setup.py now. I just left them as they are now (eventlet being required), but that's of course a bit wrong since it's not really required someone wanted to use it with gevent. Perhaps one could pip install nameko[eventlet] or pip install nameko[gevent], but that would mean that just installing nameko would not let you run anything (that doesn't seem to new user friendly).

I think it should be possible to specify the concurrency backend with the configuration file.

Did this, and seems to work well. However, the backend is still "set" once you import nameko.concurrency. I don't think it's possible to "proxy" the imports dynamically, and we also need to decide on the backend before we monkey patch.
For this reason I also added extra test environments (for gevent and eventlet), and skipped coverage for nameko.concurrency as one half would always not be covered otherwise.
There are a few extra things I had to change to run the tests for both (see the changes in .coveragerc and Makefile), and I'm not requiring full coverage for the gevent tests since some of the super eventlet specific regression tests do not make sense for gevent and of course websockets are skipped.

Ultimately I don't think Nameko has particularly complicated concurrency requirements, so simpler is probably better.

This is true, I tried to just use the simplest api that seemed to fit for both. Nameko itself would probably be fine with a lot less primitives, most of the api compatibility layer is actually needed to run the tests in both environments.

Tangential point -- I would love to see extensions for an ASGI server like https://gitlab.com/pgjones/hypercorn available for Nameko

I looked at this and think I get it, but this would probably be a separate pull right? I'm definitely not a pro in the ASGI space so I don't know how tricky it would be to actually build this.

Copy link
Member

mattbennett left a comment

This is so awesome @daviskirk. Gevent compatibility is tantalisingly close!

nameko/testing/pytest.py Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
nameko/cli/run.py Outdated Show resolved Hide resolved
nameko/concurrency/gevent_backend.py Show resolved Hide resolved
nameko/concurrency/gevent_backend.py Show resolved Hide resolved
nameko/utils/__init__.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
test/test_container.py Outdated Show resolved Hide resolved
@daviskirk

This comment has been minimized.

Copy link
Contributor Author

daviskirk commented Jul 28, 2019

Now that the discussion has exploded anyway, I'll add another question:

There's a nameko.utils.concurrency package. Should this be moved into the new nameko.concurrency package?

daviskirk added 3 commits Jul 29, 2019
- yield_thread is an alias for sleep (at least with regard to gevent and
  eventlet), but should be used to only yield the current green thread instead.
  This will make code using it clearer and more explicit compared to using
  sleep.
daviskirk added 6 commits Aug 4, 2019
- while the socket lib is not really used (only the error) it does not matter
  too much but this makes it harder to use the unpatched socket lib by
  accident.
@daviskirk daviskirk force-pushed the daviskirk:gevent branch from f222d1e to dbd60d6 Aug 5, 2019
@mattbennett

This comment has been minimized.

Copy link
Member

mattbennett commented Aug 11, 2019

There's a nameko.utils.concurrency package. Should this be moved into the new nameko.concurrency package?

Yes I think so. I would quite like everything in this package to go away eventually (it's only really used when starting and stopping extensions, which we already want to refactor with https://www.notion.so/namekopython/Refactor-sub-extension-management-380c0bfaea474e0f92b223cc7f33edae) but until then it makes sense to sit with the other concurrency primitives.

tox.ini Show resolved Hide resolved
@mattbennett

This comment has been minimized.

Copy link
Member

mattbennett commented Aug 11, 2019

This is amazing work @daviskirk. There are a couple of tiny things left, and then is this ready to go?

A sensible next step would probably be to make a release candidate available so it can be tested for a while.

@daviskirk daviskirk force-pushed the daviskirk:gevent branch 4 times, most recently from 8aafbe6 to a64163b Aug 18, 2019
* Simplify `monkey_patch_if_enforced` and rename
* Move `nameko.utils.concurrency` to `nameko.concurrency`
* Move and add tests accordingly
@daviskirk daviskirk force-pushed the daviskirk:gevent branch from a64163b to 85a9859 Aug 18, 2019
@daviskirk

This comment has been minimized.

Copy link
Contributor Author

daviskirk commented Aug 18, 2019

but until then it makes sense to sit with the other concurrency primitives

Moved it into the nameko.concurrency package.

A sensible next step would probably be to make a release candidate available so it can be tested for a while.

Sounds good 👍. I will be able to continue to test this on a few test and toy projects but it might be interesting to see who else would be interested in testing this to get a more robust test set.

@daviskirk daviskirk requested a review from mattbennett Sep 9, 2019
Copy link
Member

mattbennett left a comment

This is ❤️ ❤️ ❤️ Thank you @daviskirk!

I'm happy with these changes, and also have a project in mind to test a release candidate with.

I think we need to get v3 released (finally; @vlcinsky is pushing for this) and aim to have this out in 3.1. Rather than pushing a pre-release version to PyPI it might be more appropriate to install from this branch directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.