subprocess module import hooks breaks back compatibility

[dw]

BPO	35486
Nosy	@brettcannon, @ncoghlan, @ned-deily, @ericsnowcurrently, @dw, @applio, @miss-islington
PRs	bpo-35486: Note Py3.6 import system API requirement change #11540 bpo-35486: Note Py3.6 import system API requirement change #11540 bpo-35486: Note Py3.6 import system API requirement change #11540 [3.7] bpo-35486: Note Py3.6 import system API requirement change (GH-11540) #11587 [3.7] bpo-35486: Note Py3.6 import system API requirement change (GH-11540) #11587 [3.7] bpo-35486: Note Py3.6 import system API requirement change (GH-11540) #11587 [3.6] bpo-35486: Note Py3.6 import system API requirement change (GH-11540) #11588 [3.6] bpo-35486: Note Py3.6 import system API requirement change (GH-11540) #11588 [3.6] bpo-35486: Note Py3.6 import system API requirement change (GH-11540) #11588

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-01-22.11:20:27.021>
created_at = <Date 2018-12-13.23:14:15.410>
labels = ['type-bug', '3.8', 'docs']
title = 'subprocess module import hooks breaks back compatibility'
updated_at = <Date 2019-01-22.11:20:27.016>
user = 'https://github.com/dw'

bugs.python.org fields:

activity = <Date 2019-01-22.11:20:27.016>
actor = 'ncoghlan'
assignee = 'docs@python'
closed = True
closed_date = <Date 2019-01-22.11:20:27.021>
closer = 'ncoghlan'
components = ['Documentation']
creation = <Date 2018-12-13.23:14:15.410>
creator = 'dw'
dependencies = []
files = []
hgrepos = []
issue_num = 35486
keywords = ['patch', 'patch', 'patch']
message_count = 13.0
messages = ['331774', '331775', '331776', '332029', '332139', '332140', '333246', '333545', '333848', '333849', '333924', '333925', '334206']
nosy_count = 8.0
nosy_names = ['brett.cannon', 'ncoghlan', 'ned.deily', 'docs@python', 'eric.snow', 'dw', 'davin', 'miss-islington']
pr_nums = ['11540', '11540', '11540', '11587', '11587', '11587', '11588', '11588', '11588']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35486'
versions = ['Python 3.8']

[dw]

The subprocess package since 880d42a / 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.

[dw]

Corrected GitHub link for the commit: 880d42a3b24

[dw]

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

[brettcannon]

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).

[ncoghlan]

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.

[ncoghlan]

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)

[dw]

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.

[ncoghlan]

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.

[miss-islington]

New changeset cee29b4 by Miss Islington (bot) (Nick Coghlan) in branch 'master':
bpo-35486: Note Py3.6 import system API requirement change (GH-11540)
cee29b4

[miss-islington]

New changeset 422db37 by Miss Islington (bot) in branch '3.7':
bpo-35486: Note Py3.6 import system API requirement change (GH-11540)
422db37