new API ideas

[aryaniyaps]

This discussion has been made in good faith. I am not forcing that all of these changes must be implemented in Strawberry. These are rather my opinions and suggestions, from my experience with the library.

pythonic dataloader API

The current dataloader implementation is a port of the js version, which is lacking a few features:

• passing in a custom cache map
  • (default) in memory cache map - dictionary
  • LRU cache map
• passing in a custom cache key function
• passing in a custom batch schedule function
• methods to play with the cache map, like prime, clear, clear_all
• calling asyncio.gather everytime is verbose, we also need a load_many method

Javascript is a callback-based language. Therefore, the following javascript code:

const myLoader = new DataLoader(async keys => {
    // do something with the keys
    return keys
  }
);

would be equivalent to the following python code:

async def load_fn(keys):
    # do something with the keys
    return keys

my_loader = DataLoader(load_fn=load_fn)

IMO this isn't pythonic. In python, you cannot define functions dynamically, like javascript. Therefore, the load function will be lying around somewhere in the air. We could make the API more pythonic by making the load function a method on the dataloader, like this:

class MyLoader(DataLoader):
    async def perform_load(self, keys):
        # do something with the keys
        return keys

loader = MyLoader()
await loader.load(1)

I have already got a dataloader implementation ready locally with all these features, if you're okay with these changes I can submit a PR! :)
this implementation has been inspired by Shopify's graphql-batch gem.

Here's the full API I've been working on!

# batching.py

from __future__ import annotations

from asyncio import create_task, gather, get_event_loop
from asyncio.events import AbstractEventLoop
from asyncio.futures import Future
from dataclasses import dataclass
from inspect import isawaitable
from typing import Any, Awaitable, Callable, Generic, MutableMapping, Sequence, TypeVar

__all__ = ("BatchLoader",)

KT = TypeVar("KT")
VT = TypeVar("VT")
CT = TypeVar("CT")


@dataclass
class Batch(Generic[KT, VT]):
    """
    The :class:`Batch` is an abstraction layer that
    holds load keys and their corresponding futures
    for a batch-loader. Every batch-loader has only
    one :class:`Batch` at a given time.
    """

    #: Whether the batch has been dispatched
    #: by it's batch-loader yet.
    has_dispatched: bool

    #: The load keys that the batch holds.
    load_keys: list[KT]

    #: The futures corresponding to the load
    #: keys that the batch holds.
    futures: list[Future[VT]]

    def add_task(self, load_key: KT, future: Future[VT]) -> None:
        """
        Adds the given load_key and it's future to the batch.

        :param load_key: The key to add to the batch.
        :param future: The future to add to the batch.
        """
        self.load_keys.append(load_key)
        self.futures.append(future)


class BatchLoader(Generic[KT, VT, CT]):
    """
    The :class:`BatchLoader` creates an abstraction layer for
    loading data from a particular data source with unique keys
    such as the `ID` column of a SQL table or document name in a
    MongoDB database, given a batch loading function. Each
    :class:`BatchLoader` instance contains a unique memoized cache,
    which improves performance when multiple load calls are made.
    """

    #: An implementation of the :class:`MutableMapping
    #: <collections.abc.MutableMapping>` abstract base class, to
    #: use for memoization caching. Setting this to None disables caching.
    cache_map: MutableMapping[CT, Awaitable[VT]] | None = {}

    #: An abstract event loop implementation to use for batching.
    #: Uses the standard asyncio event loop by default.
    event_loop: AbstractEventLoop | None = None

    #: The current :class:`Batch` instance of the batch-loader.
    batch: Batch | None = None

    #: Limits the number of items that get passed into the batch
    #: loading function. Setting this to None implies that there
    #: is no limit on the batch size.
    max_batch_size: int | None = None

    async def perform_load(self, keys: Sequence[KT]) -> Sequence[VT]:
        """
        Loads data from a particular datasource.
        This method must be overridden when subclassing.

        :param keys: The load keys to extract data for.
        :return: A sequence of values corresponding to the
            given sequence of load keys.
        """
        raise NotImplementedError

    @staticmethod
    def cache_key_for(load_key: KT) -> Any:
        """
        Produces a cache key for the given load key.
        This method must be overridden to use a different
        cache key than the load key.

        :param load_key: The key to produce a cache key for.
        :return: The cache key for the given load key.
        """
        return load_key

    def schedule_batch(self, callback: Callable, *args: Any) -> None:
        """
        Schedules the later execution of a given batch.
        The callable is expected to call the provided callback
        in the immediate future. This method must be overridden
        to use a different scheduler implementation.

        :param callback: The callback which executes the batch.
        :param args: The arguments to be passed to the callback.
        """
        self.get_event_loop().call_soon(callback, *args)

    def load(self, key: KT) -> Awaitable[VT]:
        """
        Loads a key using the given batch-loading function.

        Here's an example:

        .. code-block:: python

            from asyncio import gather, sleep
            from typing import Sequence

            from ceres.batching import BatchLoader


            class UserLoader(BatchLoader):
                async def perform_load(usernames: Sequence[str]) -> Sequence[str]:
                    # loading call goes here.
                    await sleep(0.25)
                    return usernames

            user_loader = UserLoader()
            user = await user_loader.load("alpha")

        :param key: The key to be loaded.

        :return: A :class:`Future <asyncio.Future>` resulting in
            the value represented by the key.
        """
        cache_key = self.cache_key_for(key)
        if self.cache_map:
            future = self.cache_map.get(key=cache_key)
            if future is not None:
                return future
        future = self.get_event_loop().create_future()
        if self.cache_map:
            self.cache_map[cache_key] = future
        self.current_batch.add_task(key, future)
        return future

    def load_many(self, keys: Sequence[KT]) -> Awaitable[Sequence[VT]]:
        """
        Loads multiple keys using the given batch-loading function.
        This is equivalent to calling the :func:`load` method multiple
        times using :func:`asyncio.gather`, which is more verbose:

        .. code-block:: python

            from asyncio import gather, sleep
            from typing import Sequence

            from ceres.batching import BatchLoader


            class UserLoader(BatchLoader):
                async def perform_load(usernames: Sequence[str]) -> Sequence[str]:
                    # loading call goes here.
                    await sleep(0.25)
                    return usernames

            user_loader = UserLoader()
            users = await gather(
                user_loader.load("alpha"),
                user_loader.load("beta")
            )

        Alternatively, you could use the :func:`load_many` method, which
        is more convenient:

        .. code-block:: python

            from asyncio import gather, sleep
            from typing import Sequence

            from ceres.batching import BatchLoader


            class UserLoader(BatchLoader):
                async def perform_load(usernames: Sequence[str]) -> Sequence[str]:
                    # loading call goes here.
                    await sleep(0.25)
                    return usernames

            user_loader = UserLoader()

            # load multiple values at once.
            users = await user_loader.load_many(("alpha", "beta",))

        :param keys: The keys to be loaded.

        :return: A :class:`Future <asyncio.Future>` resulting
            in a sequence of values.
        """
        tasks = [self.load(key=key) for key in keys]
        return gather(*tasks)

    def prime(self, key: KT, value: VT | Exception) -> BatchLoader:
        """
        Sets the given key-value pair on the cache
        map, if the cache key doesn't already exist.

        Here's an example:

        .. code-block:: python

            from asyncio import gather, sleep
            from typing import Sequence

            from ceres.batching import BatchLoader


            class UserLoader(BatchLoader):
                async def perform_load(usernames: Sequence[str]) -> Sequence[str]:
                    # loading call goes here.
                    await sleep(0.5)
                    return usernames

            user_loader = UserLoader()

            # prime the batch-loader's memoization cache.
            user_loader.prime(key="alpha", value="alpha")

            # we hit the cache when loading the `alpha` key.
            alpha = await user_loader.load("alpha")

        :param key: The key to be set on the cache map.

        :param value: The value to set for the cache key.

        :return: The batch-loader for method cascading.
        """
        if self.cache_map:
            cache_key = self.cache_key_for(key)
            if cache_key not in self.cache_map:
                # check if the cache key exists in the cache
                # map already, and only set the key if it does
                # not already exist.
                future = self.get_event_loop().create_future()
                if isinstance(value, Exception):
                    future.set_exception(value)
                else:
                    future.set_result(value)
                self.cache_map[cache_key] = future
        return self

    def clear(self, key: KT) -> BatchLoader:
        """
        Wipes the given key from the cache map.

        It's a common pattern to clear bad data
        from the cache in-case an exception is thrown:

        .. code-block:: python

            from asyncio import gather, sleep
            from typing import Sequence

            from ceres.batching import BatchLoader


            class UserLoader(BatchLoader):
                async def perform_load(usernames: Sequence[str]) -> Sequence[str]:
                    if "alpha" in usernames:
                        throw Exception("Cannot perform load call!")
                    # loading call goes here.
                    await sleep(0.5)
                    return usernames

            try:
                # try to load using the batch-loader.
                user_loader = UserLoader()
                alpha = await user_loader.load("alpha")
            except Exception:
                # clear the loaded key if an exception arises.
                user_loader.clear("alpha")

        :param key: The cache key to clear.

        :return: The batch-loader for method cascading.
        """
        if self.cache_map:
            cache_key = self.cache_key_for(key)
            self.cache_map.pop(cache_key, None)
        return self

    def clear_all(self) -> BatchLoader:
        """
        Wipes out the batch-loader's cache map.

        :return: The batch-loader for method cascading.
        """
        if self.cache_map:
            self.cache_map.clear()
        return self

    def get_event_loop(self) -> AbstractEventLoop:
        """
        Gets the event loop to be used by the batch-loader.

        :return: The event loop for the batch-loader.
        """
        if self.event_loop is None:
            return get_event_loop()
        return self.event_loop

    @property
    def current_batch(self) -> Batch:
        """
        Either gets the current batch or creates and schedules
        a dispatch of a new batch for the batch-loader instance.

        :return: The current batch for the batch-loader.
        """
        if (
            self.batch is not None
            and not self.batch.has_dispatched
            and (
                self.max_batch_size is None
                or len(self.batch.load_keys) < self.max_batch_size
            )
        ):
            return self.batch

        batch: Batch = Batch(has_dispatched=False, load_keys=[], futures=[])
        self.batch = batch
        self.schedule_batch(
            create_task,
            self.dispatch_batch(
                batch=batch,
            ),
        )
        return batch

    async def dispatch_batch(self, batch: Batch[KT, VT]) -> None:
        """
        Dispatches the given batch instance, performing
        a load call with the keys it holds at once.

        :param batch: The batch to dispatch.
        """
        batch.has_dispatched = True
        batch_result = self.perform_load(batch.load_keys)
        if not batch_result or not isawaitable(batch_result):
            exception = TypeError(
                f"BatchLoader must be constructed with a function which accepts "
                f"Sequence[key] and returns Awaitable[Sequence[value]], but the "
                f"function did not return a Awaitable object: {batch_result}."
            )
            return self.failed_dispatch(batch, exception)

        values = await batch_result
        # TODO: also allow the load call to return a dictionary.
        if not isinstance(values, Sequence):
            raise TypeError(
                f"BatchLoader must be constructed with a function which accepts "
                f"Sequence[key] and returns Awaitable[Sequence[value]], but the function "
                f"did not return a Awaitable object of Sequence[value]: {values}."
            )

        if len(values) != len(batch.load_keys):
            raise TypeError(
                "BatchLoader must be constructed with a function which accepts "
                "Sequence[key] and returns Awaitable[Sequence[value]], but the function "
                "did not return a Awaitable object of Sequence[value] of the same "
                "length as the Sequence of keys."
                + f"\n\nKeys: \n {batch.load_keys}\n\nValues: {values}"
            )

        for future, value in zip(batch.futures, values):
            if isinstance(value, Exception):
                future.set_exception(value)
            else:
                future.set_result(value)

    def failed_dispatch(self, batch: Batch[KT, VT], exception: Exception) -> None:
        # do not cache individual loads if the entire batch dispatch fails,
        # but still set_exception to each future so they do not hang.
        for key, future in zip(batch.load_keys, batch.futures):
            self.clear(key=key)
            future.set_exception(exception)

moving away from decorators

strawberry decorators take too many arguments, that they become multi-line very quickly.

import strawberry

@strawberry.type(description="""
    my multi-line description.
    """
)
class MyObjectType(MyInterface):
    my_field_str: str
    my_field_int: int

IMO, multi-line decorators don't look right in python. This is rather a personal preference / developer ergonomics related issue.
I am well aware that the core of strawberry has been built around dataclasses. Keeping this in mind, this is my alternative API suggestion:

from strawberry import ObjectType

class MyObjectType(ObjectType):
    my_field_str: str
    my_field_int: int

    class Meta:
        interfaces = (MyInterface,)
        description = """
        my multi-line description.
        """

the above API is inspired by Pydantic, which also uses dataclasses.
You could also see that interfaces are not implemented by inheritance. This is because in GraphQL, there is no concept of inheritance.
I think that this nicely lines up with that philosophy.

I believe with this approach we can also get rid of the dataclass hacks in the codebase.

[marcoacierno]

pythonic dataloader API

I think it would be nice to just be able to pass a function as DataLoader, instead of having to implement a class just for a function? What do you think?

Something like:

@strawberry.data_loader
def load_users(keys):
    return [1, 2, 3]

then you can something like

def abc(id):
	await load_users(id)

Not sure if it is possible, but I am just not 100% convinced on having a custom class just to customise a method

[aryaniyaps]

Having a class helps to keep related stuff together IMO, otherwise we will be passing a lot of functions like cache key or batch schedule functions to every dataloader.

Also, inheritance helps a ton when you have custom cache maps and stuff for every dataloader :)

[marcoacierno]

can you show me more real examples where having a class helps?

[BryceBeagle]

IMO, multi-line decorators don't look right in python. This is rather a personal preference / developer ergonomics related issue.
I am well aware that the core of strawberry has been built around dataclasses. Keeping this in mind, this is my alternative API suggestion

I agree with this 100%. I much prefer the nested Meta class system over multi-line decorators. They feel much more declarative, which is perfect for laying out a schema

[patrick91]

I prefer decorator based, mixing decorator and meta feels wrong. Also I can't really see how using a Meta class is more declarative than using a decorator :D

Using just decorators allows to use one class for multiple types too :)

[aryaniyaps]

I prefer decorator based, mixing decorator and meta feels wrong. Also I can't really see how using a Meta class is more declarative than using a decorator :D

Using just decorators allows to use one class for multiple types too :)

IMO decorators are generally used to hide repetition, for example we use decorators to check if the request is authenticated in flask/ fastapi routes.

In a way, yes, decorators make sense to be used to declare stuff, flask routes also do this like:

@app.route("/foo")
def handle_foo():
    pass

But graphql schemas can get complex and nested, and I feel it is natural to use classes to represent blocks of it.

[BryceBeagle]

Also, while I'm not completely sold on switching away from decorators (there are still many questions to be answered before that could even be considered), inheritance does have one key advantage. It makes the plan to have everything on the internal Strawberry type/object tree be an instance of StrawberryType a lot easier without a lot of hacks.

I will note for the other developers that we can still have the dataclass-style syntax (i.e. with the type annotations), but just swapping the decorator for inheritance.

[marcoacierno]

moving away from decorators

What I really like about Strawberry are actually the decorators :D
They are very easy to use, don't require special knowledge about Strawberry (a Meta class does)

I also don't understand why a interfaces = () would be better

The example you gave gives me Graphene vibes: https://docs.graphene-python.org/en/latest/types/objecttypes/ which feels like a downgrade to our easy to understand syntax. Now you need to remember, what do I need to import? ObjectType? InputObjectType? ArgumentObjectType? A simple strawberry.type strawberry.input work the same and are much easier to use

[marcoacierno]

In my opinion our direction should be to use decorators for more stuff.

Take FastAPI as example, I really like that the endpoints are defined via decorator

I think something similar for strawberry, like:

schema = strawberry.Schema()

# then in your files:
@schema.query
def query():
	pass

@schema.mutation
def mutation():
	pass

would really make life easier / better