Literal["pickle"] type annotation

[XzzX]

Regarding the Literal["pickle"] type hint here:

pyiron_workflow/pyiron_workflow/storage.py

Lines 281 to 318 in a3d38f7

	def available_backends(
	backend: Literal["pickle"] | StorageInterface | None = None,
	only_requested: bool = False,
	) -> Generator[StorageInterface, None, None]:
	"""
	A generator for accessing available :class:`StorageInterface` instances, starting
	with the one requested.
	Args:
	backend (Literal["pickle"] | StorageInterface | None): The interface to yield
	first.
	only_requested (bool): Stop after yielding whatever was specified by
	:param:`backend`.
	Yields:
	StorageInterface: An interface for serializing :class:`Node`.
	"""
	standard_backends = {"pickle": PickleStorage}
	def yield_requested():
	if isinstance(backend, str):
	yield standard_backends[backend]()
	elif isinstance(backend, StorageInterface):
	yield backend
	if backend is not None:
	yield from yield_requested()
	if only_requested:
	return
	for key, value in standard_backends.items():
	if (
	backend is None
	or (isinstance(backend, str) and key != backend)
	or (isinstance(backend, StorageInterface) and value != backend)
	):
	yield value()

I know it is quite common in Python modules to have arguments like backend or method of type string to select what to do. However, I think a solution like this is much better:

from __future__ import annotations
from enum import, StrEnum
from typing_extensions import assert_never

class Direction(StrEnum):
    UP = 'up'
    DOWN = 'down'

def foo(a: Direction):
    print(a)

def bar(a: Direction):
    match a:
        case Direction.UP:
            print('up')
            return
        case Direction.DOWN:
            print('down')
            return
    assert_never(a)


foo(Direction.DOWN)
bar(Direction.DOWN)

1. Type hint restricts to only available types.
2. If I add another direction every type hint will be updated automatically. I do not have to touch everything myself.
3. mypy can catch non-exhaustive match cases or if-else cascades, aka I forgot to add the new type there.

Anyhow, I do not see why this function needs to be so complex in the first place. I think enforcing node.save(PickleStorage()) and not allowing node.save("pickle") is reasonable. It also avoids the obscured initialization of the PickleStorage. If we want to keep the interface, what about

from enum import Enum

class StorageInterfacesEnum(Enum):
    pickle: PickleStorage

def available_backends(
    backend: StorageInterfacesEnum | type[StorageInterface] | None = None,
    only_requested: bool = False,
) -> Generator[StorageInterface, None, None]::
    backend = backend.value if isinstance(backend, StorageInterfacesEnum) else backend

    standard_backends = [PickleStorage]

    if item is not None:
        yield item()
        if only_requested:
            return

    yield from (selected_backend() for selected_backend in standard_backends if selected_backend!=backend)

However, this requires node.save(StorageInterfacesEnum.pickle)

[liamhuber]

So first, at a high level, you get no disagreement from me. I prefer to avoid strings, and I find your StrEnum solution somewhat superior.

For some context, you're right that the string options like this do tend to appear regularly in python code. In our more specific community, there is a "pyironic" aversion to imports. Both node.save(StorageInterfacesEnum.pickle) and ode.save(PickleStorage()) require additional imports, or to slap these objects onto the namespace of the Workflow object. I don't think this is an impermeable barrier, just a speedbump here.

The fact we have the argument at all is a backwards-compatibility step to an earlier period when there was indeed more options ("h5io"). So, for now, if we're going to make any changes I would propose to simply accept backend: StorageInterface | None = None with None internally creating a PickleStorage(), and throw away the backend dispatching stuff altogether until (if!) it becomes necessary again.

[XzzX]

I think I have to change my opinion about None as a default argument. My initial thought was, why not use backend: StorageInterface = PickleStorage(). In C++, this would be perfectly fine. But, well, does not not work in python, of course. The way python handles mutable default arguments is so annoying in my opinion.

[XzzX]

BTW or (isinstance(backend, StorageInterface) and value != backend) should be always true as value is a type and backend is an instance.

[XzzX]

Let's see if I can make mypy detect this.

[XzzX]

Tada
mypy --ignore-missing-imports --strict-equality pyiron_workflow/storage.py
gives

pyiron_workflow/storage.py:316: error: Non-overlapping equality check (left operand type: "type[PickleStorage]", right operand type: "StorageInterface")  [comparison-overlap]

We should enable more checks :) Do not compare apples with oranges.