Skip to content

Commit

Permalink
Make 'multiple Definitions objects' error message more actionable (#2…
Browse files Browse the repository at this point in the history
…5453)

## Summary

Ran into this with a user - the "Cannot have more than one Definitions
object defined at module scope." error can be a bit of a rabbithole to
track down. The case we hit in particular was
when one of the `Definitions` objects was being imported unknowingly.

This PR updates the error message to include the names of all the
located `Definitions` objects, so it's easier to track down where
they're coming from and rectify the error.

## How I Tested These Changes

Update unit tests.

## Changelog

> Errors raised from defining more than one `Definitions` object at
module scope now include the object names so that the source is easier
to determine.
  • Loading branch information
benpankow authored Nov 13, 2024
1 parent 35a634f commit c5da936
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 2 deletions.
10 changes: 9 additions & 1 deletion python_modules/dagster/dagster/_core/workspace/autodiscovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ def loadable_targets_from_python_package(
return loadable_targets_from_loaded_module(module)


def _format_loadable_def(module: ModuleType, loadable_target: LoadableTarget) -> str:
return f"{module.__name__}.{loadable_target.attribute}"


def loadable_targets_from_loaded_module(module: ModuleType) -> Sequence[LoadableTarget]:
from dagster._core.definitions import JobDefinition
from dagster._utils.test.definitions import LazyDefinitions
Expand All @@ -64,8 +68,12 @@ def loadable_targets_from_loaded_module(module: ModuleType) -> Sequence[Loadable

if loadable_defs:
if len(loadable_defs) > 1:
loadable_def_names = ", ".join(
_format_loadable_def(module, loadable_def) for loadable_def in loadable_defs
)
raise DagsterInvariantViolationError(
"Cannot have more than one Definitions object defined at module scope"
"Cannot have more than one Definitions object defined at module scope."
f" Found Definitions objects: {loadable_def_names}"
)

return loadable_defs
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from dagster import Definitions

from .defs1 import defs1 # noqa: TID252
from .defs2 import defs2 # noqa: TID252

defs = Definitions.merge(
defs1,
defs2,
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from dagster import Definitions

defs1 = Definitions()
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from dagster import Definitions

defs2 = Definitions()
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,34 @@ def test_double_defs_in_file() -> None:
dagster_defs_path = file_relative_path(__file__, "double_defs.py")
with pytest.raises(
DagsterInvariantViolationError,
match="Cannot have more than one Definitions object defined at module scope",
match="Cannot have more than one Definitions object defined at module scope. Found Definitions objects: double_defs.defs, double_defs.double_defs",
):
loadable_targets_from_python_file(dagster_defs_path)


def test_local_directory_module_multiple_defs() -> None:
# this is testing that modules that *can* be resolved using the current working directory
# for local python modules instead of installed packages will succeed.

# pytest will insert the current directory onto the path when the current directory does is not
# a module
assert not os.path.exists(file_relative_path(__file__, "__init__.py"))
assert os.path.dirname(__file__) in sys.path

if "autodiscover_in_module_multiple" in sys.modules:
del sys.modules["autodiscover_in_module_multiple"]

with pytest.raises(
DagsterInvariantViolationError,
match="Cannot have more than one Definitions object defined at module scope. Found Definitions objects: autodiscover_in_module_multiple.defs, autodiscover_in_module_multiple.defs1, autodiscover_in_module_multiple.defs2",
):
loadable_targets_from_python_module(
"autodiscover_in_module_multiple",
working_directory=os.path.dirname(__file__),
remove_from_path_fn=_current_test_directory_paths,
)


def _current_test_directory_paths():
# need to remove the current test directory when checking for module/package resolution. we
# have to manually remove the appropriate path from sys.path because pytest does a lot of
Expand Down

0 comments on commit c5da936

Please sign in to comment.