Skip to content

Stop using imp.find_module() in multiprocessing #61516

Closed
Closed
Stop using imp.find_module() in multiprocessing#61516
Assignees
brettcannon
Labels
stdlibStandard Library Python modules in the Lib/ directory

Description

@brettcannon
brettcannon
opened on Feb 27, 2013
BPO 17314
Nosy @brettcannon
Files
  • remove_imp.find_module.diff: remove use of imp.find_module() from multiprocessing/forking.py
  • mp-importlib.diff
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/brettcannon'
closed_at = <Date 2013-06-07.15:46:09.012>
created_at = <Date 2013-02-27.19:56:34.952>
labels = ['library']
title = 'Stop using imp.find_module() in multiprocessing'
updated_at = <Date 2013-06-07.15:46:09.011>
user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2013-06-07.15:46:09.011>
actor = 'brett.cannon'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2013-06-07.15:46:09.012>
closer = 'brett.cannon'
components = ['Library (Lib)']
creation = <Date 2013-02-27.19:56:34.952>
creator = 'brett.cannon'
dependencies = []
files = ['29270', '29274']
hgrepos = []
issue_num = 17314
keywords = ['patch']
message_count = 9.0
messages = ['183177', '183191', '183194', '189967', '189991', '190031', '190036', '190089', '190758']
nosy_count = 3.0
nosy_names = ['brett.cannon', 'python-dev', 'sbt']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue17314'
versions = ['Python 3.4']

    Activity

    1. brettcannon commented on Feb 27, 2013

      @brettcannon
      brettcannon
      on Feb 27, 2013
      MemberAuthor

      I'm trying to remove all uses of imp.find_module()/load_module() and multiprocessing seems to have a single use of both purely for (re)loading a module. The attached patch moves over to importlib.find_loader() and subsequent load_module() call to match the semantics of imp.find_module()/load_module(). If a guaranteed reload is not necessary then importlib.import_module() is a single-line change.

      I ran the test suite, but there don't seem to be any explicit tests for this chunk of code (or am I missing something?).

    2. brettcannon
      assigned
      sbt
      on Feb 27, 2013
    3. brettcannon
      added
      stdlibStandard Library Python modules in the Lib/ directory
      on Feb 27, 2013

    4. sbt commented on Feb 27, 2013

      @sbt
      sbtmannequin
      on Feb 27, 2013
      Mannequin

      I think this change will potentially make the main module get imported twice under different names when we transfer pickled data between processes. The current code (which is rather a mess) goes out of its way to avoid that.

      Basically the main process makes sys.modules['__mp_main__'] an alias for the main module, and other process import the parent's main module with __name__ == '__mp_main__' and make sys.modules['__main__'] an alias for that. This means that any functions/classes defined in the main module (from whatever process) will have

      obj.__module__ in {'__main__', '__mp_main__'}

      Unpickling such an object will succeed in any process without reimporting the main module.

      Attached is an alternative patch which is more like the original code and seems to work. (Maybe modifying loader.name is an abuse of the API.)

    5. brettcannon commented on Feb 28, 2013

      @brettcannon
      brettcannon
      on Feb 28, 2013
      MemberAuthor

      It is an abuse since I didn't design that part of the API to function that way, but it's cool that it just happens to. =)

      I do see your use-case and it is legitimate, although extremely rare and narrow. Let me think about whether I want to add specific support either through your approach, Richard, or if I want to decouple the setting of module attributes so that it is more along the lines of::

        main_module = imp.new_module('__mp_main__')
  loader.set_attributes(main_module)  # BRAND-NEW; maybe private to the stdlib?
  main_module.__name__ = '__mp_main__'
  code_object = loader.get_code(main_name)
  sys.modules['__main__'] = sys.modules['__mp_main__'] = main_module  # OLD
  exec(code_object, main_module.__dict__)

      I'm currently leaning towards the latter option since it's an annoying bit to get right and it doesn't hurt anything to expose.

    6. brettcannon
      assigned
      brettcannon
      and unassigned
      sbt
      on Feb 28, 2013

    7. brettcannon commented on May 25, 2013

      @brettcannon
      brettcannon
      on May 25, 2013
      MemberAuthor

      So I think I have come up with a way to expose a new method that makes this use-case doable and in a sane manner. Richard, let me know what you think so that I know that this makes sense before I commit myself to the new method (init_module_attrs())::

      --- a/Lib/multiprocessing/forking.py	Fri May 24 13:51:21 2013 +0200
+++ b/Lib/multiprocessing/forking.py	Fri May 24 08:06:17 2013 -0400
@@ -449,7 +449,7 @@
         elif main_name != 'ipython':
             # Main modules not actually called __main__.py may
             # contain additional code that should still be executed
-            import imp
+            import importlib
 
             if main_path is None:
                 dirs = None
@@ -460,16 +460,17 @@
 
             assert main_name not in sys.modules, main_name
             sys.modules.pop('__mp_main__', None)
-            file, path_name, etc = imp.find_module(main_name, dirs)
+            # We should not try to load __main__
+            # since that would execute 'if __name__ == "__main__"'
+            # clauses, potentially causing a psuedo fork bomb.
+            loader = importlib.find_loader(main_name, path=dirs)
+            main_module = imp.new_module(main_name)
             try:
-                # We should not do 'imp.load_module("__main__", ...)'
-                # since that would execute 'if __name__ == "__main__"'
-                # clauses, potentially causing a psuedo fork bomb.
-                main_module = imp.load_module(
-                    '__mp_main__', file, path_name, etc
-                    )
-            finally:
-                if file:
-                    file.close()
+                loader.init_module_attrs(main_module)
+            except AttributeError:
+                pass
+            main_module.__name__ = '__mp_main__'
+            code = loader.get_code(main_name)
+            exec(code, main_module.__dict__)
 
             sys.modules['__main__'] = sys.modules['__mp_main__'] = main_module

    8. sbt commented on May 25, 2013

      @sbt
      sbtmannequin
      on May 25, 2013
      Mannequin

      Looks good to me.

      (Any particular reason for ignoring AttributeError?)

    9. brettcannon commented on May 25, 2013

      @brettcannon
      brettcannon
      on May 25, 2013
      MemberAuthor

      Catching the AttributeError is in case a loader is used that doesn't define init_module_attrs(). Since it will be new to Python 3.4 I can't guarantee that it will exist (in case someone doesn't subclass the proper ABC).

    10. sbt commented on May 25, 2013

      @sbt
      sbtmannequin
      on May 25, 2013
      Mannequin

      The unit tests pass with the patch already (if we don't delete the "import imp" line).

      What attributes will be set by init_module_attrs()?

    11. brettcannon commented on May 26, 2013

      @brettcannon
      brettcannon
      on May 26, 2013
      MemberAuthor

      In the common case of SourceLoader it will set __loader__, __package__, __file__, and __cached__.

    12. python-dev commented on Jun 7, 2013

      @python-dev
      python-devmannequin
      on Jun 7, 2013
      Mannequin

      New changeset 97adaa820353 by Brett Cannon in branch 'default':
      Issue bpo-17314: Stop using imp in multiprocessing.forking and move over
      http://hg.python.org/cpython/rev/97adaa820353

    13. brettcannon
      closed this as completedon Jun 7, 2013
    14. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    stdlibStandard Library Python modules in the Lib/ directory

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions