msg255838 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2015-12-03 20:48 |
When you do a relative import, __package__ is used to help resolve it, but if it isn't defined we fall back on using __name__ and __path__. We should probably consider raising an ImportWarning if __package__ isn't defined so that some day we can consider cleaning up __import__ and its call signature to be a bit more sane and drop the `globals` argument. It would also help people catch errors where they went overboard deleting attributes off a module.
We should probably even extend it to start using __spec__.parent and then falling back to __package__, and only after both are found missing do we raise the ImportWarning about needing to use __name__ and __path__. That way __import__ can simply start taking in the spec of the calling module to do all of its work instead of having to pass all of globals().
|
msg258171 - (view) |
Author: Rose Ames (superluser) * |
Date: 2016-01-13 21:34 |
Patch with tests. Not sure if importlib.h should be included?
|
msg258172 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2016-01-13 21:36 |
Including importlib.h doesn't hurt.
|
msg258173 - (view) |
Author: Rose Ames (superluser) * |
Date: 2016-01-13 21:39 |
that's what I figured.
|
msg258178 - (view) |
Author: Rose Ames (superluser) * |
Date: 2016-01-13 23:00 |
Thanks for the quick review, new patch uploaded.
|
msg258329 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-01-15 21:33 |
New changeset 6908b2c9a404 by Brett Cannon in branch 'default':
Issue #25791: Raise an ImportWarning when __spec__ or __package__ are
https://hg.python.org/cpython/rev/6908b2c9a404
|
msg258330 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2016-01-15 21:33 |
Thanks for the patch, Rose! I did notice your review comments about the missing goto and the stacklevel and I simply added them myself.
|
msg258354 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2016-01-16 04:43 |
Favouring __spec__.parent over __package__ will break the documented workaround in PEP 366 for enabling explicit relative imports from __main__ even when a module is run directly instead of via -m:
if __name__ == "__main__" and __package__ is None:
__package__ = "expected.package.name"
It may be that workaround is something we *want* to break (as per http://bugs.python.org/issue21762#msg258332 ), but if so, it's at least worthy of a porting note in the What's New document.
|
msg258400 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2016-01-16 18:18 |
As I commented on another issue, I think I'm going to tweak the change to continue to prefer __package__, but raise ImportWarning when it doesn't match __spec__.parent. Once we do that for all attributes we can wait until Python 2.7 is done and then swap priority to __spec__ and raise a DeprecationWarning if __spec__ is missing. That way post-Python 2.7 we can move entirely to a spec-based import system.
|
msg258842 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-01-22 23:27 |
New changeset 219c44fe8968 by Brett Cannon in branch 'default':
Issue #25791: Warn when __package__ != __spec__.parent.
https://hg.python.org/cpython/rev/219c44fe8968
|
msg294632 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-05-28 08:32 |
This new warning is introducing difficulties for some Cython-compiled modules: https://github.com/cython/cython/issues/1720
|
msg294652 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2017-05-28 18:03 |
Is Cython not defining actual module objects or working around types.ModuleType? I'm just trying to figure out how a Cython module is ending up in a place where the attributes that are set by PyModule_NewObject() aren't there (https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Objects/moduleobject.c#L80).
|
msg294653 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-05-28 18:10 |
AFAICT, Cython simply calls PyModule_Create() on Python 3.
|
msg294656 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2017-05-28 19:18 |
OK, so the warning is triggered if __package is None or __spec__ is None (https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Lib/importlib/_bootstrap.py#L1038). That's defined in _calc___package__() which is only called if index != 0 when calling __import__() (https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Lib/importlib/_bootstrap.py#L1062 or https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Python/import.c#L1520).
So the question becomes how is Cython importing modules? This warning should only be triggered if you're attempting a relative import from within a package (and thus have a level > 0). Based on Antoine's Cython issue, since something like multidict isn't a package it would suggest either Cython is setting the level > 0 when it doesn't mean to or there's a bug somewhere with level == 0 and yet Python is still trying to calculate the parent package for no reason.
|
msg294657 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-05-28 19:25 |
multidict is a package, the Cython module is multidict._multidict.
Cython translated "import sys" into this:
__pyx_t_1 = __Pyx_Import(__pyx_n_s_sys, 0, -1); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 5, __pyx_L1_error)
And __Pyx_Import is this:
https://gist.github.com/pitrou/9a0d94aba3495ce3eb8630d4797d67bb
Note that __Pyx_MODULE_NAME is defined thusly:
#define __Pyx_MODULE_NAME "multidict._multidict"
So perhaps Cython is indeed attempting a PyImport_ImportModuleLevelObject()
call with a level of 1 for "import sys" before falling back on a level of -1...
Why I don't know.
(also since __Pyx_MODULE_NAME seems known, Cython could actually define __package__)
|
msg294668 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2017-05-29 04:10 |
Is it possible Cython is still supporting pre-PEP-328 style implicit relative imports, even in Python 2.7+?
|
msg294677 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-05-29 07:10 |
> Is it possible Cython is still supporting pre-PEP-328 style implicit relative imports, even in Python 2.7+?
That might be the case. I can't find anything in the Cython docs. Stefan, could you shed a light?
|
msg294678 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-05-29 07:14 |
Ok, I did an experiment. I added "from __future__ import absolute_import" at the top of _multidict.c, and after a recompile the warning went away.
What changed was that the following line:
__pyx_t_1 = __Pyx_Import(__pyx_n_s_sys, 0, -1); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 5, __pyx_L1_error)
was replaced with:
__pyx_t_1 = __Pyx_Import(__pyx_n_s_sys, 0, 0); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 3, __pyx_L1_error)
Ignore the change from "3" to "5", which is the line number for the traceback. The relevant change is the "level" argument to __Pyx_Import() which went from -1 to 0.
|
msg296951 - (view) |
Author: Stefan Behnel (scoder) * |
Date: 2017-06-26 21:50 |
Sorry for not responding, missed the message, it seems.
Cython has to support old-style relative imports also in Py3 because that's how the user code was originally written, using Py2-style syntax and semantics. Most Cython code has not been converted to Py3 syntax/semantics, but still needs to work in Py3.
From a user perspective, it's best to switch on "__future__.absolute_import" and use explicit relative imports (or write Py3 code), because it reduces the overhead at import time.
I'm hoping to look into multi-step module initialisation this summer. As Nick noted, this might also help with this issue.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:24 | admin | set | github: 69977 |
2017-06-26 21:50:10 | scoder | set | messages:
+ msg296951 |
2017-05-29 07:14:33 | pitrou | set | messages:
+ msg294678 |
2017-05-29 07:10:17 | pitrou | set | messages:
+ msg294677 |
2017-05-29 04:10:22 | ncoghlan | set | messages:
+ msg294668 |
2017-05-28 19:25:15 | pitrou | set | messages:
+ msg294657 |
2017-05-28 19:18:17 | brett.cannon | set | messages:
+ msg294656 |
2017-05-28 18:10:24 | pitrou | set | nosy:
+ scoder messages:
+ msg294653
|
2017-05-28 18:03:30 | brett.cannon | set | messages:
+ msg294652 |
2017-05-28 08:32:06 | pitrou | set | nosy:
+ pitrou messages:
+ msg294632
|
2016-01-22 23:28:43 | brett.cannon | link | issue21762 dependencies |
2016-01-22 23:27:30 | brett.cannon | set | status: open -> closed |
2016-01-22 23:27:05 | python-dev | set | messages:
+ msg258842 |
2016-01-16 18:18:45 | brett.cannon | set | status: closed -> open
messages:
+ msg258400 |
2016-01-16 04:43:41 | ncoghlan | set | messages:
+ msg258354 |
2016-01-15 21:33:44 | brett.cannon | set | status: open -> closed resolution: fixed messages:
+ msg258330
stage: test needed -> resolved |
2016-01-15 21:33:11 | python-dev | set | nosy:
+ python-dev messages:
+ msg258329
|
2016-01-14 00:20:29 | brett.cannon | set | assignee: brett.cannon |
2016-01-13 23:00:29 | superluser | set | files:
+ issue25791_2.patch
messages:
+ msg258178 |
2016-01-13 21:39:28 | superluser | set | messages:
+ msg258173 |
2016-01-13 21:36:08 | brett.cannon | set | messages:
+ msg258172 |
2016-01-13 21:34:49 | superluser | set | messages:
+ msg258171 |
2016-01-13 21:32:36 | superluser | set | files:
+ issue25791.patch keywords:
+ patch |
2015-12-05 22:27:56 | superluser | set | nosy:
+ superluser
|
2015-12-03 20:48:12 | brett.cannon | create | |