bpo-28053: Allow custom reducer when using multiprocessing by pierreglaser · Pull Request #15058 · python/cpython

pierreglaser · 2019-07-31T16:17:39Z

This PR is a follow up on #9959. It implements the suggestions I made here #9959 (comment).

I'm making it a new PR to get the CI working now and gather separate feedback.

pass the current context to Process objects
split AbstractPickler into AbstractPickler and AbstractUnpickler
clean everything up (still WIP)

https://bugs.python.org/issue28053

pierreglaser · 2019-08-01T21:05:12Z

You will notice that for the Process class, I chose to pass the reducer (which is the only bit of information we need from the context for now) to the Process class at construction instead of the Context

    def process(self, *args, **kwargs):
        return self._Process(*args, reducer=self.get_reducer(), **kwargs)

This comes from the fact that theProcess is actually relying on the default context to be properly defined:

cpython/Lib/multiprocessing/context.py

Lines 220 to 224 in 7397cda

    
           class Process(process.BaseProcess): 
        
               _start_method = None 
        
               @staticmethod 
        
               def _Popen(process_obj): 
        
                   return _default_context.get_context().Process._Popen(process_obj)

For the process class, passing the context object a Process bind to would require changing it to something like:

class Process(process.BaseProcess):
    _start_method = None
    @staticmethod
    def _Popen(process_obj):
          if self._ctx is not None:
              return self._ctx.get_context()._Process._Popen(process_obj)
          return _default_context.get_context()._Process._Popen(process_obj)

with _Process being a Process class specified (or not, in this case the default is Process) by the user. You can see the infinite recursion here as if _Process is not specified or set to Process, self._ctx.get_context()._Process._Popen is Process._Popen... To prevent this situation we would need to add a bunch of guards, and this ends up being not so trivial.

So implementation-wise, it is easier to simply pass the reducer in BaseContext.process, and not the whole context. Also, one noticeable consequence is that the reducer in Process is static, as it is not called using something like self._ctx.get_reducer() as it is done elsewhere.

But I am opened to suggestions.

Lib/multiprocessing/context.py

pppery · 2019-08-05T19:39:20Z

Lib/multiprocessing/context.py

The checks below aren't strict enough; they pass for a .register()ed subclass of Abstract<Reducer|Pickler|Unpickler>, but the code won't actually work with such a class due to depending on undocumented APIs.

I do not understand what you mean by "Undocumented API". This PR comes with documentation for the AbstractReducer, AbstractPickler, and AbstractUnpickler.

What I mean is that the documentation suggests that the below code should work:

class DummyReducer: def get_pickler_class(): return pickle.Pickler def get_unpickler_class(): return pickle.Unpickler AbstractReducer.register(DummyReducer) multiprocessing.set_reducer(DummyReducer())

However, in practice, it appears that running that code (and then doing something that uses reduction) will produce a (rather cryptic) error message in some cases. Either the documentation should be clear that virtual subclasses don't work and the code should raise an error when a virtual subclass is passed, or the code should be updated to handle virtual subclasses.

What I mean is that the documentation suggests that the below code should work

Not really: the get_pickler_class API documentation specifies that get_pickler_class must return anAbstractPickler subclass, but your DummyReducer (which itself, should be an AbstractReducer subclass) simply returns a Pickler, which is not an AbstractPickler subclass. Or maybe I misunderstood your message?

You didn't misunderstand anything, my example was accidentally bogus, and I misunderstood the code. The actual bug I was trying to point out appears not to exist with reducers, but instead with picklers.

class DummyReducer(AbstractReducer): def get_pickler_class(): return DummyPickler class DummyPickler(pickle.Pickler): pass AbstractPickler.register(DummyPickler) multiprocessing.set_reducer(DummyReducer())

This seemingly harmless code appears to have the effect of breaking the passing of all of the custom types that multiprocessing registers, in an entirely non-intuitive way, which I think should either be fixed or documented.

Lib/multiprocessing/forkserver.py

pierreglaser · 2019-08-07T18:17:07Z

@pitrou what could I do to ease the review process of this PR?

pitrou · 2019-08-07T18:29:46Z

@pierreglaser The first necessary condition is for me to find some time... sorry :-/

pierreglaser · 2019-08-07T18:36:15Z

@pitrou no worries :)

Lib/multiprocessing/context.py

pppery · 2019-08-07T19:28:57Z

Lib/multiprocessing/reduction.py

None of the attributes defined between here and __init__ appear to be used.

Same comment as below.

pppery · 2019-08-07T19:29:11Z

Lib/multiprocessing/reduction.py

This property does not appear to be used.

This is work taken from @pablogsal previous commits (see #9959). I assume there was a good reason for this, but he'll probably know better than me.

pppery · 2019-08-07T19:32:54Z

Lib/multiprocessing/reduction.py

This method (a) doesn't appear to be used and (b) doesn't necessarily work, because nothing requires the get_pickler_class to define a register method.

Regarding (b), this is stated in the documentation that get_pickler_class should return a subclass of an AbstractPickler, that itself (as a subclass of ForkingPickler), implements a register method. Regarding (a), same comment as above (this @pablogsal's work), but I'll think about whether this function can be removed.

pierreglaser · 2020-02-02T21:39:41Z

@pablogsal friendly ping: I am curious to know what are your thoughts on this.

terryjreedy · 2020-02-03T15:29:55Z

Misc/NEWS.d/next/Library/2018-12-09-19-20-12.bpo-28053.JiOeSY.rst

@@ -0,0 +1 @@
+Fix the implementation of custom reducers for in `multiprocessing`.


'for in' Either a word is missing or there is an extra word.

To me, it looks like "for" is the extra word. I'd personally go with:

Suggested change

Fix the implementation of custom reducers for in `multiprocessing`.

Fix the implementation of custom reducers in :mod:`multiprocessing`.

(Also adds the sphinx role :mod: to provide an inline link to the module. It's optional for Misc/NEWS.d, but we may as well include it with the typo fix. This makes it easier to move over to the "What's New" doc later.)

Also note that `multiprocessing` will cause make check to fail: it should either be :mod:`multiprocessing` (as suggested by @aeros) or ``multiprocessing``.

terryjreedy · 2020-02-03T16:02:24Z

(sorry, failed rebasing attempt)

Rebasing is, as evidenced by repeated failures like this, error prone. Besides the obvious noise, removing the spurious review requests does not unsubscribe people.

If you were just trying to update your branch to your current repository/fork master (once synchronized), rebasing is also not needed. After checking out the branch, git pull origin/master will, if there are no conflicts, pull, merge, and commit updates to the branch with a standard commit message.

MaxwellDupre

Run multiprocessing tests:
Ran 369 tests in 215.720s
OK (skipped=35)
Ran 39 tests in 22.709s
OK
Ran 369 tests in 85.400s
OK (skipped=39)
Ran 369 tests in 99.032s
OK (skipped=32)
Look ok to me.

gpshead · 2022-05-14T17:51:56Z

Doc/library/multiprocessing.rst

+   Gets the current reduction class in use by the module's primitive internals.
+
+For example, substituting the reducer class to use the :mod:`pickle` protocol
+version 2 to be able to communicate with a Python 2.x programs.::


Please change documentation and examples not to reference Python 2 as that long EOL at this point. Just say it in the abstract "communicate with processes using older Python versions" and offer some explanation as to how this is even possible by linking to the relevant APIs in multiprocessing that enable people to create such unexpected monsters. (see #67592 (comment))

Most multiprocessing users view it as a module with a single parent process controlling identical by definition Python version child processes that it launches and controls.

ezio-melotti · 2022-05-15T05:48:51Z

Doc/library/multiprocessing.rst

+.. method:: set_reducer(reduction)
+
+   Sets a reduction instance to be used for serialization and deserialization
+   by the module primitive internals. **reduction** must be an instance of a


Suggested change

by the module primitive internals. **reduction** must be an instance of a

by the module primitive internals. *reduction* must be an instance of a

Arguments should be marked in italic: https://devguide.python.org/documenting/#rest-inline-markup

ezio-melotti · 2022-05-15T05:50:37Z

Doc/library/multiprocessing.rst

+
+   .. method:: dump(obj)
+
+      Write a pickled representation of obj to the open file.


Suggested change

Write a pickled representation of obj to the open file.

Write a pickled representation of *obj* to the open file.

ezio-melotti · 2022-05-15T05:51:44Z

Doc/library/multiprocessing.rst

+   Base class providing access to custom ``Pickler`` and ``Unpickler``
+   classes to be used by ``multiprocessing`` when serializing objects.
+
+   .. method:: get_pickler_class():


Suggested change

.. method:: get_pickler_class():

.. method:: get_pickler_class()

ezio-melotti · 2022-05-15T05:51:55Z

Doc/library/multiprocessing.rst

+      This method must return an subclass of :class:`AbstractPickler` to be used
+      by ``multiprocessing`` when serializing objects.
+
+   .. method:: get_unpickler_class():


Suggested change

.. method:: get_unpickler_class():

.. method:: get_unpickler_class()

ezio-melotti · 2022-05-15T05:56:12Z

Misc/NEWS.d/next/Library/2018-12-09-19-20-12.bpo-28053.JiOeSY.rst

@@ -0,0 +1 @@
+Fix the implementation of custom reducers for in `multiprocessing`.


Also note that `multiprocessing` will cause make check to fail: it should either be :mod:`multiprocessing` (as suggested by @aeros) or ``multiprocessing``.

ezio-melotti · 2022-05-15T05:56:48Z

Doc/library/multiprocessing.rst

+.. currentmodule:: multiprocessing
+
+Several primitives of the :mod:`multiprocessing` module such as
+:class:`multiprocessing.Queue`, :class:`multiprocessing.connection.Listener` or
+:class:`multiprocessing.connection.Server` need to serialize and deserialize Python


Aren't either the currentmodule directive or the multiprocessing.*s in the roles redundant in this part?

pierreglaser requested review from 1st1, rhettinger and tiran as code owners July 31, 2019 16:17

the-knights-who-say-ni added the CLA signed label Jul 31, 2019

bedevere-bot added the awaiting review label Jul 31, 2019

pppery reviewed Aug 5, 2019

View reviewed changes

pppery reviewed Aug 7, 2019

View reviewed changes

Lib/multiprocessing/context.py Outdated Show resolved Hide resolved

pppery reviewed Aug 7, 2019

View reviewed changes

pierreglaser requested review from abalkin, asvetlov, berkerpeksag, brettcannon, encukou, ericsnowcurrently, ericvsmith, ethanfurman, ezio-melotti, gpshead, gvanrossum, ilevkivskyi, methane, ncoghlan, pablogsal, pganssle, skrah and terryjreedy as code owners February 2, 2020 20:30

pganssle requested review from 1st1, rhettinger and tiran and removed request for abalkin, ericsnowcurrently, ericvsmith, ethanfurman, ezio-melotti, gvanrossum, ilevkivskyi, ncoghlan, pablogsal, pganssle, rhettinger, skrah and terryjreedy February 2, 2020 20:51

update to use new timeout constants

2f117ec

terryjreedy reviewed Feb 3, 2020

View reviewed changes

tiran removed their request for review April 17, 2021 21:05

rhettinger removed their request for review May 3, 2022 06:31

MaxwellDupre approved these changes May 14, 2022

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 14, 2022

gpshead reviewed May 14, 2022

View reviewed changes

ezio-melotti reviewed May 15, 2022

View reviewed changes

applio mentioned this pull request Apr 10, 2022

parameterize what serialization is used in multiprocessing #72240

Open

vstinner added the topic-multiprocessing label Jun 6, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

		@@ -0,0 +1 @@
		Fix the implementation of custom reducers for in `multiprocessing`.

	by the module primitive internals. reduction must be an instance of a
	by the module primitive internals. reduction must be an instance of a


		.. method:: dump(obj)

		Write a pickled representation of obj to the open file.

	Write a pickled representation of obj to the open file.
	Write a pickled representation of obj to the open file.

	.. method:: get_pickler_class():
	.. method:: get_pickler_class()

	.. method:: get_unpickler_class():
	.. method:: get_unpickler_class()

Uh oh!

Conversation

pierreglaser commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierreglaser commented Aug 1, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pppery Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierreglaser Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pppery Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pierreglaser commented Aug 7, 2019

Uh oh!

pitrou commented Aug 7, 2019

Uh oh!

pierreglaser commented Aug 7, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierreglaser Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierreglaser Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierreglaser commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Feb 3, 2020

Uh oh!

MaxwellDupre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

pierreglaser commented Jul 31, 2019 •

edited

Loading

pppery Aug 7, 2019 •

edited

Loading

pierreglaser Aug 7, 2019 •

edited

Loading

pppery Aug 7, 2019 •

edited

Loading

pierreglaser Feb 2, 2020 •

edited

Loading

pierreglaser Feb 2, 2020 •

edited

Loading

pierreglaser commented Feb 2, 2020 •

edited

Loading