pytorch-lightning: [RFC] Revisit Usage of MisconfigurationException

I think the best would be to open a draft PR so that we can directly review the code at hand 😃 Of course if you have any questions before that, feel free to ask here:)

Yes, let’s go with the 3rd option. This is my suggested implementation:

class _DeprecatedException(type):
    def __instancecheck__(cls: Type, instance: object) -> bool:
        # https://peps.python.org/pep-3119/
        return any(cls.__subclasscheck__(c, stacklevel=7) for c in {type(instance), instance.__class__})

    def __subclasscheck__(cls, subclass: Type, stacklevel: int = 5) -> bool:
        mro = subclass.mro()
        real = mro[1]
        if cls is not subclass and cls is real:
            raise TypeError(f"`{subclass.__name__}` should not be used for checks.")
        if cls is MisconfigurationException:
            rank_zero_deprecation(
                f"`{MisconfigurationException.__name__}` is deprecated. Please check with `{real.__name__}` instead.",
                stacklevel=stacklevel,
            )
        return cls in mro


class MisconfigurationException(Exception, metaclass=_DeprecatedException):
    """Exception used to inform users of misuse with PyTorch Lightning."""

    def __init__(self, *args: Any, **kwargs: Any) -> None:
        rank_zero_deprecation(f"Using `{type(self).__name__}` is deprecated.", stacklevel=5)
        super().__init__(*args, **kwargs)


# protected to avoid future deprecations
class _ValueError(ValueError, MisconfigurationException):
    ...

def test_exception_deprecations():
    from pytorch_lightning.utilities.exceptions import _ValueError, MisconfigurationException
    from tests_pytorch.deprecated_api import no_deprecated_call

    with no_deprecated_call():
        x = _ValueError("test")
        assert isinstance(x, _ValueError)  # this needs to work for proper instantiation
    with pytest.deprecated_call(match="Please check with `ValueError"):
        assert isinstance(x, MisconfigurationException)
    with no_deprecated_call():
        assert isinstance(x, ValueError)

    with pytest.deprecated_call(match="Using `MisconfigurationException` is deprecated"):
        y = MisconfigurationException("this is deprecated")
    with no_deprecated_call():
        assert isinstance(y, MisconfigurationException)
    with no_deprecated_call():
        assert not isinstance(y, ValueError)

    try:
        raise x
    except MisconfigurationException:
        pass

    try:
        raise x
    except ValueError:
        pass

Where we would use these custom protected variants until MisconfigurationException is removed.

The work left for you @calebbuffa would be to update all usages of MisconfigurationException to use these new alternatives

I believe we can deprecate it doing this:

from pytorch_lightning.utilities.exceptions import MisconfigurationException


class ValueError(ValueError, MisconfigurationException):
    ...

error = ValueError("this is a test")
print(isinstance(error, __builtins__.ValueError))  # True
print(isinstance(error, MisconfigurationException))  # True
raise error  # __main__.ValueError: this is a test

where importing MisconfigurationException is what shows the deprecation message.

Looks great so far @calebbuffa. Left minor comments

Deprecating the MisconfigurationError is definitely not the first step. First we need to replace the usages. Only at the end, when only the definition of MisconfigurationError is left can we deprecate this error class.

@awaelchli The reasoning behind creating this DeprecatedException “magic” was to not break users who had except MisconfigurationException in their code.

why new exceptions need to inherit from a deprecated exception?

The above is the reason

introduces a lot of coupling

What is this coupling?

new code inheriting from deprecated code feels like a recipe for refactoring/future breakages

Yes, they would all be removed after the deprecation process is over.

while new errors (i.e. LightningValueError) simply inherent from Exception

This would not be different to what MisconfigurationException does. The end goal to use the more informative and native exceptions (ValueError, TypeError, …), but what’s tricky is how to get there whilst minimizing breaking changes

Note that not all of them should be ValueError. TypeError, RuntimeError, or a different exception might fit better depending on the case. I just wrote _ValueError in my previous message for conciseness.

Choosing the most fitting exception class is what’s most important: https://docs.python.org/3/library/exceptions.html#base-classes

Agreed, so in option 3 we would want a deprecation warning if MisconfigurationException is imported but not if an error inheriting from it is, correct? Also, did we lean towards lightning specific sub classes (i.e LightingValueError) as originally proposed or using generic errors?