Repository navigation
fix(utils): narrow check_import exception handling - #1720
Conversation
Catch ImportError instead of bare Exception so present-but-broken modules surface their real error instead of a misleading missing-module hint. Focused tests in tests/utils/test_import_utils.py.
Yunnglin
left a comment
Yunnglin
left a comment
There was a problem hiding this comment.
Thanks for the focused fix and the regression coverage — the underlying issue is valid.
However, changing except Exception to except ImportError does not fully separate a missing requested module from a module that exists but fails during initialization. ImportError can be raised deliberately by the module itself, and ModuleNotFoundError is also an ImportError subclass, including when an already-installed module is missing one of its private dependencies. Those cases would still be converted into the misleading "<module> not found" / install-hint path.
I suggest splitting discovery from execution instead of catching import-time errors broadly:
- Use
importlib.util.find_spec()to determine whether the requested module is discoverable. - Treat
find_spec(...) is None(or aModuleNotFoundErrorfor the requested module / an absent parent of a dotted requested name) as the only “missing module” case. - Run
importlib.import_module()after discovery, outside that missing-module handler, so every failure from executing an available module propagates with its original traceback.
This keeps the existing missing-dependency UX while preserving actionable errors from broken or incompatible installations.
Please also add regressions for:
- an existing module whose body raises
ImportError("boom"); - an existing module that imports a missing internal dependency and raises
ModuleNotFoundError; - both cases propagating the original exception without a
not foundwarning or install hint.
The current RuntimeError test is useful, but these two ImportError-family cases are the boundary this change needs to handle.
|
Thanks for the detailed review — revised in "46fe5b5b" as suggested. "check_import" now uses "find_spec()" for discovery and runs "import_module()" outside the missing-module handler, so import-time failures from available modules propagate unchanged. I also added the requested regressions for body-raised "ImportError("boom")" and an internal-dependency "ModuleNotFoundError", plus dotted-module boundary coverage. Repository pytest still can't collect in this sandbox because the full dependency chain ("datasets") isn't installed; the focused harness passes all 9 scenarios and Ruff passes. CI can provide the final repository-level validation. |
Yunnglin
left a comment
Yunnglin
left a comment
There was a problem hiding this comment.
LGTM. The requested discovery/execution separation and regression coverage address the review feedback; targeted tests and Ruff checks pass.
check_import previously reported any import-time exception as module-not-found, hiding real breakage behind a pip-install hint. The first revision (catching ImportError only) still misclassified two cases, as pointed out in review: a module body deliberately raising ImportError("boom"), and an installed module failing on its own missing internal dependency (ModuleNotFoundError, an ImportError subclass).
This revision splits discovery from execution, as suggested:
importlib.util.find_spec()decides whether the requested module is truly discoverable; only undiscoverable modules enter the missing-module warning/install-hint/raise_error path.ModuleNotFoundErrorfromfind_spec()on a dotted name counts as missing only when the absent module is the requested module itself or one of its parents (name-prefix check); a broken parent failing on its own internal dependency re-raises untouched.importlib.import_module()runs outside any handler, so every import-time failure of an available module propagates with its original traceback — never rewritten as " not found".Behavior and messages for genuinely missing modules are unchanged.
Focused tests in
tests/utils/test_import_utils.pycover: missing module returns False; missing module withraise_errorraisesImportError("not found"); existing module (os) and dotted existing module (os.path) return True; body-raisedRuntimeError/ImportError("boom")propagate with the original message and no warning; an internal missing dependency propagates asModuleNotFoundErrornaming the internal dependency (not the requested module) with no warning; dotted missing parent and dotted missing child of an existing package stay on the missing-module path (False + warning +raise_error).Note: the new tests could not run via repository pytest in this sandbox (collection requires the full installed dependency chain, e.g.
datasets); they were validated with a standalone harness loading the realimport_utils.py(9/9 scenarios pass, including fail-before confirmation on the ImportError-family cases). Please let CI run the full suite as final validation. Ruff check passes on both touched files; the applicable Ruff format check also passes.