Skip to content

Surface internal import errors in _import_dotted_name #606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
+44 −1
Open

Surface internal import errors in _import_dotted_name #606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion fiddle/_src/absl_flags/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,17 @@ def _import_dotted_name(
f'attribute {failing_name!r}; available names: {available_names}'
) from None
return value
except ModuleNotFoundError:
except ModuleNotFoundError as e:
# If the missing module isn't along the path we tried to import,
# it's an internal import error — surface the real error.
# e.name is None when the error has no associated module name
if e.name is None:
raise
# If e.name is a prefix of name_pieces, the path simply doesn't
# exist at this split point. Otherwise, the module
# exists but broke importing an unrelated dependency.
if (missing := e.name.split('.')) != name_pieces[: len(missing)]:
raise
if i == 1: # Final iteration through the loop.
raise

Expand Down
33 changes: 33 additions & 0 deletions fiddle/_src/absl_flags/utils_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
# limitations under the License.

import dataclasses
import os
import sys
import tempfile
from typing import Any

from absl.testing import absltest
Expand Down Expand Up @@ -159,5 +161,36 @@ def test_from_fully_qualified_name(self):
self.assertEqual(cfg, base_experiment())


class ImportDottedNameTest(absltest.TestCase):
"""Tests that _import_dotted_name surfaces real import errors."""

def test_internal_import_error_is_not_swallowed(self):
"""A module that exists but has a broken import should raise."""
tmpdir = self.enterContext(tempfile.TemporaryDirectory())
module_path = os.path.join(tmpdir, '_broken_module.py')
with open(module_path, 'w') as f:
f.write('import _nonexistent_dependency\n')
sys.path.insert(0, tmpdir)
self.addCleanup(lambda: sys.path.remove(tmpdir))
self.addCleanup(lambda: sys.modules.pop('_broken_module', None))

with self.assertRaises(ModuleNotFoundError) as ctx:
utils._import_dotted_name(
'_broken_module.some_symbol',
mode=_IRRELEVANT_MODE,
module=None,
)
self.assertIn('_nonexistent_dependency', str(ctx.exception))

def test_nonexistent_module_raises_module_not_found(self):
"""A module that doesn't exist should raise ModuleNotFoundError."""
with self.assertRaises(ModuleNotFoundError):
utils._import_dotted_name(
'completely_nonexistent_module.some_symbol',
mode=_IRRELEVANT_MODE,
module=None,
)


if __name__ == '__main__':
absltest.main()
Loading