classification
Title: subprocess module import hooks breaks back compatibility
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, davin, dw, eric.snow, miss-islington, ncoghlan, ned.deily
Priority: normal Keywords: patch, patch, patch

Created on 2018-12-13 23:14 by dw, last changed 2019-01-18 01:12 by ned.deily.

Pull Requests
URL Status Linked Edit
PR 11540 merged ncoghlan, 2019-01-13 07:23
PR 11540 merged ncoghlan, 2019-01-13 07:23
PR 11540 merged ncoghlan, 2019-01-13 07:23
PR 11587 merged miss-islington, 2019-01-17 10:42
PR 11587 merged miss-islington, 2019-01-17 10:42
PR 11587 merged miss-islington, 2019-01-17 10:42
PR 11588 merged miss-islington, 2019-01-17 10:42
PR 11588 merged miss-islington, 2019-01-17 10:42
PR 11588 merged miss-islington, 2019-01-17 10:42
Messages (12)
msg331774 - (view) Author: David Wilson (dw) * Date: 2018-12-13 23:14
The subprocess package since 880d42a3b24 / September 2018 has begun using this idiom:

try:
    import _foo
except ModuleNotFoundError:
    bar

However, ModuleNotFoundError may not be thrown by older import hook implementations, since that subclass was only introduced in Python 3.6 -- and so the test above will fail. PEP-302 continues to document ImportError as the appropriate exception that should be raised.

https://mitogen.readthedocs.io/en/stable/ is one such import hook that lazily loads packages over the network when they aren't available locally. Current Python subprocess master breaks with Mitogen because when it discovers the master cannot satisfy the import, it throws ImportError.

The new exception subtype was introduced in https://bugs.python.org/issue15767 , however very little in the way of rationale was included, and so it's unclear to me what need the new subtype is addressing, whether this is a problem with the subprocess module or the subtype as a whole, or indeed whether any of this should be considered a bug.

It seems clear that some kind of regression is in the process of occurring during a minor release, and it also seems clear the new subtype will potentially spawn a whole variety of similar new regressions.

I will be updating Mitogen to throw the new subtype if it is available, but I felt it was important to report the regression to see what others think.
msg331775 - (view) Author: David Wilson (dw) * Date: 2018-12-13 23:15
Corrected GitHub link for the commit: https://github.com/python/cpython/commit/880d42a3b24
msg331776 - (view) Author: David Wilson (dw) * Date: 2018-12-14 01:10
Having considered this for a few hours, it seems the following is clear:

- 3.6 introduces ModuleImportError
- 3.7 begins using it within importlib
- 3.8 first instance of stdlib code catching it

- PEP-302 and PEP-451 are the definitive specifications for how sys.meta_path is supposed to work, and neither makes mention of ModuleNotFoundError. It seems at least this should have been flagged during review of the original change, but apparently the name of the exception was more important.

- The recent work done to move most of the import machinery on to sys.meta_path has exposed a set of import hooks that don't quite comply to the documented import hook interface

- The newly advertised ModuleNotFoundError appearing in stack traces for everyone means that more people will continue to write new cases of "except ModuleNotFoundError:", which while following best practice (catch most specific relevant exception), at present it amounts to relying on an implementation detail of the default importer. GitHub search reveals this to be an accurate reading: https://github.com/search?q=%22except+ModuleNotFoundError%22&type=Code

Therefore we are in a position where:

- Natural developer habit will cause much more protocol-violating code to exist over time, there is no option to stop this process
- New import hooks written against the existing documentation will break in the face of developer habit
- Existing import hooks were broken in Python 3.6 and this is not documented anywhere
- Python's own import machinery contravenes its specification

Options:

- Updating PEP-302 to mention introduction of the new exception type
- Updating ModuleNotFoundError/ImportError documentation to somehow capture the compatibility issue
msg332029 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2018-12-17 21:49
RE: "PEP-302 and PEP-451 are the definitive specifications for how sys.meta_path is supposed to work"

That's actually not true. In the case of import the language reference is considered the reference for import: https://docs.python.org/3/reference/import.html (and true for PEPs in general once they are implemented).
msg332139 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-12-19 13:52
The two errors mean different things: ModuleNotFoundError means literally that the module could not be found at all (i.e. no import hook offered to try to load it)

A plain ImportError then means that the module was located, but attempting to actually load it failed.

find_spec()/find_loader()/find_module() implementations on import plugins shouldn't be raising exceptions for modules they don't offer, and hence shouldn't be needing to raise ModuleNotFoundError directly.
msg332140 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-12-19 13:55
Note that the above distinction is also the rationale for introducing the new subtype: so that it's easy to tell the difference between "that module was not found at all" (ModuleNotFoundError) and "the module was found, but attempting to actually load it failed" (other cases of ImportError)
msg333246 - (view) Author: David Wilson (dw) * Date: 2019-01-08 19:06
Hi Nick,

The purpose of ModuleNotFoundError is clear and unrelated to the problem documented here. The problem is that due to the introduction of ModuleNotFoundError, ImportError's semantics have been changed within a minor release in a breaking, backwards-incompatible manner.

As of Python 3.5, ImportError meant both "ImportError" and "ModuleNotFoundError". As of 3.6, the senses are distinct, and thus code written against ImportError as it existed in 3.5 no longer works correctly as of 3.6.
msg333545 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-01-13 07:30
dw: we routinely impose new requirements on folks modifying runtime internals in new feature releases, so the only aspect we missed for this changing is to explicitly call it out in the Porting section of the Python 3.6 What's New document as a potential forward compatibility risk for import system replacement authors that don't update their plugins accordingly.

For folks implementing PEP 302 finders and loaders, no change is required, as they indicate failure to find a module by returning None, not by raising ImportError.

It's only folks emulating the full import system by replacing `__import__` that should need to worry about this.
msg333848 - (view) Author: miss-islington (miss-islington) Date: 2019-01-17 10:41
New changeset cee29b46a19116261b083dc803217aa754c7df40 by Miss Islington (bot) (Nick Coghlan) in branch 'master':
bpo-35486: Note Py3.6 import system API requirement change (GH-11540)
https://github.com/python/cpython/commit/cee29b46a19116261b083dc803217aa754c7df40
msg333849 - (view) Author: miss-islington (miss-islington) Date: 2019-01-17 10:48
New changeset 422db3777874f4f31fc8f4e718f440a2abc59347 by Miss Islington (bot) in branch '3.7':
bpo-35486: Note Py3.6 import system API requirement change (GH-11540)
https://github.com/python/cpython/commit/422db3777874f4f31fc8f4e718f440a2abc59347
msg333924 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-01-18 01:11
New changeset 1edb3dc6ff70db88a7e89586578e58a86ee0e75e by Ned Deily (Miss Islington (bot)) in branch '3.6':
bpo-35486: Note Py3.6 import system API requirement change (GH-11540) (GH-11588)
https://github.com/python/cpython/commit/1edb3dc6ff70db88a7e89586578e58a86ee0e75e
msg333925 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-01-18 01:12
I've merged the doc changes for 3.6, thanks.  Can we close this now?
History
Date User Action Args
2019-01-18 01:12:22ned.deilysetkeywords: patch, patch, patch

messages: + msg333925
2019-01-18 01:11:12ned.deilysetnosy: + ned.deily
messages: + msg333924
2019-01-17 10:48:17miss-islingtonsetmessages: + msg333849
2019-01-17 10:42:59miss-islingtonsetpull_requests: + pull_request11282
2019-01-17 10:42:47miss-islingtonsetpull_requests: + pull_request11281
2019-01-17 10:42:37miss-islingtonsetpull_requests: + pull_request11283
2019-01-17 10:42:27miss-islingtonsetpull_requests: + pull_request11280
2019-01-17 10:42:17miss-islingtonsetpull_requests: + pull_request11279
2019-01-17 10:42:07miss-islingtonsetpull_requests: + pull_request11278
2019-01-17 10:41:31miss-islingtonsetnosy: + miss-islington
messages: + msg333848
2019-01-13 07:30:47ncoghlansetkeywords: patch, patch, patch

messages: + msg333545
2019-01-13 07:24:14ncoghlansetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request11149
2019-01-13 07:24:05ncoghlansetkeywords: + patch
stage: test needed -> test needed
pull_requests: + pull_request11148
2019-01-13 07:23:55ncoghlansetkeywords: + patch
stage: test needed -> test needed
pull_requests: + pull_request11147
2019-01-08 19:06:21dwsetmessages: + msg333246
2018-12-19 13:55:10ncoghlansetmessages: + msg332140
2018-12-19 13:52:49ncoghlansetmessages: + msg332139
2018-12-17 21:49:06brett.cannonsetnosy: + davin
messages: + msg332029
2018-12-14 21:52:24terry.reedysetnosy: + brett.cannon, ncoghlan, eric.snow

type: behavior
stage: test needed
title: subprocess module breaks backwards compatibility with import hooks -> subprocess module import hooks breaks back compatibility
2018-12-14 01:10:25dwsetmessages: + msg331776
2018-12-14 00:02:33dwsettitle: subprocess module breaks backwards compatibility with older import hooks -> subprocess module breaks backwards compatibility with import hooks
2018-12-13 23:15:03dwsetmessages: + msg331775
2018-12-13 23:14:15dwcreate