Add property for optional runner

[ogenstad]

The runner argument to Nornir is defined as being optional but we later use it as if it's always set. Adding a property that returns a RunnerPlugin or raises an error, with this we can also remove the strict_optional = False bypass in the mypy config.

[dbarrosop]

I wonder if it's better to change the signature instead

[ogenstad]

That would be cleaner for sure, I didn't want to change anything that could potentially break someones workflow. I don't know if it would be an issue but potentially someone could do something like this now:

nr = Nornir()
my_nr = nr.with_runner(runner=xyz)

My guess is that most people would be using InitNornir where the runner is always set but I don't know if this would be considered enough of an issue?

Alternatively load_runner could be moved so that it's called here again if runner isn't set.

self.runner = runner or load_runner(self.config)

[dbarrosop]

I honestly would consider this a bug. It's also a typing issue so it will only break linters/development, it shouldn't break the runtime, should it?

[ogenstad]

It's a typing issue but depending on how it's solved it could be problematic in some use cases.

    def __init__(
        self,
        inventory: Inventory,
        config: Optional[Config] = None,
        data: Optional[GlobalState] = None,
        processors: Optional[Processors] = None,
        runner: Optional[RunnerPlugin] = None,
    ) -> None:
        self.data = data if data is not None else GlobalState()
        self.inventory = inventory
        self.config = config or Config()
        self.processors = processors or Processors()
        self.runner = runner

The cleanest way to change this would be to make runner required as below, a potential problem here is if anyone today uses Nornir() without a runner and then adds one later (it's possible this never happens but hard to know):

    def __init__(
        self,
        inventory: Inventory,
        runner: RunnerPlugin,       <--- Change
        config: Optional[Config] = None,
        data: Optional[GlobalState] = None,
        processors: Optional[Processors] = None,
    ) -> None:
        self.data = data if data is not None else GlobalState()
        self.inventory = inventory
        self.config = config or Config()
        self.processors = processors or Processors()
        self.runner = runner

An alternative would be:

    def __init__(
        self,
        inventory: Inventory,
        config: Optional[Config] = None,
        data: Optional[GlobalState] = None,
        processors: Optional[Processors] = None,
        runner: Optional[RunnerPlugin] = None,
    ) -> None:
        self.data = data if data is not None else GlobalState()
        self.inventory = inventory
        self.config = config or Config()
        self.processors = processors or Processors()
        self.runner = runner or load_runner(self.config)       <--- Change

[dbarrosop]

got it, I am starting to think the proposal on this PR is probably the best option. I don't like it is a runtime issue but we'd have to change the signature for this to be a "clean" fix as that's probably not ideal...

[dbarrosop]

shouldn't we do the opposite? store in _runner and have the propery be runner?

[ogenstad]

I tried that first however it breaks some other things, in the below functions we use self.__dict__ which causes issues with an attributed such as self._runner as we'd send in "_runner" as an argument to Nornir instead of "runner".

    def with_processors(self, processors: List[Processor]) -> "Nornir":
        """
        Given a list of Processor objects return a copy of the nornir object with the processors
        assigned to the copy. The orinal object is left unmodified.
        """
        return Nornir(**{**self.__dict__, **{"processors": Processors(processors)}})

    def with_runner(self, runner: RunnerPlugin) -> "Nornir":
        """
        Given a runner return a copy of the nornir object with the runner
        assigned to the copy. The orinal object is left unmodified.
        """
        return Nornir(**{**self.__dict__, **{"runner": runner}})

    def filter(self, *args: Any, **kwargs: Any) -> "Nornir":
        """
        See :py:meth:`nornir.core.inventory.Inventory.filter`

        Returns:
            :obj:`Nornir`: A new object with same configuration as ``self`` but filtered inventory.
        """
        b = Nornir(**self.__dict__)
        b.inventory = self.inventory.filter(*args, **kwargs)
        return b

What we could do instead is to replace self.__dict__ in these cases with a property that returns a dictionary where the keys are hardcoded instead, i.e: inventory, data, config, runner, processors.

[dbarrosop]

yeah, either that or just a cleaner clone method

[ogenstad]

I can update the PR to add something like that. Within that property or method I will have to call self._runner but that might be the best option for now. Would be nice too then go back and delete this when there's a new major version to clean it up a bit..