Skip to content

Commit

Permalink
Allow classes to be decorated with @experimental
Browse files Browse the repository at this point in the history
Summary:
The existing @experimental decorator doesn't work particularly well when
trying to decorate a class. Previously, for vanilla class decorations,
things would mostly work:

```
@experimental
class Foo:
  pass

foo = Foo()
```

would emit:

```
ExperimentalWarning: "Foo" is an experimental function.
```

That error is mostly correct except for the fact that `Foo` is supposed
to be an experimental `class` and not an experimental `function`. But
it's more than just a copy error - because the `@experimental` decorator
is returning an `_inner` function, `Foo` truly is of type `function`.

The most immediate consequence is that nothing can properly subclass
`Foo`:

```
@experimental
class Bar(Foo):
  pass
```

fails with:

```
E   TypeError: function() argument 'code' must be code, not str
```

It's a bit of a cryptic error but it's throwing because you can't
subclass a function. https://bugs.python.org/issue6829

An interesting Dagster-specific consequence is that you can't mark
anything that subclasses `ConfigurableClass` as `@experimental`:

```
@experimental
class Configurable(ConfigurableClass):
  ...
```

fails with:

```
E       TypeError: issubclass() arg 1 must be a class
```

It's throwing because `issubclass` expects its first argument to
be a `class` and we're giving it a `function`:

https://github.com/dagster-io/dagster/blob/34bfa587ffb19821a272f76c52dfb750e0ca7de8/python_modules/dagster/dagster/serdes/config_class.py#L70

One option is to take the approach that we took when introducing
`@experimental_decorator` - we could add a new `@experimental_class` and
leave it up to the author to correctly decide when to decorate with
`@experimental` and when to decorate with `@experimental_class`.

Instead, I've decided to conditionally switch on whether the callable
being decorated is a `function` or a `class`. If it's a `function`, we
do the same wrapping behavior we've always done. If it's a `class`, we
augment `__init__` to also include an `experimental_class_warning` but
we still return a `class`.

Test Plan: unit

Reviewers: sandyryza

Reviewed By: sandyryza

Differential Revision: https://dagster.phacility.com/D8484
  • Loading branch information
jmsanders committed Jun 21, 2021
1 parent 34bfa58 commit a1900c2
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 16 deletions.
31 changes: 24 additions & 7 deletions python_modules/dagster/dagster/utils/backcompat.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import inspect
import warnings
from functools import wraps

Expand Down Expand Up @@ -170,7 +171,7 @@ def experimental_class_param_warning(param_name, class_name, stacklevel=3):
)


def experimental(fn):
def experimental(callable_):
"""
Spews an "experimental" warning whenever the given callable is called. If the argument is a
class, this means the warning will be emitted when the class is instantiated.
Expand All @@ -182,15 +183,31 @@ def experimental(fn):
@experimental
def my_experimental_function(my_arg):
do_stuff()
@experimental
class MyExperimentalClass:
pass
"""
check.callable_param(fn, "fn")
check.callable_param(callable_, "callable_")

@wraps(fn)
def _inner(*args, **kwargs):
experimental_fn_warning(fn.__name__, stacklevel=3)
return fn(*args, **kwargs)
if inspect.isfunction(callable_):

return _inner
@wraps(callable_)
def _inner(*args, **kwargs):
experimental_fn_warning(callable_.__name__, stacklevel=3)
return callable_(*args, **kwargs)

return _inner

if inspect.isclass(callable_):
undecorated_init = callable_.__init__

def __init__(self, *args, **kwargs):
experimental_class_warning(callable_.__name__, stacklevel=3)
undecorated_init(self, *args, **kwargs)

callable_.__init__ = __init__
return callable_


def experimental_decorator(decorator):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import pytest
from dagster.check import CheckError
from dagster.utils.backcompat import (
EXPERIMENTAL_WARNING_HELP,
ExperimentalWarning,
canonicalize_backcompat_args,
experimental,
Expand Down Expand Up @@ -108,13 +107,10 @@ def stable_function(_stable_arg, _experimental_arg):
assert warning[0].filename.endswith("test_backcompat.py")


def test_experimental_decorator():
with assert_no_warnings():

@experimental
def my_experimental_function(some_arg):
assert some_arg == 5
return 4
def test_experimental_decorator_function():
@experimental
def my_experimental_function(arg):
return arg

assert my_experimental_function.__name__ == "my_experimental_function"

Expand All @@ -123,6 +119,50 @@ def my_experimental_function(some_arg):
match='"my_experimental_function" is an experimental function. It may break in future'
" versions, even between dot releases. ",
) as warning:
assert my_experimental_function(5) == 4
assert my_experimental_function(5) == 5

assert warning[0].filename.endswith("test_backcompat.py")


def test_experimental_decorator_class():
@experimental
class ExperimentalClass:
def __init__(self, salutation="hello"):
self.salutation = salutation

def hello(self, name):
return f"{self.salutation} {name}"

@experimental
class ExperimentalClassWithExperimentalFunction(ExperimentalClass):
def __init__(self, sendoff="goodbye", **kwargs):
self.sendoff = sendoff
super().__init__(**kwargs)

@experimental
def goodbye(self, name):
return f"{self.sendoff} {name}"

with pytest.warns(
ExperimentalWarning,
match='"ExperimentalClass" is an experimental class. It may break in future versions, even between dot releases.',
):
experimental_class = ExperimentalClass(salutation="howdy")

with assert_no_warnings():
assert experimental_class.hello("dagster") == "howdy dagster"

with pytest.warns(
ExperimentalWarning,
match='"ExperimentalClassWithExperimentalFunction" is an experimental class. It may break in future versions, even between dot releases.',
):
experimental_class_with_experimental_function = ExperimentalClassWithExperimentalFunction()

with assert_no_warnings():
assert experimental_class_with_experimental_function.hello("dagster") == "hello dagster"

with pytest.warns(
ExperimentalWarning,
match='"goodbye" is an experimental function. It may break in future versions, even between dot releases.',
):
assert experimental_class_with_experimental_function.goodbye("dagster") == "goodbye dagster"

1 comment on commit a1900c2

@vercel
Copy link

@vercel vercel bot commented on a1900c2 Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.