Skip to content

Commit

Permalink
fix is_subclass bug where issubclass(list[str], DagsterType) throws a…
Browse files Browse the repository at this point in the history
… surprising exception (#8287)
  • Loading branch information
gibsondan authored and johannkm committed Jun 9, 2022
1 parent 58fa8ca commit 561b81a
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 7 deletions.
5 changes: 3 additions & 2 deletions python_modules/dagster/dagster/config/field.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from dagster.config.config_schema import ConfigSchemaType
from dagster.core.errors import DagsterInvalidConfigError, DagsterInvalidDefinitionError
from dagster.serdes import serialize_value
from dagster.seven import is_subclass
from dagster.utils import is_enum_value
from dagster.utils.typing_api import is_closed_python_optional_type, is_typing_type

Expand All @@ -13,7 +14,7 @@


def _is_config_type_class(obj):
return isinstance(obj, type) and issubclass(obj, ConfigType)
return isinstance(obj, type) and is_subclass(obj, ConfigType)


def helpful_list_error_string():
Expand Down Expand Up @@ -111,7 +112,7 @@ def resolve_to_config_type(dagster_type: object) -> Union[ConfigType, bool]:
'another dagster config type. E.g. "Noneable(Permissive)" should instead be "Noneable(Permissive())".',
)

if isinstance(dagster_type, type) and issubclass(dagster_type, DagsterType):
if isinstance(dagster_type, type) and is_subclass(dagster_type, DagsterType):
raise DagsterInvalidDefinitionError(
"You have passed a DagsterType class {dagster_type} to the config system. "
"The DagsterType and config schema systems are separate. "
Expand Down
9 changes: 5 additions & 4 deletions python_modules/dagster/dagster/core/types/dagster_type.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from dagster.core.definitions.metadata import MetadataEntry, RawMetadataValue, normalize_metadata
from dagster.core.errors import DagsterInvalidDefinitionError, DagsterInvariantViolationError
from dagster.serdes import whitelist_for_serdes
from dagster.seven import is_subclass

from ..definitions.resource_requirement import (
RequiresResources,
Expand Down Expand Up @@ -835,12 +836,12 @@ def resolve_dagster_type(dagster_type: object) -> DagsterType:
from .transform_typing import transform_typing_type

check.invariant(
not (isinstance(dagster_type, type) and issubclass(dagster_type, ConfigType)),
not is_subclass(dagster_type, ConfigType),
"Cannot resolve a config type to a runtime type",
)

check.invariant(
not (isinstance(dagster_type, type) and issubclass(dagster_type, DagsterType)),
not is_subclass(dagster_type, DagsterType),
"Do not pass runtime type classes. Got {}".format(dagster_type),
)

Expand Down Expand Up @@ -907,12 +908,12 @@ def is_dynamic_output_annotation(dagster_type: object) -> bool:
from dagster.seven.typing import get_args, get_origin

check.invariant(
not (isinstance(dagster_type, type) and issubclass(dagster_type, ConfigType)),
not is_subclass(dagster_type, ConfigType),
"Cannot resolve a config type to a runtime type",
)

check.invariant(
not (isinstance(dagster_type, type) and issubclass(dagster_type, DagsterType)),
not is_subclass(dagster_type, DagsterType),
"Do not pass runtime type classes. Got {}".format(dagster_type),
)

Expand Down
16 changes: 16 additions & 0 deletions python_modules/dagster/dagster/seven/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,19 @@ def get_import_error_message(import_error):
@contextmanager
def nullcontext():
yield


def is_subclass(child_type, parent_type):
"""Due to some pathological interactions betwen bugs in the Python typing library
(https://github.com/python/cpython/issues/88459 and
https://github.com/python/cpython/issues/89010), some types (list[str] in Python 3.9, for
example) pass inspect.isclass check above but then raise an exception if issubclass is called
with the same class. This function provides a workaround for that issue."""

if not inspect.isclass(child_type):
return False

try:
return issubclass(child_type, parent_type)
except TypeError:
return False
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import inspect
import json
import sys
import tempfile
from functools import update_wrapper
from typing import List

import pytest

from dagster import seven
from dagster import DagsterType, seven
from dagster.core.types.dagster_type import ListType
from dagster.seven import is_subclass
from dagster.utils import file_relative_path


Expand Down Expand Up @@ -90,3 +95,37 @@ def yoodles():
assert seven.is_function_or_decorator_instance_of(bar, Quux) == True
assert seven.is_function_or_decorator_instance_of(baz, Quux) == False
assert seven.is_function_or_decorator_instance_of(yoodles, Quux) == True


class Foo:
pass


class Bar(Foo):
pass


def test_is_subclass():
assert is_subclass(Bar, Foo)
assert not is_subclass(Foo, Bar)

assert is_subclass(DagsterType, DagsterType)
assert is_subclass(str, str)
assert is_subclass(ListType, DagsterType)
assert not is_subclass(DagsterType, ListType)
assert not is_subclass(ListType, str)

# type that aren't classes can be passed into is_subclass
assert not inspect.isclass(List[str])
assert not is_subclass(List[str], DagsterType)


@pytest.mark.skipif(
sys.version_info.minor < 9, reason="Generic aliases only exist on py39 or later"
)
def test_is_subclass_generic_alias():
# See comments around is_subclass for why this fails
with pytest.raises(TypeError):
issubclass(list[str], DagsterType)

assert not is_subclass(list[str], DagsterType)

0 comments on commit 561b81a

Please sign in to comment.