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

Serve multiple agents from a single chainlit server, access request path/query parameters #1260

Closed
wants to merge 0 commits into from

Conversation

nethi
Copy link

@nethi nethi commented Aug 25, 2024

This PR adds support to serve multiple agents from a single chainlit server. A good example is a no-code agent server that takes agent_id from URL (request path or query parameters), fetches details of the agent from another server and setups chainlit agent accordingly.

In order to do this, I needed to do the following.

  1. Ability to access agent_id from request path/query parameters
    Several chainlit hooks donot have any way to get request path or query parameter details from several of the http hooks. I have added a ContextVar setup and a utility function to access fastapi request in chainlit http hooks

With this, we can now mount chainlit on path like /agents/{agent_id} and access path in hooks

For http based cl hooks

        from chainlit.utils import get_current_http_request

        req = get_current_http_request()
        agent_id = extract_agent_id_from_path(req.url.path, prefix_to_match="/agents")

For ws based cl hooks

@cl.on_chat_start
async def on_chat_start():
        http_path = cl.user_session.get("http_path")
        http_query = cl.user_session.get("http_query")

Besides being able to serve multiple agents from single server, accessing URL path and query parameters is also a frequent ask from several chainlit users in the forums.

  1. Customize Socket IO path

while we can set http endpoint path as /agents/{agent_id}, we cannot use this approach for Socket.io as socket.io asgi server doesn't support path parameters or wild card prefixes. In the above example, socket.io path will become /agents/{agent_id}/ws/socket.io, which won't work. But socket.io server allows to add any extra path parameters such as /ws/socket.io/foobar

With this ability to customize socket.io path, it can instead be configured as, for example: /agents/ws/socket.io. With this, a custom frontend can connect to /agents/ws/socket.io/agent1. websocket hooks then can use logic like this to extract "agent1" from path parameter.

 cl.mount_chainlit(app=app, target=target, 
                                    path="/agents/{agent_id}",
                                    sio_path="/agents/ws")

@cl.on_chat_start
async def on_chat_start():
        http_path = cl.user_session.get("http_path")
        http_query = cl.user_session.get("http_query")

        agent_id = extract_agent_id_from_path(http_path, prefix_to_match="/agents/ws/socket.io")
        
        #fetch agent_id details from an external server

@nethi nethi marked this pull request as ready for review August 25, 2024 08:31
@dokterbob
Copy link
Collaborator

Thanks for the contrib @nethi!

As I see it, there's two major and desirable features in this one. But they're a big overhaul, so it's essential we think and align well on it.

In cases of larger overhauls, I usually prefer to hash out the features really well in issues, to make sure we can accept as much of your work as possible. In addition, to facilitate review, feedback and overview, I strongly prefer to have separate PR's for separate features.

In this case, your work overlaps with the following two issues;

  1. Make full Request object available in chat #1213 (as you rightly mention)
  2. Export ASGI app allowing for app.mount() instead of mount_chainlit() #1223

After a quick read through your PR and code, in light of the more general suggestions above, I'm observing that:

  1. You're choosing to privilege the path and query above other request parameters.
  2. You're adding rather than removing complexity from the mount_chainlit() logic.

1. Copying Request properties

With regards to 1, the problem is that we are opening ourselves up to adding support (increasing our API and maintenance requirements) for a literal unlimited amount of manually configured parameters mapping to properties of Request. We already have quite a few and it seems a slippery slope to keep adding more of them rather than just providing access to the full Request object as it's received either upon loading the frontend (index view) or on starting the Websocket sessions (on_chat_start()).

Unless I'm missing something (still somewhat new to the Chainlit codebase), I would need some convincing why we shouldn't actually remove/deprecate mirrored properties of Request and allow access to the entire Request object.

2. Simplifying app/mount logic

I fully feel your pain on not being able to have multiple Chainlit's mounted and it's really a feature we need. In fact, it's something I personally need for my own use cases.

But, again, there seems to be some fishy stuff going on that smells like workaround to me in the original implementation. In mount_chainlit(), we currently seem to be using environment variables to pass information from one end of the application to another. While, if I understand this well, this information is only ever passed in this direction (though it's also referenced somewhere else). Hence, we're implicitly using the environment to manage global state.

My proposal in #1223 tries to make Chainlit a normal ASGI app, which can be mounted as any ASGI app in Starlette or FastAPI and in which SocketIO is mounted on a subpath. This would get rid of silly global state (in favour of normal access to request path) and not only allow multiple agents but allow developers to truly go bananas in terms of use cases (dynamically created share links, whatever).

Again, I might be wrong on this and the reason I have not yet started work on this is that I first want to explore all implications of this, so we can arrive at a more stable API, a smaller code base and a better developer experience.

As such, seeing what you did to make this work in the current PR, I'd love to hear your opinion on my proposals. In the process, I am still convincible that my approach is not the right way to go -- but I'll need some convincing before choosing to go that way. ;)

@nethi
Copy link
Author

nethi commented Aug 29, 2024

Thanks for the contrib @nethi!

As I see it, there's two major and desirable features in this one. But they're a big overhaul, so it's essential we think and align well on it.

In cases of larger overhauls, I usually prefer to hash out the features really well in issues, to make sure we can accept as much of your work as possible. In addition, to facilitate review, feedback and overview, I strongly prefer to have separate PR's for separate features.

In this case, your work overlaps with the following two issues;

  1. Make Request object available in on_chat_start() #1213 (as you rightly mention)
  2. Export ASGI app allowing for app.mount() instead of mount_chainlit() #1223

After a quick read through your PR and code, in light of the more general suggestions above, I'm observing that:

  1. You're choosing to privilege the path and query above other request parameters.
  2. You're adding rather than removing complexity from the mount_chainlit() logic.

1. Copying Request properties

With regards to 1, the problem is that we are opening ourselves up to adding support (increasing our API and maintenance requirements) for a literal unlimited amount of manually configured parameters mapping to properties of Request. We already have quite a few and it seems a slippery slope to keep adding more of them rather than just providing access to the full Request object as it's received either upon loading the frontend (index view) or on starting the Websocket sessions (on_chat_start()).

Unless I'm missing something (still somewhat new to the Chainlit codebase), I would need some convincing why we shouldn't actually remove/deprecate mirrored properties of Request and allow access to the entire Request object.

2. Simplifying app/mount logic

I fully feel your pain on not being able to have multiple Chainlit's mounted and it's really a feature we need. In fact, it's something I personally need for my own use cases.

But, again, there seems to be some fishy stuff going on that smells like workaround to me in the original implementation. In mount_chainlit(), we currently seem to be using environment variables to pass information from one end of the application to another. While, if I understand this well, this information is only ever passed in this direction (though it's also referenced somewhere else). Hence, we're implicitly using the environment to manage global state.

My proposal in #1223 tries to make Chainlit a normal ASGI app, which can be mounted as any ASGI app in Starlette or FastAPI and in which SocketIO is mounted on a subpath. This would get rid of silly global state (in favour of normal access to request path) and not only allow multiple agents but allow developers to truly go bananas in terms of use cases (dynamically created share links, whatever).

Again, I might be wrong on this and the reason I have not yet started work on this is that I first want to explore all implications of this, so we can arrive at a more stable API, a smaller code base and a better developer experience.

As such, seeing what you did to make this work in the current PR, I'd love to hear your opinion on my proposals. In the process, I am still convincible that my approach is not the right way to go -- but I'll need some convincing before choosing to go that way. ;)

Thank you for your detailed comments/observations. I am inclined to pass Request object, but two concerns:
a) Given websocket session is handled by socketio, starlette request mayn't be readily available. Is there another way to do this ?
b) Question of API contract change - I didn't want to introduce a major contract change and followed along the approach used in the current code base.

Also, I have been using chainlit only for the last 8 weeks or so, so I mayn't fully understand some of the design choices and rationale. I can rework the PR based on consensus.

@dokterbob dokterbob added debate Under open debate. enhancement New feature or request labels Sep 3, 2024
@nethi nethi closed this Oct 31, 2024
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debate Under open debate. enhancement New feature or request size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants