Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
bpo-29851: Have importlib.reload() raise ImportError if the module's spec is not found #972
Conversation
if _bootstrap._find_spec() return None, an ImportError is raised stating: module could not be found in the path specified
|
@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. |
|
Hi @garvitdelhi, I think this might also require tests to verify the change. |
brettcannon
changed the title
|
Thanks for the patch, but tests are needed for this. |
| @@ -164,6 +164,9 @@ def reload(module): | |||
| pkgpath = None | |||
| target = module | |||
| spec = module.__spec__ = _bootstrap._find_spec(name, pkgpath, target) | |||
| if spec is None: | |||
| msg = "module {} not found in the path specified" | |||
| @@ -164,6 +164,9 @@ def reload(module): | |||
| pkgpath = None | |||
| target = module | |||
| 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) | |||
brettcannon
Apr 5, 2017
Member
As mentioned in the issue, this should probably be ModuleNotFoundError.
Raising ModuleNotFounderror instead of ImportError Changing error text
|
Hi, I am a little busy and hence will by writing tests by the weekend. |
|
Hi, I tried writing tests but I am facing a problem. I tried this: 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" |
|
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 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: |
|
Please add an entry to |
| @@ -164,6 +164,9 @@ def reload(module): | |||
| pkgpath = None | |||
| target = module | |||
| 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) | |||
berkerpeksag
Apr 19, 2017
Member
You can use an f-string here:
raise ModuleNotFoundError(f"spec not found for the module {name!r}", name=name)| @@ -145,6 +146,15 @@ def test_reload(self): | |||
| importlib.reload(module) | |||
| self.assertIs(ex_class, module.Example) | |||
|
|
|||
| def test_reload_faliure(self): | |||
| '''Test that reload throws ModuleNotFounderror if reloading''' | |||
berkerpeksag
Apr 19, 2017
Member
"""...""" 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.
brettcannon
Apr 28, 2017
•
Member
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.
| def test_reload_faliure(self): | ||
| '''Test that reload throws ModuleNotFounderror if reloading''' | ||
| '''a module whose spec cannot be found''' | ||
| datetime.__name__ = 'destroyed_datetime' |
berkerpeksag
Apr 19, 2017
Member
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)
brettcannon
May 1, 2017
Member
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
|
Hey sorry for the delay was busy in relocating, back to contributions now :) |
|
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. |
| @@ -145,6 +145,21 @@ def test_reload(self): | |||
| importlib.reload(module) | |||
| self.assertIs(ex_class, module.Example) | |||
|
|
|||
| def test_reload_faliure(self): | |||
| @@ -99,3 +99,4 @@ Tools/ssl/amd64 | |||
| Tools/ssl/win32 | |||
| .vs/ | |||
| .vscode/ | |||
| .idea/ | |||
garvitdelhi
May 9, 2017
Author
Contributor
will make a different issue for this removing this as of now
berkerpeksag
May 10, 2017
Member
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.
garvitdelhi
May 10, 2017
Author
Contributor
Thank you for pointing this out almost forgot about global git ignores.
|
This is also going to require a versionchanged mention in the docs. |
| name = 'spam' | ||
| subname = 'ham' | ||
| with test_util.temp_module(name, pkg=True) as pkg_dir: | ||
| fullname, _ = test_util.submodule(name, subname, pkg_dir) |
| module = self.init.import_module(fullname) | ||
| module.__spec__ = None | ||
| module.__name__ = 'destroyed_spam' | ||
| sys.modules[module.__name__] = module |
brettcannon
May 9, 2017
Member
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.
garvitdelhi
May 14, 2017
Author
Contributor
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 Report
@@ 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
Continue to review full report at Codecov.
|
brettcannon
changed the title
|
@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
garvitdelhi
changed the title
|
@brettcannon done :) |
|
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. |
|
Thanks for PR, @garvitdelhi and being patient with the whole process! |
…spec is not found (pythonGH-972)
|
@garvitdelhi I know that this contribution is from a while ago but I am dynamically loading my own modules and get 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_aWhen I inspect the module in debugging mode, I can see that it has its own
|
|
@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. |
if _bootstrap._find_spec() return None, an ImportError is raised
stating: module could not be found in the path specified