importorskip variant that uses ModuleNotFoundError

[graingert]

What's the problem this feature will solve?

sometimes I install a package eg PyTables and it fails to compile properly and fails with an ImportError with an undefined symbol, however I don't see this in CI because I only do pytest.importorskip("pytables")

Describe the solution you'd like

a new kwarg to importorskip or a new function that catches ModuleNotFoundError instead of ImportError eg
importorskip(exc=ModuleNotFoundError) or findmoduleorskip(...)

Alternative Solutions

N/A

Additional context

Originally asked on discord

[nicoddemus]

Hey @graingert thanks for the proposal.

a new kwarg to importorskip or a new function that catches ModuleNotFoundError instead of ImportError eg
importorskip(exc=ModuleNotFoundError) or findmoduleorskip(...)

I personally like importorskip(exc=ModuleNotFoundError) 👍

[RonnyPfannschmidt]

i think it would be very nice if a requirement could be passed to allow checking for versions

[bluetech]

The alternative to exc= is to just narrow the current except ImportError to except ModuleNotFoundError. This will a breaking change, but I wonder if anyone really wants to skip on non-ModuleNotFoundError import errors?

[graingert]

We could also check the exception message matches the module being imported

[nicoddemus]

This will a breaking change, but I wonder if anyone really wants to skip on non-ModuleNotFoundError import errors?

Thought the same, I for one would always add exc=ModuleNotFoundError.

Perhaps it is worthwhile to go through a deprecation period, where we issue a warning in case a importorskip skipped a module but the message did not include the actual module name? Later we can apply the change to only catch ModuleNotFoundError then.

[The-Compiler]

I don't understand the rationale of doing string matching against the exception message here. Why would we need to check that if we can just check the exception type? Why would it be useful to catch an ImportError which is not a ModuleNotFoundError but just happens to include the module name, vs. one which does not?

[nicoddemus]

Why would we need to check that if we can just check the exception type? Why would it be useful to catch an ImportError which is not a ModuleNotFoundError but just happens to include the module name, vs. one which does not?

Just to issue a warning that the behavior will change in the future, where we will only catch ModuleNotFoundError:

except ModuleNotFoundError:
    skip()
except ImportError as e:
    if module_name not in str(e):
        warning(f"importorskip('{modulename}') caught an ImportError, but the message:\n{e}\n does not contain the module name. This behavior will change in the future to only catch ModuleNotFoundError")
   skip()

The idea is that we remove the except ImportError clause in a future release, as this is a breaking change (for the better IMHO).

[The-Compiler]

I still don't follow. Why would we not want to warn if the message contains the module name?

[nicoddemus]

To let users know that this might break their test suite in the future, suppose a user has this in their code base:

def test_numpy_support() -> None:
    pytest.importorskip("numpy")
    ...

Currently, the test is skipped, but not because numpy is not installed, but because it is failing to import due to something else entirely, for example due to an undefined symbol.

If we just change importorskip to catch ModuleNotFoundError, this would cause the test suite to break.

We might decide instead to introduce a warning, so users would see a warning like this currently:

PytestDeprecationWarning: importorskip caught ImportError with this message:

    ImportError: undefined symbol ...

Seems like the reason for the failure was not that the module was missing, but another cause.
pytest X will only catch `ModuleNotFoundError`, so this will fail in the future.

Then in a future release we change the code to only catch ModuleNotFoundError.

---

But now that I write this, perhaps does not make sense to introduce this warning at all? Seems like users would like for this to fail right away?

[The-Compiler]

But in any case the module actually isn't present, we already will get a ModuleNotFoundError from Python. Why not warn on any ImportError, if anything? I don't see a reason why we should warn if, say, the undefined symbol in your example happens to be SomeLib::bla() but should not warn if the symbol happens to be called NumpyCExt::bla().

[nicoddemus]

Oh you are right, we will already be catching ModuleNotFoundError explicitly, so any ImportError would be enough to generate a warning, no need to check the message at all.

Sorry for the noise!

We need to decide if we want to generate a warning, or go with the "breaking" change directly (I'm using quotes because one might argue that this is a clear improvement).

[nicoddemus]

#12220 has been submitted, we need to decide if we will go with a standard improvement, or want to introduce a warning first.