CI failing due to typing issues

[Nikoleta-v3]

Hello everyone 👋🏻 I hope you are enjoying the holidays.

Currently the CI is failing https://github.com/Axelrod-Python/Axelrod/actions/runs/3773670578/jobs/6415295764, because of typing issues. I had a look and apparently there are two types of issues:

1. “Incompatible default for argument”

For example this error occurs in finite_state_machines.py at the __init__ of class EvolvableFSMPlayer:

class EvolvableFSMPlayer(FSMPlayer, EvolvablePlayer):
    """Abstract base class for evolvable finite state machine players."""
   ...
    def __init__(
        self,
        transitions: tuple = None,
        initial_state: int = None,
        initial_action: Action = None,
        num_states: int = None,
        mutation_probability: float = 0.1,
        seed: int = None,
    ) -> None:

Here we declare that transitions is a tuple but set None as a default value and mypy is not happy with this. I believe there are two ways to fix the above, we either give some default values that have are of correct type or we can use the Optional type modifier as described on the mypy documentation: https://mypy.readthedocs.io/en/stable/kinds_of_types.html.

For example if we tweak the __init__ class to be as follows:

    def __init__(
        self,
        transitions: Optional[tuple] = None,
        initial_state: Optional[int] = None,
        initial_action: Optional[Action] = None,
        num_states: Optional[int] = None,
        mutation_probability: float = 0.1,
        seed: Optional[int] = None,
    ) -> None:

the tests pass.

2. "Missing return statement"

These errors occur because all the return statements are under if statements, and mypy is complaining that there is no return statement in case all of the if statements fail. This can be fixed by slightly changing the code.

I am happy to work on this 👍🏻 could you please let me know what you prefer regarding the errors “Incompatible default for argument”?

[marcharper]

In this specific case the arguments are truly optional since the strategy generates a random set of parameters (given the seed) if a non-sufficient set of arguments is given. If we were to specify default options of the correct type it would change this behavior.

However there is a non-trivial collection of which combinations parameters can be specified or not (see the logic in the code here), so each argument isn't truly independently optional. It may be a good idea to better factor out these behaviors at some point into an auxiliary class that handles the parameters and associated logic for generating new values (and mutated existing ones).

[Nikoleta-v3]

However there is a non-trivial collection of which combinations parameters can be specified or not (see the logic in the code here), so each argument isn't truly independently optional.

Yup, you are right.

It may be a good idea to better factor out these behaviors at some point into an auxiliary class that handles the parameters and associated logic for generating new values (and mutated existing ones).

Just to make sure I understood you correctly: you suggest that we create an auxiliary class that would be responsible for generating random inputs (regarding the player class) when arguments are optional and not given by the user, and also to be able to mutate default values?

[marcharper]

you suggest that we create an auxiliary class that would be responsible for generating random inputs (regarding the player class) when arguments are optional and not given by the user, and also to be able to mutate default values?

Yes, I'm suggesting that the parameters, how they are randomly generated, and how they mutate would make more sense as a separate class rather than intermingled with the strategy code. You could imagine the Player class asking the auxiliary class:

• if the parameters are sufficiently specified on __init__ (with the derived parameters generated as needed)
• if the parameters have nontrivial mutation options
• to generate a new variant

I haven't thought through all the implications.

This suggestion could also resolve issue #1345 -- the parameter holding class could have a default no-op mutation case (alternatively so could the Player class...). These could be good candidates for using dataclasses or the attrs library, which play nicely with mypy.

Anyway that's separate from the immediate typing issues, just wanted to mention it as something to think about in the context of whether the parameters are truly optional or not, since at least some of them have to be specified.