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

Provide features to ease static type checking #658

Closed
aleneum opened this issue Apr 25, 2024 · 14 comments
Closed

Provide features to ease static type checking #658

aleneum opened this issue Apr 25, 2024 · 14 comments
Assignees

Comments

@aleneum
Copy link
Member

aleneum commented Apr 25, 2024

Since transitions uses runtime decoration quite excessively static type checkers have a hard time to analyse trigger and convenience functions. This has been discussed in previous issues:

Functions decorators or wrapper functions might help here:

def MyModel:

    @transition(src="A", dest="B", unless=[...])
    def my_trigger(self):
        """A transition event"""
    
    another_trigger = transition(src="B", dest="C", condition=[...])

Furthermore one could consider a new flag that attempts to create convenience functions that follow common style guides. See for instance #385 where the use of enums result in functions such as is_ERROR that most linters will complain about.

@aleneum aleneum self-assigned this Apr 25, 2024
@sy-be
Copy link

sy-be commented May 3, 2024

Thanks! It'd also help if the code would be typed in general

@aleneum
Copy link
Member Author

aleneum commented May 3, 2024

Hello @sy-bee,

Thanks! It'd also help if the code would be typed in general

currently type information are stored in stub (*.pyi) files for backward compatibility reasons. Frameworks like MyPy are able to consider stub files while type checking. Some IDEs (like PyCharm) usually also consider stub files without issues. Some other IDEs (e.g. VSCode) only consider stub files of imported functions (see picture below).

vscode_stub_example

@sy-be
Copy link

sy-be commented May 3, 2024

Thank you for quick responese! I see it now, in my case I am using VScode with mypy and as you mentioned - VSCode does not always consider stub files. Now the issue I'm having (apart from mentioned runtime decorated functions) is related to subclassing AsyncTransition in order to override its _change_state() method. In my override method I keep it mostly as is apart from one extra call. The method signature remains the same and no matter how I try to annotate its return type - mypy returns errors. For clarity, I've kept the method as is:

from transitions import EventData
from transitions.extensions.asyncio import AsyncTransition


class MyTransition(AsyncTransition):
    async def _change_state(self, event_data: EventData) -> None:
        if hasattr(event_data.machine, "model_graphs"):
            graph = event_data.machine.model_graphs[id(event_data.model)]
            graph.reset_styling()
            graph.set_previous_transition(self.source, self.dest)
        await event_data.machine.get_state(self.source).exit(event_data)
        event_data.machine.set_state(self.dest, event_data.model)
        event_data.update(getattr(event_data.model, event_data.machine.model_attribute))
        await event_data.machine.get_state(self.dest).enter(event_data)

with the return type set to None mypy returns:

my_transition.py:6: error: Return type "Coroutine[Any, Any, None]" of "_change_state" incompatible with return type "None" in supertype "Transition"  [override]
my_transition.py:8: error: Item "None" of "Machine | None" has no attribute "model_graphs"  [union-attr]
my_transition.py:11: error: Incompatible types in "await" (actual type "Any | None", expected type "Awaitable[Any]")  [misc]
my_transition.py:11: error: Item "None" of "Machine | None" has no attribute "get_state"  [union-attr]
my_transition.py:12: error: Item "None" of "Machine | None" has no attribute "set_state"  [union-attr]
my_transition.py:13: error: Item "None" of "Machine | None" has no attribute "model_attribute"  [union-attr]
my_transition.py:14: error: Incompatible types in "await" (actual type "Any | None", expected type "Awaitable[Any]")  [misc]
my_transition.py:14: error: Item "None" of "Machine | None" has no attribute "get_state"  [union-attr]

I've tried different return type, e.g. with Coroutine[Any, Any, None] I get

...
my_transition.py:7: error: Return type "Coroutine[Any, Any, Coroutine[Any, Any, None]]" of "_change_state" incompatible with return type "Coroutine[Any, Any, None]" in supertype "AsyncTransition"  [override]
my_transition.py:7: error: Return type "Coroutine[Any, Any, Coroutine[Any, Any, None]]" of "_change_state" incompatible with return type "None" in supertype "Transition"  [override]
...

This is apart from the rest of the typing issues with missing attributes.

Could it be that stub files are outdated? Thank you!

@aleneum
Copy link
Member Author

aleneum commented May 7, 2024

Hi @sy-be,

Could it be that stub files are outdated? Thank you!

they are not outdated but could use improvements. As transitions was initially built rather "duck-ish" and very agnostic towards passed arguments, "post-mortem typing" isn't trivial. Furthermore, it reveals some weaknesses the inheritance approach which heavily relies on overriding has (e.g. sync to async or Liskov substitution principle violations) that are not easy to fix. That's also a good thing but requires some refactoring.

I made some minor changes in master (EventData.machine is not optional any longer, Transitions.dest might be None) and when you are okay with ignoring override issues, this should appease mypy.

from transitions.extensions.asyncio import AsyncTransition, AsyncEventData


class MyTransition(AsyncTransition):
    async def _change_state(self, event_data: AsyncEventData) -> None:   # type: ignore[override]
        assert self.dest is not None
        if hasattr(event_data.machine, "model_graphs"):  # [1]
            graph = event_data.machine.model_graphs[id(event_data.model)]
            graph.reset_styling()
            graph.set_previous_transition(self.source, self.dest)
        await event_data.machine.get_state(self.source).exit(event_data)
        event_data.machine.set_state(self.dest, event_data.model)
        event_data.update(getattr(event_data.model, event_data.machine.model_attribute))
        await event_data.machine.get_state(self.dest).enter(event_data)

However, I guess that block [1] will be moved somewhere else because the graph should be updated even if no state change happened during an internal transition. _state_change should only be called if an actual state change is happening. My first guess would be to add a graph update function to machine.before_state_change or machine.after_state_change.

@sy-be
Copy link

sy-be commented May 7, 2024

Excellent! Thanks for your input! I like the library a lot, with better typing it'd be even better. Any plans on releasing these changes? I'm working on adding types to a large project and ideally would love to minimise the amount of type: ignores. There aren't many related to transitions, mostly this subclass I mentioned above and the attr-defined ones - which I can workaround with a decorator theoretically.

@aleneum
Copy link
Member Author

aleneum commented May 8, 2024

Hi @sy-be,

Any plans on releasing these changes?

end of this week. I need to check open issues and see whether there are some major issues to be tackled. features will be postponed to the next release though.

@aleneum
Copy link
Member Author

aleneum commented Jun 13, 2024

Hello @sy-be, @quantitative-technologies, @lucaswhipple-sfn! I am working on some utilities that hopefully ease the work with transitions when doing static type checks. The current state of the process can be found in the dev-experimental-model-creation branch.

Here is the relevant section from the documentation:


Typing support

As you probably noticed, transitions uses some of Python's dynamic features to give you handy ways to handle models. However, static type checkers don't like model attributes and methods not being known before runtime. Historically, transitions also didn't assign convenience methods already defined on models to prevent accidental overrides.

But don't worry! You can use the machine constructor parameter model_override to change how models are decorated. If you set model_override=True, transitions will only override already defined methods. This prevents new methods from showing up at runtime and also allows you to define which helper methods you want to use.

from transitions import Machine

# Dynamic assignment
class Model:
    pass

model = Model()
default_machine = Machine(model, states=["A", "B"], transitions=[["go", "A", "B"]], initial="A")
print(model.__dict__.keys())  # all convenience functions have been assigned
# >> dict_keys(['trigger', 'to_A', 'may_to_A', 'to_B', 'may_to_B', 'go', 'may_go', 'is_A', 'is_B', 'state'])
assert model.is_A()  # Unresolved attribute reference 'is_A' for class 'Model'


# Predefined assigment: We are just interested in calling our 'go' event and will trigger the other events by name
class PredefinedModel:
    # state (or another parameter if you set 'model_attribute') will be assigned anyway 
    # because we need to keep track of the model's state
    state: str

    def go(self) -> bool:
        raise RuntimeError("Should be overridden!")

    def trigger(self, trigger_name: str) -> bool:
        raise RuntimeError("Should be overridden!")


model = PredefinedModel()
override_machine = Machine(model, states=["A", "B"], transitions=[["go", "A", "B"]], initial="A", model_override=True)
print(model.__dict__.keys())
# >> dict_keys(['trigger', 'go', 'state'])
model.trigger("to_B")
assert model.state == "B"

If you want to use all the convenience functions and throw some callbacks into the mix, defining a model can get pretty complicated when you have a lot of states and transitions defined.
The method generate_base_model in transitions can generate a base model from a machine configuration to help you out with that.

from transitions.experimental.utils import generate_base_model
simple_config = {
    "states": ["A", "B"],
    "transitions": [
        ["go", "A", "B"],
    ],
    "initial": "A",
    "before_state_change": "call_this",
    "model_override": True,
} 

class_definition = generate_base_model(simple_config)
with open("base_model.py", "w") as f:
    f.write(class_definition)

# ... in another file
from transitions import Machine
from base_model import BaseModel

class Model(BaseModel):  #  call_this will be an abstract method in BaseModel

    def call_this(self) -> None:
        # do something  

model = Model()
machine = Machine(model, **simple_config)

Where I need your feedback

I plan to add some model decorators that enables the definition of trigger/events in a model class. I have two approaches: a) via function decorators and b) via functions that return a callable placeholder. This is how they could look like:

from transitions.experimental.utils import trigger_decorator, trigger, transition, with_trigger_decorator
from transitions import Machine


class Model:

    @trigger_decorator(source="A", dest="B")  # version A
    @trigger_decorator(source="B", dest="C")
    def foo(self):
        raise RuntimeError("Trigger was not initialized correctly.")

    bar = trigger(transition(source="B", dest="A", conditions=lambda: False),
                  transition(source="B", dest="A"))  # version B


@with_trigger_decorator
class MyMachine(Machine):
    pass


model = Model()
machine = MyMachine(model, states=["A", "B"], initial="A")
model.foo()
model.bar()
assert model.state == "A"

What do you think?

Does this look useful to you?

Which version to you prefer?

What do you think about model_override and generate_base_model?

Also let me know if you have some further ideas that would ease the work with transitions when it comes to typing and beyond 👋

@lucaswhipple-sfn
Copy link

lucaswhipple-sfn commented Jun 13, 2024

Excellent! Thank you for this consideration.

I quite like the idea of generate_base_model - to make a static reference object that can be easily modified either at runtime or in a CI step is very useful - I currently update my diagram as a CI Step in my pipeline, and could imagine doing the same for generate_base_model to update my static, reference model configuration.

For better or worse, I generally don't make a separate Model class when defining my state machine - I typically just override the __init__ method and do things there, which may be anathemic to your design but is what I ended up doing. For this reason, model_override does not seem useful for MY use case, though others I'm sure would appreciate it.

My $.02 on Version A vs. Version B is that version A seems to have a method definition syntax that is less hidden to me that the magic of declaring a trigger as a class variable (which is what I think version B is doing?) but my limited knowledge of the behind-the-scenes magic may be compounding my opinion.

Thank you for doing this work and I hope my feedback helps.

@aleneum
Copy link
Member Author

aleneum commented Jun 14, 2024

Hello @lucaswhipple-sfn and thank you for the feedback.

I generally don't make a separate Model class when defining my state machine - I typically just override the init method and do things there

do you mean you inherit from (Hierarchical/Async)Machine?

For this reason, model_override does not seem useful for MY use case, though others I'm sure would appreciate it.

It depends. Machine.model_override is checked inside Machine._checked_assignment so whenever a model (or the machine itself) is decorated with convenience functions (e.g. when adding a transition with a new trigger or a new state or a new model). So even if you inherit from a Machine: If you don't pass a dedicated model to super it will treat machine as a model and chose the decoration strategy based on model_override:

from transitions import Machine


# You could also use a BaseModel here
# class MyMachine(BaseModel, Machine):
class MyMachine(Machine):
    def __init__(self):
        super(MyMachine, self).__init__(states=["A", "B"], initial="A", model_override=True)
        self.add_transition("go", "A", "B")

    def go(self) -> bool:
        raise RuntimeError("Should be overridden!")

    def is_A(self) -> bool:
        raise RuntimeError("Should be overridden!")


m = MyMachine()
assert m.go()
assert not m.is_A()
# m.is_B()  # AttributeError: 'is_B' does not exist on

My $.02 on Version A vs. Version B is that version A seems to have a method definition syntax that is less hidden to me that the magic of declaring a trigger as a class variable (which is what I think version B is doing?)

You are right. Version B more or less returns a Callable placeholder to a class variable. IDEs and language servers should treat version A and B almost as the same. VSCode highlights version B as a variable though.

I also prefer version A when it comes to comprehensibility. When many transitions need to be defined, version B will be more compact as you don't need empty lines between definitions and could also add multiple transitions in one line.

@sy-be
Copy link

sy-be commented Jun 14, 2024

Thank you for your work! I like the model_override parameter, I prefer to define my methods and their types and then call them directly or via trigger().

On generate_base_model - it's useful in certain scenarios, but I generally don't use autogenerated code in my code, it introduces ambiguity and extra steps to ensure the generated code is up to date.

As per version A or B for triggers, I'd generally prefer version A as it's more clearly defined and improves visibility. However in the project I'm working on, historically the whole model was designed in a very unusual way, so I doubt I'll be able to use any of these approaches unless I refactor the code (which I hope to do one day!).

On another note

I tried running from the branch and also on version 0.9.1, I now get extra mypy warnings like:

  • Argument 1 to "get" of "dict" has incompatible type "str | None"; expected "str" [arg-type] - this is because Transition.dest can sometimes be None. How can we transition if there's no destination?
  • On the following function:
class MyTransition(AsyncTransition):
    async def _change_state(self, event_data: AsyncEventData) -> None:   # type: ignore[override]
        assert self.dest is not None
        if hasattr(event_data.machine, "model_graphs"):
            graph = event_data.machine.model_graphs[id(event_data.model)]
            graph.reset_styling()
            graph.set_previous_transition(self.source, self.dest)
        await event_data.machine.get_state(self.source).exit(event_data)
        event_data.machine.set_state(self.dest, event_data.model)
        event_data.update(getattr(event_data.model, event_data.machine.model_attribute))
        await event_data.machine.get_state(self.dest).enter(event_data)

I get errors (line numbers aligned):

8: error: "exit" of "State" does not return a value (it only ever returns None)  [func-returns-value]
9: error: Argument 1 to "set_state" of "Machine" has incompatible type "str | None"; expected "str | Enum | State"  [arg-type]
11: error: "enter" of "State" does not return a value (it only ever returns None)  [func-returns-value]
11: error: Argument 1 to "get_state" of "Machine" has incompatible type "str | None"; expected "str | Enum"  [arg-type]

lines 9 and the last 11 are related to the above - transition.dest potentially being None
other lines are due to Machine.get_state and Machine.set_state do not return any awaitable (they are normal functions)

This second issue was there before, I guess the code needs updating as well as pyi files.

@aleneum
Copy link
Member Author

aleneum commented Jun 14, 2024

... is because Transition.dest can sometimes be None. How can we transition if there's no destination?

internal transitions have no destination 1. In this case _change_state is not called. But mypy does not know that. I would have thought that assert self.dest is not None tells the type checker that self.dest cannot be None here. That's why the graph update should happen outside of _change_state as internal transition are valid transitions and should update the graph (but obviously not change states).

The other issues look like incomplete typing for 'protected' functions. I'll have a look.

@aleneum
Copy link
Member Author

aleneum commented Jun 17, 2024

lines 9 and the last 11 are related to the above - transition.dest potentially being None
other lines are due to Machine.get_state and Machine.set_state do not return any awaitable

What's your type checking setup?

VSCode (Pylance?) can infer from assert self.dest is not None that self.dest is a string. PyCharm does not explicitly show that but also does not complain about get_state. Furthermore, both, PyCharm and VSCode infer machine to be AsyncMachine | HierarchicalAsyncMachine. Both machines define async def enter/exit ... -> None. Afaik, async def does not require a return type to be awaitable 1. VSCode and PyCharm show exit/enter return type to be Coroutine[Any, Any, None] which looks correct to me.mypy --strict also does not complain.

@aleneum
Copy link
Member Author

aleneum commented Jun 17, 2024

I reworked the naming and signatures a bit. I wanted to keep the signatures comparable to how transitions can be defined as of now (lists of lists and dicts) but wanted to provide the convenience of Machine.add_transitions with keywords. This is my current suggestion:

from transitions.experimental.utils import with_model_definitions, event, add_transitions, transition
from transitions import Machine


class Model:

    state: str = ""

    @add_transitions(transition(source="A", dest="B"), ["C", "A"])
    @add_transitions({"source": "B",  "dest": "A"})
    def foo(self): ...  # version A

    bar = event(
        {"source": "B", "dest": "A", "conditions": lambda: False},
        transition(source="B", dest="C")
    )  # version B


@with_model_definitions
class MyMachine(Machine):
    pass


model = Model()
machine = MyMachine(model, states=["A", "B", "C"], initial="A")
model.foo()
model.bar()
assert model.state == "C"
model.foo()
assert model.state == "A"

@add_transitions can be stacked. Both, event and add_transitions expect one or more transition definitions that can be either a list, a dict or the result of the helper function transition which is also a dict.

aleneum added a commit that referenced this issue Jun 18, 2024
…ils.generate_base_model`

When `model_override` is set to True, Machine will assign only methods to a model that are already defined. This eases static type checking (#658) and enables tailored helper function assigment. Default value is False which prevents override of already defined model functions.
aleneum added a commit that referenced this issue Jun 18, 2024
…ns, event, with_model_definitions, transition}

Add helper functions to define transitions on a model class for better type checking (#658)
aleneum added a commit that referenced this issue Jun 18, 2024
…ils.generate_base_model`

When `model_override` is set to True, Machine will assign only methods to a model that are already defined. This eases static type checking (#658) and enables tailored helper function assigment. Default value is False which prevents override of already defined model functions.
aleneum added a commit that referenced this issue Jun 18, 2024
…ns, event, with_model_definitions, transition}

Add helper functions to define transitions on a model class for better type checking (#658)
@aleneum
Copy link
Member Author

aleneum commented Jun 18, 2024

The mentioned features have been merged into master. I will close this issue. If you face other obstacles -- typing-related or other -- please open a new issue. Thanks again for your feedback!

@aleneum aleneum closed this as completed Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants