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

Synchronous endpoints on ragna.core.Chat #212

Open
pmeier opened this issue Nov 19, 2023 · 2 comments
Open

Synchronous endpoints on ragna.core.Chat #212

pmeier opened this issue Nov 19, 2023 · 2 comments

Comments

@pmeier
Copy link
Member

pmeier commented Nov 19, 2023

In #183 I suggested to have sync endpoints on the ragna.core.Chat in addition to the async ones. This could help people that are not familiar with async programming and aren't looking for maximum performance, but rather just something that they can run easily.

I see two ways to go about this:

  1. Make Chat.prepare and Chat.answer the sync endpoints and move the current functionality to Chat.aprepare and Chat.aanswer. This would break existing code, but aligns the naming scheme with the canonical one in the Python ecosystem. Breaking stuff is not terribly bad right now, since we are still on a 0. release.
  2. Create Chat.sprepare and Chat.sanswer as sync endpoints and keep the async endpoints as is. This would make it more explicit that Ragna is async first, but would go against canonical naming and thus could lead to confusion.

I assumed this was not hard given that we have the async endpoints already. However, I've tried to implement this in #205, but couldn't get it working properly. The problem comes from the fact that I don't want to re-implement the sync endpoints when we already have the async ones. Meaning, the sync endpoints should just be thin wrappers that run the async endpoints synchronously. To make matters worse, this also needs to be supported in case we actually have an async event loop already running that the user doesn't want to use, e.g. they are programming in a jupyter notebook. So actually what we want is to synchronously run async code while potentially being inside an existing event loop.

Here is some test code that I had in #205 when going for option 1. This would need to run without any issues on all platforms for a PR to be accepted:

import asyncio
import inspect

import pytest

import ragna.core
from ragna import Rag
from ragna.assistants import RagnaDemoAssistant
from ragna.source_storages import RagnaDemoSourceStorage


async def e2e_async(*, documents, source_storage, assistant):
    async with Rag().chat(
        documents=documents,
        source_storage=source_storage,
        assistant=assistant,
    ) as chat:
        return await chat.aanswer("?")


async def e2e_sync_in_async(*, documents, source_storage, assistant):
    with Rag().chat(
        documents=documents,
        source_storage=source_storage,
        assistant=assistant,
    ) as chat:
        return chat("?")


def e2e_sync(*, documents, source_storage, assistant):
    with Rag().chat(
        documents=documents,
        source_storage=source_storage,
        assistant=assistant,
    ) as chat:
        return chat("?")


@pytest.mark.parametrize("e2e_fn", [e2e_async, e2e_sync_in_async, e2e_sync])
def test_e2e(tmp_local_root, e2e_fn):
    document_root = tmp_local_root / "documents"
    document_root.mkdir()
    document_path = document_root / "test.txt"
    with open(document_path, "w") as file:
        file.write("!\n")

    document = ragna.core.LocalDocument.from_path(document_path)

    answer = e2e_fn(
        documents=[document],
        source_storage=RagnaDemoSourceStorage,
        assistant=RagnaDemoAssistant,
    )
    if inspect.iscoroutine(answer):
        answer = asyncio.run(answer)

    assert isinstance(answer, ragna.core.Message)
    assert answer.role is ragna.core.MessageRole.ASSISTANT
    assert {source.document.name for source in answer.sources} == {document.name}

This is a superset of what we have in

def test_e2e(tmp_local_root):

which currently only tests the equivalent of e2e_async.

@pmeier pmeier added type: enhancement 💅 New feature or request help wanted Extra attention is needed area: python-api ⌨️ labels Nov 19, 2023
@nenb
Copy link
Contributor

nenb commented Nov 23, 2023

My impression is that your work in #205 was probably the recommended approach - at least that is what I got from reading this still active thread on the topic. It seems to me like doing what you have done and sticking with the async-only approach is the most straightforward approach for the moment.

I just spent a little time playing around with it in a Jupyter notebook where an event loop is already running (couldn't resist!) and here was my fairly crude attempt:

from concurrent.futures import ThreadPoolExecutor
import asyncio
import threading

async def async_func(event):
    print("Sleeping...")
    await asyncio.sleep(2)
    print("Exiting async...")
    event.set()

async def other_async_func_in_main_thread():
    print("Enter other...")
    await asyncio.sleep(1)
    print("Exiting other...")

def sync_wrapper(async_func):
    new_loop = asyncio.new_event_loop()
    ThreadPoolExecutor().submit(new_loop.run_forever)
    event = threading.Event()
    asyncio.run_coroutine_threadsafe(async_func(event), new_loop)
    print("Waiting for async to finish...")
    asyncio.create_task(other_async_func_in_main_thread())
    event.wait()
    new_loop.call_soon_threadsafe(new_loop.stop)
    print("Async done!")

@pmeier
Copy link
Member Author

pmeier commented Nov 23, 2023

Thanks a ton @nenb for the xref! That was a good read. Although there is no solution yet, I'm at least glad that this is a hard problem and I didn't miss something obvious.

Since I would expect most of the users of the Python API to either

  1. be in a jupyter notebook and thus basically only need to sprinkle in an await from time to time since they have a dormant event loop running, or
  2. build something on top of Ragna and thus should use the async API anyway

I think sync endpoints are of a lower priority for now. If we reconsider here based on user feedback we at least have your snippet now as base.

@pmeier pmeier removed the help wanted Extra attention is needed label Nov 23, 2023
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

2 participants