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

WIP of Gevent-compatible MP #1158

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@arcivanov
Contributor

arcivanov commented Mar 30, 2018

fixes #993

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Mar 30, 2018

Work in progress - DO NOT MERGE

@arcivanov arcivanov force-pushed the arcivanov:issue_993 branch 2 times, most recently from 78cca9b to d193592 Apr 1, 2018

def warn(message):
_queue_warning(message, _warnings)
gevent_module = getattr(__import__('gevent.' + name), name)
module_name = getattr(gevent_module, '__target__', name)
module = __import__(module_name)
module = import_module(module_name)

This comment has been minimized.

@arcivanov

arcivanov Apr 1, 2018

Contributor

For multiprocessing.xxx

olditem = getattr(module, attr, _NONE)
if olditem is not _NONE:
saved.setdefault(module.__name__, {}).setdefault(attr, olditem)
setattr(module, attr, newitem)
if rewrite_module:

This comment has been minimized.

@arcivanov

arcivanov Apr 1, 2018

Contributor

For proper pickling/reduction of connections, queues, synchronization primitives etc into child processes

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Apr 1, 2018

@jamadden Could you please provide preliminary comments on the structure of the patch, such as monkeypatching strategies, file naming and location etc?

@jamadden

This comment has been minimized.

Member

jamadden commented Apr 1, 2018

My initial thought is that this would be a good candidate for the "plugin" monkey-patching that's been brought up a few times earlier. Distributing this as a separate project (e.g., 'gevent_multiprocessing') at least initially, would free it from the gevent release cycle, giving it the easy ability to iterate quickly to fix early bugs, free it from having to support all the OS and Python combos gevent core does (e.g., Windows is an issue in this right now), and so on.

I was hoping to get gevent 1.3b1 out this weekend, since Python 3.7b3 dropped, but that's been pushed back to Monday or Tuesday by circumstances. Adding a simple plugin system won't take me too long. I'm roughly thinking setuptools entry points, adjust patch_all to take **kwargs, which get passed to the discovered entry points, document the patching functions.

What do you think about that?

(My replies are likely to be delayed this weekend.)

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Apr 1, 2018

Are you thinking extra-based plugin? Or a separate project altogether?

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Apr 1, 2018

I think plugin system should be fine, although a counter-argument to that is that if Gevent monkey-patch tries to make Python-proper cooperative, both MP and asyncio are part of the core specifications and therefore putting them in separate plugins would be counter-intuitive.

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Apr 1, 2018

So another very important issue I've discovered just now that this construct becomes necessary in cases where we don't want the child process (e.g. ForkServer) to accidentally load gevent (MP starts the child process with a main module of a parent, which results in gevent monkey-patch becoming dragged into unwantingly):

from multiprocessing.process import current_process

from gevent import monkey

if not getattr(current_process(), "_inheriting", False):
    monkey.patch_all()

If multiprocessing is a gevent optional module then such conditions should be pluggable at the top of patch_all to make sure patching can be disabled.

@arcivanov arcivanov force-pushed the arcivanov:issue_993 branch 2 times, most recently from 2cba6af to 4d365f5 Apr 2, 2018

@arcivanov arcivanov force-pushed the arcivanov:issue_993 branch from 4d365f5 to dc4f605 Apr 2, 2018

@jamadden

This comment has been minimized.

Member

jamadden commented Apr 2, 2018

a separate project altogether?

That's definitely the way I'm leaning, at least initially, for the pragmatic reasons I mentioned. The standard library often takes the approach of saying "but it on PyPI, see what the popularity and maintenance concerns are like before making it the responsibility of the core" and I can see that being a reasonable tactic here.

both MP and asyncio are part of the core specifications and therefore putting them in separate plugins would be counter-intuitive.

That's certainly a valid point. The counter-counter argument is that gevent is "a coroutine-based Python networking library" (documentation) whose primary objective is to enable network based servers (me). "greening-all-the-things" is a (useful) secondary goal, and often a necessary one. In my experience, multiprocessing is not very commonly used by networking programs and servers; servers tend to use an external task queue of some sort. It looks like it takes a large surface area and moderately invasive patching (e.g., __module__ swizzling) to make multiprocessing green. So also from those two points a separate library (again, at least initially) seems reasonable to me.

We would of course want to link to this library from the documentation, and since the failure case of patch_threading + multiprocessing is so bad (hung process) we might want to look into a way to issue a runtime warning as part of core if we can detect that scenario.

(asyncio is a separate ball of wax. 😄)

such conditions should be pluggable at the top of patch_all to make sure patching can be disabled.

Reasonable. My current thoughts are to combine setuptools entry points and the new gevent.events module to "broadcast" important steps in the patching process to both plugins and event subscribers. If any receiver raised a special exception, the process would stop.

Roughly:

def patch_module(module):
   try:
      broadcast_gevent_will_patch_module(module)
   except DoNotPatch:
      return False
   ...
   broadcast_gevent_did_patch_module(module)

def patch_all(**kwargs):
   try:
     broadcast_gevent_will_patch_all(kwargs)
   except DoNotPatch:
     return False
   
   ... 

   execute_patch_plugins(kwargs)
   broadcast_gevent_did_patch_all(kwargs)
@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Apr 2, 2018

Let me process this a bit.

Meanwhile, there is a whole bunch of unrelated failures in 3.5 and 3.4:

  • GEVENT_DEBUG=debug GEVENT_RESOLVER=thread GEVENT_RESOLVER_NAMESERVERS=8.8.8.8 /home/travis/.runtimes/snakepit/python3.5.4 -u test__local.py
  • GEVENT_DEBUG=error GEVENT_RESOLVER=thread GEVENT_RESOLVER_NAMESERVERS=8.8.8.8 /home/travis/.runtimes/snakepit/python3.4.7 -u -W ignore -mgreentest.monkey_test test_threading.py
  • GEVENT_DEBUG=debug GEVENT_RESOLVER=thread GEVENT_RESOLVER_NAMESERVERS=8.8.8.8 /home/travis/.runtimes/snakepit/python3.4.7 -u test__local.py
@jamadden

This comment has been minimized.

Member

jamadden commented Apr 3, 2018

While you process, I’d like to clarify that I absolutely do feel this is a significant piece of work and I do believe it of value to the gevent community. I am also hopeful it can serve as a pilot project in a way to share parts of development and maintenance throughout the community.

@jamadden

This comment has been minimized.

Member

jamadden commented Apr 3, 2018

Meanwhile, there is a whole bunch of unrelated failures in 3.5 and 3.4:

Hmm. There are a few timing related failures I have learned can often be solved by simply rerunning the affected scenario in isolation, but spot checking a few of those didn’t look the same. So I don’t know what’s up. I would suggest (and I know this is weak tea) running each scenario individually again.

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Apr 3, 2018

There are a few timing related failures

It seems that the failures are ample even in master.

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Apr 3, 2018

While you process,

I didn't take it as if you didn't. 😄 Thanks!

@jamadden

This comment has been minimized.

Member

jamadden commented Apr 3, 2018

It seems that the failures are ample even in master.

Hmm, I don't see what you're talking about. The most recent master builds on travis are green as was Appveyor for Python 3. I don't reproduce any failures with 3.5 locally on master either.

[Moved to #1167]

jamadden added a commit that referenced this pull request Apr 3, 2018

jamadden added a commit that referenced this pull request Apr 3, 2018

jamadden added a commit that referenced this pull request Apr 3, 2018

@jaddison

This comment has been minimized.

jaddison commented Aug 27, 2018

This would be amazing to see completed - what is missing/outstanding for this to be made available?

@jamadden

This comment has been minimized.

Member

jamadden commented Aug 27, 2018

gevent 1.3 should contain all the necessary hooks to implement this as the suggested separate library.

@arcivanov

This comment has been minimized.

Contributor

arcivanov commented Aug 29, 2018

Sorry guys, I'll get back to it soon.

@Bogdanp Bogdanp referenced this pull request Oct 21, 2018

Closed

Enhancements+windows compat #126

0 of 3 tasks complete

@Bogdanp Bogdanp referenced this pull request Oct 21, 2018

Closed

Windows compatibility #119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment