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

Add a chat-identifier endpoint #339

Open
nenb opened this issue Mar 4, 2024 · 9 comments
Open

Add a chat-identifier endpoint #339

nenb opened this issue Mar 4, 2024 · 9 comments
Labels
type: enhancement 💅 New feature or request

Comments

@nenb
Copy link
Contributor

nenb commented Mar 4, 2024

Feature description

I would like to propose a new API endpoint that will only return a list of previous chat IDs. More specifically, the endpoint would return a list of mappings from chat ID to chat name for a user.

Value and/or benefit

This is useful functionality for interacting with specific historical chats. For example, if I would like to only show messages from a certain chat. Currently, it's necessary to retrieve all information about all chats to get this sort of information. This can become an issue for UI development when there are lots of users/users with a large amount of chats.

Anything else?

I have proposed an example in #338

@nenb nenb added the type: enhancement 💅 New feature or request label Mar 4, 2024
@pmeier
Copy link
Member

pmeier commented Mar 4, 2024

I don't really understand what the issue is with the current approach. GET /chats will give you all of the chats of the user.

@app.get("/chats")
async def get_chats(user: UserDependency) -> list[schemas.Chat]:
with get_session() as session:
return database.get_chats(session, user=user)

If you know the ID upfront, you can also just get the data of this particular chat with GET /chats/{id}

@app.get("/chats/{id}")
async def get_chat(user: UserDependency, id: uuid.UUID) -> schemas.Chat:
with get_session() as session:
return database.get_chat(session, user=user, id=id)

This can become an issue for UI development when there are lots of users/users with a large amount of chats.

How does the number of users play into this? Both endpoints listed above are user-specific.

@nenb
Copy link
Contributor Author

nenb commented Mar 4, 2024

I don't really understand what the issue is with the current approach. GET /chats will give you all of the chats of the user.

It gives all the messages, sources etc for all chats. The size of this object that is sent over the network grows over time and also requires a bunch of logic on the consumer side for extracting chat IDs and chat names.

If you know the ID upfront, you can also just get the data of this particular chat with GET /chats/{id}

That's the point of this PR - how can I know the ID (for a conversation that I created in a previous session)? I have to hit the /chats endpoint above.

How does the number of users play into this? Both endpoints listed above are user-specific.

I confused things here, apologies. There is the performance of the UI (see previous comments) and the performance of the API: If we have a central service hosting the API, then the API will need to load a user's entire chat history into memory just to get the names of previous chats. For a large number of users, this can create unnecessary load.

Although I now realise that #338 doesn't actually address this API issue, only the UI issue.


Ultimately, it's a QoL feature request. It's possible to get the behaviour I want with a GET to /chats, but it's not very user-friendly and has possible performance implications for anyone centrally hosting ragna.

@pmeier
Copy link
Member

pmeier commented Mar 4, 2024

Ultimately, it's a QoL feature request. It's possible to get the behaviour I want with a GET to /chats, but it's not very user-friendly and has possible performance implications for anyone centrally hosting ragna.

Could you clarify the use case you have? Why do you need only this information rather than what is returned by GET /chats?

@nenb
Copy link
Contributor Author

nenb commented Mar 4, 2024

Could you clarify the use case you have? Why do you need only this information rather than what is returned by GET /chats?

See my initial message.

This is useful functionality for interacting with specific historical chats. For example, if I would like to only show messages from a certain chat. Currently, it's necessary to retrieve all information about all chats to get this sort of information.

@pmeier
Copy link
Member

pmeier commented Mar 5, 2024

Here is how we are currently doing it in our UI:

  1. When starting the UI, we hit GET /chats, store its result, and populate the left sidebar, i.e. the chat selection

    async def refresh_data(self):
    self.chats = await self.api_wrapper.get_chats()
    @param.depends("chats", watch=True)
    def after_update_chats(self):
    self.left_sidebar.chats = self.chats
    if len(self.chats) > 0:
    chat_id_exist = (
    len([c["id"] for c in self.chats if c["id"] == self.current_chat_id])
    > 0
    )
    if self.current_chat_id is None or not chat_id_exist:
    self.current_chat_id = self.chats[0]["id"]
    for c in self.chats:
    if c["id"] == self.current_chat_id:
    self.central_view.set_current_chat(c)
    break

  2. When selecting a new chat, we pass the full chat object to the central view, which holds the chat interface

    self.left_sidebar = LeftSidebar(api_wrapper=self.api_wrapper)
    self.left_sidebar.on_click_chat = self.on_click_chat

    # Left sidebar callbacks
    def on_click_chat(self, chat):
    self.central_view.set_loading(True)
    self.current_chat_id = chat["id"]
    self.central_view.set_current_chat(chat)
    self.central_view.set_loading(False)

    def set_current_chat(self, chat):
    self.current_chat = chat

  3. In the central view we render the message objects from the chat object that we got passed

    @pn.depends("current_chat")
    def chat_interface(self):
    if self.current_chat is None:
    return
    return RagnaChatInterface(
    *[
    RagnaChatMessage(
    message["content"],
    role=message["role"],
    user=self.get_user_from_role(message["role"]),
    sources=message["sources"],
    timestamp=message["timestamp"],
    on_click_source_info_callback=self.on_click_source_info_wrapper,
    )
    for message in self.current_chat["messages"]
    ],

@nenb IIUC, here is what you want to do instead:

  1. When starting the UI, we only pull in the chat IDs and names, which is sufficient for populating the chat selection.
  2. Whenever we select a chat, e.g. on startup when at least one is available, we hit GET /chats/{id} and pass this result on to the central view.
  3. Same as above: we render the message objects from the passed chat object.

Compared to our method this has the upside that on startup we only need to load the data we actually require and thus can be quite a bit faster if there are multiple chats with a ton of messages. However, now we need to hit GET /chats/{id} when the user switches chats, introducing a slowdown at a different place. This could be counteracted by performing the GET /chats/{id} calls in the background after the initial view is rendered and cache their result. Basically building the the output of get /chats incrementally. Such a system is of course quite a bit more complex than what we have now.

Is that an accurate description of what you want to do?

@nenb
Copy link
Contributor Author

nenb commented Mar 5, 2024

Is that an accurate description of what you want to do?

More or less. There is a branch here. I'm not sure if it will be particularly helpful, as it has lots of changes, but the idea is that actions in the left sidebar drive the rest of the UI.

However, now we need to hit GET /chats/{id} when the user switches chats, introducing a slowdown at a different place.

My experience so far is that this is negligible compared to the time taken to actually rebuild and display the chat messages when switching between chats (which also affects the current version).

Here is a screenshot if what the branch currently looks like:
Screenshot 2024-03-05 at 12 50 29

@pmeier
Copy link
Member

pmeier commented Mar 6, 2024

My experience so far is that this is negligible compared to the time taken to actually rebuild and display the chat messages when switching between chats (which also affects the current version).

In that case I'm confused again. In #339 (comment) you wrote

This can become an issue for UI development when there are lots of users/users with a large amount of chats.

I assumed the "issue" you are referencing here is performance. If that is not the problem, could you elaborate what the issue actually is?

@nenb
Copy link
Contributor Author

nenb commented Mar 6, 2024

In that case I'm confused again.

GET /chats/{id} makes a request for a single chat. GET /chats makes a request for all chats. If there are a lot of chats (with a lot of bulky sources) they can be quite different.


I'm not sure if I have any more to add to this discussion (but I am still happy to open a PR if you are willing to consider the addition). I'll add an earlier message I posted here again as I think it best summarises the situation:

Ultimately, it's a QoL feature request. It's possible to get the behaviour I want with a GET to /chats, but it's not very user-friendly and has possible performance implications for anyone centrally hosting ragna.

@pmeier
Copy link
Member

pmeier commented Mar 6, 2024

GET /chats/{id} makes a request for a single chat. GET /chats makes a request for all chats. If there are a lot of chats (with a lot of bulky sources) they can be quite different.

What do you mean by "quite different"? The chat content is exactly the same.

From #339 (comment)

The size of this object that is sent over the network grows over time and also requires a bunch of logic on the consumer side for extracting chat IDs and chat names.

  1. You ruled out I/O performance as an issue
  2. What consumer logic are you talking about here? With your sample implementation in Add chat-identifiers endpoint #338, you get a JSON like [{"id": "...", "name": "..."}, ...]. With GET /chats you get [{"id": "...", "metadata": {"name": "...", ...}, ...}, ...]. You don't need any logic here. This is just plain attribute access.

Unless I'm missing something, there is no QoL improvement here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement 💅 New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants