bpo-29851: Have importlib.reload() raise ImportError if the module's spec is not found by plusminushalf · Pull Request #972 · python/cpython

plusminushalf · 2017-04-03T11:19:16Z

if _bootstrap._find_spec() return None, an ImportError is raised
stating: module could not be found in the path specified

if _bootstrap._find_spec() return None, an ImportError is raised stating: module could not be found in the path specified

mention-bot · 2017-04-03T11:19:17Z

@garvitdelhi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @ericsnowcurrently and @ncoghlan to be potential reviewers.

DimitrisJim · 2017-04-03T14:15:13Z

Hi @garvitdelhi, I think this might also require tests to verify the change.

brettcannon

Thanks for the patch, but tests are needed for this.

brettcannon · 2017-04-03T20:54:27Z

        target = module
        spec = module.__spec__ = _bootstrap._find_spec(name, pkgpath, target)
+        if spec is None:
+            msg = "module {} not found in the path specified"


I would change this message to "spec not found for the module {!r}".

brettcannon · 2017-04-05T18:52:03Z

        spec = module.__spec__ = _bootstrap._find_spec(name, pkgpath, target)
+        if spec is None:
+            msg = "module {} not found in the path specified"
+            raise ImportError(msg.format(name), name=name)


As mentioned in the issue, this should probably be ModuleNotFoundError.

Raising ModuleNotFounderror instead of ImportError Changing error text

plusminushalf · 2017-04-06T13:10:15Z

Hi, I am a little busy and hence will by writing tests by the weekend.

plusminushalf · 2017-04-16T06:08:50Z

Hi, I tried writing tests but I am facing a problem.

I tried this:

moduleName = 'testmodule'
path = os.getcwd() + '/testmodule.py'
spec = importlib.util.spec_from_file_location(moduleName, path)
module_test = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module_test)
sys.modules[moduleName] = module_test
with self.assertRaises(ModuleNotFoundError):
    importlib.reload(sys.modules[moduleName])

The problem here is I have testmodule.py within the same directory i.e Lib/test/test_importlib/extension/ but since tests are run in a sandbox environment I don't have access to above directory. Also I can't load the file itself since it has a relative import 'abc' and I get an error saying: "ImportError: attempted relative import with no known parent package"

ncoghlan · 2017-04-17T03:54:05Z

Hi @garvitdelhi, for cases like this, it's worth looking through the existing test cases to see if there are some existing examples you can copy, as we tend to have lots of odd helpers in the test suite that let us do things that we expressly tell end users not to do (since we're deliberately inducing error states so we can check for expected exceptions).

For this PR, it turns out the most relevant examples are in tests.test_importlib.test.api: https://github.com/python/cpython/blob/master/Lib/test/test_importlib/test_api.py#L198

As with the examples there, the simplest way to get a module that can't be reloaded is to take a normal module and corrupt its metadata, as in something like:

>>> import types
>>> import importlib
>>> importlib.reload(types)
<module 'types' from '/usr/lib64/python3.5/types.py'>
>>> import sys
>>> types.__name__ = "not_a_module"
>>> types.__spec__ = None
>>> sys.modules[types.__name__] = types
>>> importlib.reload(types)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.5/importlib/__init__.py", line 166, in reload
    _bootstrap._exec(spec, module)
  File "<frozen importlib._bootstrap>", line 607, in _exec
AttributeError: 'NoneType' object has no attribute 'name'

berkerpeksag

Please add an entry to Misc/NEWS (also include "Patch by Your Name.") Thanks!

berkerpeksag · 2017-04-19T07:00:07Z

        spec = module.__spec__ = _bootstrap._find_spec(name, pkgpath, target)
+        if spec is None:
+            msg = "spec not found for the module {!r}"
+            raise ModuleNotFoundError(msg.format(name), name=name)


You can use an f-string here:

raise ModuleNotFoundError(f"spec not found for the module {name!r}", name=name)

berkerpeksag · 2017-04-19T07:02:36Z

            self.assertIs(ex_class, module.Example)

+    def test_reload_faliure(self):
+        '''Test that reload throws ModuleNotFounderror if reloading'''


"""...""" supports multiline comments so you can just do:

'''Test that reload throws ModuleNotFounderror if reloading a module whose spec cannot be found'''

I'll let Brett or Nick comment on the actual wording of docstring.

Actually the whole module is a mess when it comes to docstrings.

Use a # comment instead of a docstring, and it can take more than a single line. Also make sure to end it in a period.

berkerpeksag · 2017-04-19T07:43:30Z

+    def test_reload_faliure(self):
+        '''Test that reload throws ModuleNotFounderror if reloading'''
+        '''a module whose spec cannot be found'''
+        datetime.__name__ = 'destroyed_datetime'


We probably shouldn't directly mess with a stdlib module and sys.modules and use a helper like Nick said. Untested, but I think we need to add something like the following snippet to Lib/test/test_importlib/test_api.py:

with support.CleanImport('types'): module = self.init.import_module('types') module.__name__ = 'broken_types' module.__spec__ = None sys.modules[module.__name__] = module # TODO: Do we need to cleanup sys.modules? with self.assertRaises(ModuleNotFoundError): self.init.reload(module)

All the helpers for importlib-related tests can be found in https://github.com/python/cpython/blob/master/Lib/test/test_importlib/util.py. In this instance you probably want both uncache() and temp_module().

using a temperory modulue and destroying it's meta data and trying to reload to raise ModuleNotFoundError exception

plusminushalf · 2017-05-07T09:46:14Z

Hey sorry for the delay was busy in relocating, back to contributions now :)

brettcannon

A versionchanged note should be added to the docs on top of the relocation of the tests. And don't forget to add yourself to Misc/ACKS.

brettcannon · 2017-05-08T22:21:42Z

            importlib.reload(module)
            self.assertIs(ex_class, module.Example)

+    def test_reload_faliure(self):


Tests for reloading should be in test.test_importlib.test_api.

serhiy-storchaka · 2017-05-09T19:20:37Z

 Tools/ssl/win32
 .vs/
 .vscode/
+.idea/


This change doesn't look related.

will make a different issue for this removing this as of now

Note that you can use a global .gitignore file to ignore these IDE specific files. We've rejected a few PRs because the global .gitignore solution is the preferred way to do it.

Thank you for pointing this out almost forgot about global git ignores.

brettcannon

This is also going to require a versionchanged mention in the docs.

brettcannon · 2017-05-09T20:32:09Z

+        name = 'spam'
+        subname = 'ham'
+        with test_util.temp_module(name, pkg=True) as pkg_dir:
+            fullname, _ = test_util.submodule(name, subname, pkg_dir)


Is there a specific reason to test a submodule?

brettcannon · 2017-05-09T20:33:05Z

+            module = self.init.import_module(fullname)
+            module.__spec__ = None
+            module.__name__ = 'destroyed_spam'
+            sys.modules[module.__name__] = module


Would the test still work if you simply set module.__spec__ = None and left the name alone and didn't insert a new module? Currently you end up leaving a dead module in sys.modules under the name destroyed_spam.

Not changing the name won't work. If I don't modify the name, the method _find_spec inside importlib/_bootstrap.py tries to find spec from three finder from sys.meta_path: _frozen_importlib.BuiltinImporter, _frozen_importlib.FrozenImporter and _frozen_importlib_external.PathFinder respectively. But since I have not changed the name and the module by that filename still exists PathFinder can find the spec (reload the spec) from the file. Hence I do need to change the name and put it explicitly in sys.modules

Submodule isin't needed to reproduce the error

codecov · 2017-05-14T10:44:32Z

Codecov Report

Merging #972 into master will decrease coverage by 1.02%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
- Coverage    83.7%   82.67%   -1.03%     
==========================================
  Files        1371     1432      +61     
  Lines      346499   353357    +6858     
==========================================
+ Hits       290026   292131    +2105     
- Misses      56473    61226    +4753

Impacted Files	Coverage Δ
Lib/test/test_importlib/test_api.py	`98.73% <100%> (+0.02%)`	⬆️
Lib/importlib/__init__.py	`86.13% <50%> (+1.29%)`	⬆️
Lib/idlelib/idle_test/test_help_about.py	`8.21% <0%> (-88.84%)`	⬇️
Lib/idlelib/idle_test/mock_idle.py	`51.85% <0%> (-33.34%)`	⬇️
Lib/idlelib/textview.py	`17.85% <0%> (-8.07%)`	⬇️
Lib/idlelib/help_about.py	`17.7% <0%> (-6.21%)`	⬇️
Lib/sre_compile.py	`84.06% <0%> (-4.53%)`	⬇️
Lib/idlelib/idle_test/test_textview.py	`2.94% <0%> (-1.61%)`	⬇️
Lib/test/libregrtest/cmdline.py	`93.18% <0%> (-1.4%)`	⬇️
Lib/test/test_dict.py	`98.8% <0%> (-0.38%)`	⬇️
... and 116 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 211a392...4f528ef. Read the comment docs.

brettcannon · 2017-05-16T16:30:14Z

@garvitdelhi when you think you have addressed all the requested changes, could you remove the "[WIP]" part of the title that I added and mention me in a comment so this PR shows up as a GitHub notification for me?

Adding version change to the documentation. Uncaching the module so that we cleanup the mess made in sys.modules

plusminushalf · 2017-05-17T18:27:23Z

@brettcannon done :)

brettcannon · 2017-05-23T19:00:57Z

I made some of my own changes to the PR so I'm waiting to make sure I didn't break anything. Once Travis passes I will commit.

brettcannon · 2017-05-24T22:20:08Z

Thanks for PR, @garvitdelhi and being patient with the whole process!

VKTB · 2020-08-11T11:44:46Z

@garvitdelhi I know that this contribution is from a while ago but I am dynamically loading my own modules and get a ModuleNotFoundError: spec not found for the module 'a' error message when I call importlib.reload(sys.modules['a']).

I am loading the module using this logic::

import importlib.util

module_name = 'a'
path = os.getcwd() + '/a.py'
spec = importlib.util.spec_from_file_location(module_name, path)
module_a = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module_a)
sys.modules[module_name ] = module_a

When I inspect the module in debugging mode, I can see that it has its own ModuleSpec (see screenshot below), so I do not quite understand why it is raising the ImportError that was introduced as part of this PR. I also tried module_a.__spec__ = spec before adding the module to sys.modules just to be safe that spec is added to the module, but no luck. Any ideas?

brettcannon · 2020-08-11T16:36:07Z

@VKTB this isn't the best place to be asking this. If you're after help you can try asking on python-list, else if you think it's a bug then please open one at bugs.python.org.

Raising an ImportError if module not found

45e4eba

if _bootstrap._find_spec() return None, an ImportError is raised stating: module could not be found in the path specified

the-knights-who-say-ni added the CLA signed label Apr 3, 2017

serhiy-storchaka requested a review from brettcannon April 3, 2017 16:36

brettcannon changed the title ~~bpo-29851: Raising an ImportError if module not found~~ bpo-29851: Have importlib.reload() raise ImportError if the module's spec is not found Apr 3, 2017

brettcannon requested changes Apr 3, 2017

View reviewed changes

brettcannon self-assigned this Apr 5, 2017

brettcannon reviewed Apr 5, 2017

View reviewed changes

Code review changes

5220fae

Raising ModuleNotFounderror instead of ImportError Changing error text

Adding tests for importlib.reload

eae295b

berkerpeksag requested changes Apr 19, 2017

View reviewed changes

removing usage stdlib module

d180ebe

using a temperory modulue and destroying it's meta data and trying to reload to raise ModuleNotFoundError exception

brettcannon requested changes May 8, 2017

View reviewed changes

plusminushalf added 2 commits May 10, 2017 00:46

moving tests to test_importlib/test_api

641305e

Adding intellij prefences folder

5a128d0

serhiy-storchaka reviewed May 9, 2017

View reviewed changes

will make a different issue for this removing this as of now

2d9ee2f

brettcannon requested changes May 9, 2017

View reviewed changes

plusminushalf added 2 commits May 10, 2017 07:39

Merge branch 'master' of github.com:python/cpython into fix-bpo-29851

b88d64f

Removing laoding of a submodule

57bef8e

Submodule isin't needed to reproduce the error

brettcannon changed the title ~~bpo-29851: Have importlib.reload() raise ImportError if the module's spec is not found~~ [WIP] bpo-29851: Have importlib.reload() raise ImportError if the module's spec is not found May 16, 2017

Adding version changed and uncaching module

99da8e0

Adding version change to the documentation. Uncaching the module so that we cleanup the mess made in sys.modules

plusminushalf changed the title ~~[WIP] bpo-29851: Have importlib.reload() raise ImportError if the module's spec is not found~~ bpo-29851: Have importlib.reload() raise ImportError if the module's spec is not found May 17, 2017

Adding patch information in Misc/NEWS

5ac4905

brettcannon added 3 commits May 23, 2017 11:41

Tweak NEWS entry

30834a2

Explain what changed

a8a6aeb

Simplify the test

2e725cb

brettcannon approved these changes May 23, 2017

View reviewed changes

Fix the test

4f528ef

brettcannon merged commit 9498782 into python:master May 24, 2017

Uh oh!

Conversation

plusminushalf commented Apr 3, 2017

Uh oh!

mention-bot commented Apr 3, 2017

Uh oh!

DimitrisJim commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettcannon 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!

plusminushalf commented Apr 6, 2017

Uh oh!

plusminushalf commented Apr 16, 2017

Uh oh!

ncoghlan commented Apr 17, 2017

Uh oh!

berkerpeksag 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!

brettcannon Apr 28, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

plusminushalf commented May 7, 2017

Uh oh!

brettcannon 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!

brettcannon 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!

codecov Bot commented May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brettcannon commented May 16, 2017

Uh oh!

plusminushalf commented May 17, 2017

Uh oh!

brettcannon commented May 23, 2017

Uh oh!

brettcannon commented May 24, 2017

Uh oh!

VKTB commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettcannon commented Aug 11, 2020

Uh oh!

Reviewers

Assignees

DimitrisJim commented Apr 3, 2017 •

edited

Loading

brettcannon Apr 28, 2017 •

edited

Loading

codecov Bot commented May 14, 2017 •

edited

Loading

VKTB commented Aug 11, 2020 •

edited

Loading