Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
shun-liang opened this issue Dec 5, 2024 · 4 comments
Labels

Comments

@shun-liang
Copy link
Contributor

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
@elijahbenizzy
Copy link
Contributor

elijahbenizzy commented Dec 5, 2024

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/

@shun-liang
Copy link
Contributor Author

shun-liang commented Dec 5, 2024

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.

@elijahbenizzy
Copy link
Contributor

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
Copy link
Contributor

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/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants