This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: PyImport_Import redundant calls to find module
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.2, Python 3.3, Python 3.4
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Rob Bairos, brett.cannon, eric.snow
Priority: normal Keywords:

Created on 2013-08-22 19:00 by Rob Bairos, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (6)
msg195915 - (view) Author: Rob Bairos (Rob Bairos) Date: 2013-08-22 19:00
Why does PyImport_Import (import.c) call __import__ then immediately discard the result, and then look for the module again in the module dictionary?  

It will return the same value in both cases no?

Ive included the relevant portion of the code below.
Its breaking an embedded application we've built where a hierarchy of modules are maintained outside the sys.modules dictionary.


 /* Call the __import__ function with the proper argument list
    Always use absolute import here.
    Calling for side-effect of import. */
 r = PyObject_CallFunction(import, "OOOOi", module_name, globals,
                           globals, silly_list, 0, NULL);
 if (r == NULL)
     goto err;

//-------------------------- WHY IS THIS SECTION NEEDED? --------
 Py_DECREF(r);
 modules = PyImport_GetModuleDict();
 r = PyDict_GetItem(modules, module_name);
 if (r != NULL)
//---------------------------------------------------------------
     Py_INCREF(r);
msg195936 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-08-23 00:55
My guess is that, in part, it's left-over code from before the switch to importlib.  However, it relies on builtins.__import__(), which isn't guaranteed to work correctly, hence one last sync with sys.modules.  I'm not convinced that justifies not simply returning the result of calling __import__().

Brett, am I missing something here?
msg195964 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-08-23 12:30
Because that is how it has always been: http://hg.python.org/cpython/file/b9b521efeba3/Python/import.c#l3164 . It could be changed but someone out there is relying on those semantics and it's a minor thing to leave in so I don't want to mess with it.
msg195968 - (view) Author: Rob Bairos (Rob Bairos) Date: 2013-08-23 13:36
However, if it fails __import()__  it doesn't get to the sys.modules[] call anyways.

The only case affected by this are:

  PASS the __import()__ call, 
  then FAIL the sys.modules[] lookup afterwards.

Why will that effect anything currently out there?

As it stands now, it suppresses development of some types of dynamic hierarchies of modules such as we've implemented, that don't register themselves in sys.modules[]
msg195978 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-08-23 14:56
If I were to change this code in any way it would be to stop passing in a dummy value for fromlist, not stop pulling from sys.modules. The official idiom for directly calling __import__() is::

  __import__(module_name)
  return sys.modules[module_name]

It just so happens PyImport_Import() is old enough to be using both the old idiom of a bogus fromlist item *and* been updated at some point to use the new idiom.

I would also strongly discourage you from replacing __import__. More and more code is using importlib and various importers to do things and so it might have unforeseen consequences. importlib.import_module() also relies on importlib.__import__, not builtins.__import__ so as more code switches to that function for dynamic importing it will lead to more imports not using an overridden __import__.
msg196248 - (view) Author: Rob Bairos (Rob Bairos) Date: 2013-08-26 21:00
Okay, thanks for looking into it.
Cheers
History
Date User Action Args
2022-04-11 14:57:49adminsetgithub: 63012
2013-08-26 21:00:16Rob Bairossetmessages: + msg196248
2013-08-23 14:56:07brett.cannonsetmessages: + msg195978
2013-08-23 13:36:02Rob Bairossetmessages: + msg195968
2013-08-23 12:30:24brett.cannonsetstatus: open -> closed
resolution: rejected
messages: + msg195964
2013-08-23 00:55:20eric.snowsetnosy: + brett.cannon, eric.snow
messages: + msg195936
2013-08-22 19:09:21Rob Bairossettype: behavior
2013-08-22 19:00:11Rob Bairoscreate