Improve ImportString error when internal imports fail by tsembp · Pull Request #12740 · pydantic/pydantic

tsembp · 2026-01-25T18:24:09Z

Change Summary

What this PR does

Improves the error handling of ImportString so that internal import failures
(e.g. missing dependencies inside a module) are no longer masked as
ImportError: No module named ....

If the missing module is not the requested import path (or its submodules),
the original ModuleNotFoundError is now correctly re-raised.

Why

Previously, ImportString could incorrectly report that a module did not exist,
even when the real issue was a missing internal dependency. This made debugging
harder and hid the true cause of the failure.

Changes

Detect whether ModuleNotFoundError.name refers to the requested module
Re-raise internal import errors instead of masking them
Added tests covering the new behavior

Tests

Added new tests in tests/test_types.py
All existing tests pass locally

Related issue number

Checklist

The pull request title is a good summary of the changes - it will be used in the changelog
Unit tests for the changes exist
Tests pass on CI
Documentation reflects the changes where applicable
My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @Viicos

tsembp · 2026-01-25T18:24:22Z

please review

codspeed · 2026-01-25T18:37:38Z

CodSpeed Performance Report

Merging this PR will improve performance by 6.66%

_{Comparing tsembp:fix-importstring-internal-importerror (0a18c38) with main (c42224a)}

Summary

⚡ 1 improved benchmark
✅ 211 untouched benchmarks

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	`test_list_of_ints_core_json`	780.4 µs	731.7 µs	+6.66%

tsembp · 2026-01-30T09:00:06Z

Hi @Viicos , just a quick follow-up in case this slipped through.

No rush at all, but happy to make any adjustments or add tests if needed.
Thanks!

davidhewitt · 2026-01-31T16:57:05Z

Is there a possible bug here, where if len(components) == 2 we already had a :attribute which we're discarding and ignoring if line 120 returns?

Good catch, thanks for pointing this out.

You’re right, in the case where the input already includes a :attribute, the fallback path here could incorrectly discard it. That wasn’t intentional.

I’ll adjust the logic to preserve the original attribute when : is already present and add a test to cover this case.

No worries, I think this bug existed before your diff, however would be good to fix as part of the adjustments here.

Hello again @davidhewitt !
I just made changes in the code snippet that should cover the case you pointed out and also added a test for that as well.
Let me know I need to adjust something further.

davidhewitt

Thanks, some suggestions to simplify / refine.

davidhewitt · 2026-02-02T13:50:20Z

        raise ImportError(f"Import strings should have at most one ':'; received {dotted_path!r}")
-
+
+    has_explicit_attr = len(components) == 2


I would prefer to write this as

Suggested change

has_explicit_attr = len(components) == 2

attribute = None:

if len(components) == 2:

attribute = components[1]

davidhewitt · 2026-02-02T13:50:57Z

    if len(components) > 1:
        attribute = components[1]


With the other change to set attribute above,

Suggested change

if len(components) > 1:

attribute = components[1]

if attribute is not None:

davidhewitt · 2026-02-02T13:51:53Z

+        if missing and not (missing == module_path or missing.startswith(module_path + ".")):
+            raise
+
+        if not has_explicit_attr and '.' in module_path:


With the other change to set attribute above,

Suggested change

if not has_explicit_attr and '.' in module_path:

if attribute is None and '.' in module_path:

davidhewitt · 2026-02-02T13:52:33Z

                return _import_string_logic(f'{maybe_module_path}:{maybe_attribute}')
            except ImportError:
                pass
            raise ImportError(f'No module named {module_path!r}') from e


I think this error is the real cause of your reported bug, let's just remove it and let e say what's going on.

Suggested change

raise ImportError(f'No module named {module_path!r}') from e

davidhewitt · 2026-02-02T13:53:09Z

+        # If the missing module is NOT the module we tried to import (or its submodule),
+        # then it's an internal import failure (missing dependency) and we must not mask it.
+        missing = getattr(e, "name", None)
+        if missing and not (missing == module_path or missing.startswith(module_path + ".")):
+            raise


I think if the raise ImportError(...) on line 125 is removed as I suggest, we won't need this.

Suggested change

# If the missing module is NOT the module we tried to import (or its submodule),

# then it's an internal import failure (missing dependency) and we must not mask it.

missing = getattr(e, "name", None)

if missing and not (missing == module_path or missing.startswith(module_path + ".")):

raise

davidhewitt · 2026-02-02T13:54:43Z

+    with pytest.raises(ValidationError) as exc_info:
+        adapter.validate_python('my_module.MyClass')
+
+    msg = str(exc_info.value)
+    assert "definitely_missing_dep_xyz" in msg
+    assert "my_module" in msg
+    # ensure we don't incorrectly claim the object path is missing
+    assert "my_module.MyClass" not in msg or "No module named 'my_module.MyClass'" not in msg


I think asserting the underlying message should be good enough

Suggested change

with pytest.raises(ValidationError) as exc_info:

adapter.validate_python('my_module.MyClass')

msg = str(exc_info.value)

assert "definitely_missing_dep_xyz" in msg

assert "my_module" in msg

# ensure we don't incorrectly claim the object path is missing

assert "my_module.MyClass" not in msg or "No module named 'my_module.MyClass'" not in msg

with pytest.raises(ValidationError, match="No module named 'definitely_missing_dep_xyz'") as exc_info:

adapter.validate_python('my_module.MyClass')

davidhewitt · 2026-02-02T13:58:08Z

+def test_import_string_explicit_colon_does_not_try_dot_fallback():
+    # Regression test: if the input already contains ':attr', we should NOT try
+    # to reinterpret dots as module/attribute splits (which could accidentally
+    # create an invalid import string containing two colons).
+    with pytest.raises(ModuleNotFoundError):
+        _import_string_logic("does.not.exist:Thing")


Please use a TypeAdapter in this test too, rather than the internal function. I also think we should probably use an import string like 'collections.defaultdict:get' which a buggy implementation might actually resolve, rather than a truly nonexistent path.

The removal of custom error masking now allows Python's import system to provide more detailed error messages (e.g., "'os' is not a package" instead of just "No module named"). Updated tests to match the new, more informative error output.

tsembp · 2026-02-02T15:19:32Z

Hello @davidhewitt. I've applied all your suggested changes:

Changed has_explicit_attr to attribute = None pattern
Updated conditions to use attribute is None / attribute is not None
Removed the error-masking raise ImportError(...) line
Removed the internal import detection block (no longer needed)
Simplified test assertions using match parameter
Updated test_import_string_explicit_colon_does_not_try_dot_fallback to use TypeAdapter and collections.defaultdict:get

All tests passing locally (949 passed). The error messages are now more informative. They include context from Python's import system like 'os' is not a package which helps users understand why imports fail.

Ready for another review when you have time. Thanks again for your feedback!

davidhewitt · 2026-02-02T16:14:42Z

Thanks, looks good, please fix the issues with lint and CI and then this ready to merge.

…ternal-importerror

github-actions · 2026-02-02T19:02:24Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic/_internal
_validators.py
Project Total

_{This report was generated by python-coverage-comment-action}

tsembp · 2026-02-02T19:04:42Z

@davidhewitt all lint and CI issues are now fixed and all checks are passing.
Happy to make any final tweaks, otherwise this should be ready to merge. :)

davidhewitt

Thanks, I am happy with this!

tsembp · 2026-02-05T12:49:14Z

Thank you very much!

Improve ImportString error when internal imports fail

e94bd1e

github-actions Bot added the relnotes-fix Used for bugfixes. label Jan 25, 2026

pydantic-hooky Bot added the ready for review label Jan 25, 2026

pydantic-hooky Bot assigned Viicos Jan 25, 2026

davidhewitt reviewed Jan 31, 2026

View reviewed changes

fix ImportString dot fallback when explicit attribute is provided

f7ade69

davidhewitt reviewed Feb 2, 2026

View reviewed changes

tsembp added 5 commits February 2, 2026 17:47

Merge remote-tracking branch 'upstream/main' into fix-importstring-in…

5154c99

…ternal-importerror

Fix formatting (pre-commit)

3e78903

Format tests

caead3f

Fix ruff F841 unused exception variable

76965b2

Update ImportString docstring output

0a18c38

davidhewitt approved these changes Feb 5, 2026

View reviewed changes

davidhewitt merged commit a2934a1 into pydantic:main Feb 5, 2026
67 checks passed

Viicos mentioned this pull request Feb 5, 2026

Improve logic around ImportString validation when the ImportError originates from another source #12715

Closed

1 task done

		raise ImportError(f"Import strings should have at most one ':'; received {dotted_path!r}")


		has_explicit_attr = len(components) == 2

	if len(components) > 1:
	attribute = components[1]
	if attribute is not None:

	if not has_explicit_attr and '.' in module_path:
	if attribute is None and '.' in module_path:

Conversation

tsembp commented Jan 25, 2026 • edited by Viicos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

What this PR does

Why

Changes

Tests

Related issue number

Checklist

Uh oh!

tsembp commented Jan 25, 2026

Uh oh!

codspeed Bot commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will improve performance by 6.66%

Summary

Performance Changes

Uh oh!

tsembp commented Jan 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsembp commented Feb 2, 2026

Uh oh!

davidhewitt commented Feb 2, 2026

Uh oh!

github-actions Bot commented Feb 2, 2026

Coverage report

Uh oh!

tsembp commented Feb 2, 2026

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsembp commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tsembp commented Jan 25, 2026 •

edited by Viicos

Loading

codspeed Bot commented Jan 25, 2026 •

edited

Loading