burr icon indicating copy to clipboard operation
burr copied to clipboard

Class based actions: Mypy not happy with defined parameters in the `run` method

Open shun-liang opened this issue 1 year ago • 4 comments

For class-based actions, mypy is not happy with parameters to be defined in the method signature of the run method.

Current behavior

I have copied the class-based action example from doc:

from burr.core import Action, State, ApplicationContext

class Counter(Action):
    @property
    def reads(self) -> list[str]:
        return ["counter"]

    def run(self,
            state: State,
            increment_by: int,
            # add __context here
            __context: ApplicationContext) -> dict:
        # can access the app_id from the __context
        print(__context.app_id, __context.partition_key, __context.sequence_id)
        return {"counter": state["counter"] + increment_by}

    @property
    def writes(self) -> list[str]:
        return ["counter"]

    def update(self, result: dict, state: State) -> State:
        return state.update(**result)

    @property
    def inputs(self) -> list[str]:
        # add __context here
        return ["increment_by", "__context"]

Then mypy is not happy with it with errors:

mypy src/demo_application.py
src/demo_application.py:8: error: Signature of "run" incompatible with supertype "Function"  [override]
src/demo_application.py:8: note:      Superclass:
src/demo_application.py:8: note:          def run(self, state: State[Any], **run_kwargs: Any) -> dict[Any, Any]
src/demo_application.py:8: note:      Subclass:
src/demo_application.py:8: note:          def run(self, state: State[Any], increment_by: int, ApplicationContext, /) -> dict[Any, Any]
Found 1 error in 1 file (checked 1 source file)

Library & System Information

uv pip list | grep mypy
mypy               1.13.0
mypy-extensions    1.0.0

uv pip list | grep burr
burr               0.34.1

python --version
Python 3.12.5

shun-liang avatar Dec 05 '24 13:12 shun-liang

Thanks for the report! So this is an inherent mypy limitation/conscious decisiom. Mypy has no way of knowing that the fields are dynamically set by the inputs() property. To work aroudn it, you would want to use **kwargs as the signature and access the parameters dynamically. E.G.

 def run(self,
        state: State,
        **kwargs) -> dict:
        # can access the app_id from the __context
        __context = kwargs["context"]
       increment_by = kwargs["increment_by"]
        print(__context.app_id, __context.partition_key, __context.sequence_id)
        return {"counter": state["counter"] + increment_by}

We chose not to have that as it's harder to read, and I'm less a believer in strict type-checking in python, but it's there if you want it! You may be able to do something with a typed dict/type-hinting kwargs as well -- haven't tested it out -- see https://peps.python.org/pep-0692/

elijahbenizzy avatar Dec 05 '24 15:12 elijahbenizzy

Thanks for the response. For now have considered the options, I probably will just put type: ignore[override] to each individual occassion of overriding the run method in a class based action. Being a strong believer of type checking in Python myself this is quite a lot of frictions.

shun-liang avatar Dec 05 '24 16:12 shun-liang

Thanks for the response. For now have considered the options, I probably will just put type: ignore[override] to each individual occassion of overriding the run method in a class based action. Being a strong believer of type checking in Python myself this is quite a lot of frictions.

Hey! Glad you found a workaround. Sorry for the delay in responding! TBH I think this is an area in which mypy could improve, and also an area in which python/strict type-checking falls a bit flat. That said, I wouldn't be surprised if there's a better way to define the superclass, and I'm happy to dig in a bit. It quite annoys me as well... Will report back! image

elijahbenizzy avatar Dec 10 '24 19:12 elijahbenizzy

OK, so I dug in a bit. This is not something we're going to be able to solve at a framework level but I think you have two good options:

  1. Just use **kwargs -- this is "safer" (you're doing the validation, not trusting the framework), but less typed/less fun
  2. Use this decorator which works for me. There are probably better ways to do this (it really loosens the type-checking), but it's kind of clever:
from functools import wraps
from typing import Callable, Any


def adapt_types(func: Callable[..., Any]) -> Callable[..., Any]:
    @wraps(func)
    def wrapper(*args, **kwargs) -> Any:
        return func(*args, **kwargs)

    return wrapper


from burr.core import Action, State, ApplicationContext


class Counter(Action):
    @property
    def reads(self) -> list[str]:
        return ["counter"]
    @adapt_types
    def run(self,
            state: State,
            increment_by: int,
            # add __context here
            __context: ApplicationContext) -> dict:
        # can access the app_id from the __context
        print(__context.app_id, __context.partition_key, __context.sequence_id)
        return {"counter": state["counter"] + increment_by}

    @property
    def writes(self) -> list[str]:
        return ["counter"]

    def update(self, result: dict, state: State) -> State:
        return state.update(**result)

    @property
    def inputs(self) -> list[str]:
        # add __context here
        return ["increment_by", "__context"]

Happy to consider adding adapt_types to the framework itself (we'd use it internally), and thinking about how to ensure it doesn't loose too much. But it is extra code.

This does not work, it has the same error (understandibly so) https://peps.python.org/pep-0692/.

elijahbenizzy avatar Dec 10 '24 22:12 elijahbenizzy