Chainlit / chainlit

Build Conversational AI in minutes ⚡️
https://docs.chainlit.io
Apache License 2.0
7.1k stars 931 forks source link

Make full Request object available in chat #1213

Open dokterbob opened 2 months ago

dokterbob commented 2 months ago

Is your feature request related to a problem? This would address, in one go: https://github.com/Chainlit/chainlit/issues/393 https://github.com/Chainlit/chainlit/issues/144 https://github.com/Chainlit/chainlit/issues/964 https://github.com/Chainlit/chainlit/pull/1002 https://github.com/Chainlit/chainlit/issues/1103 https://github.com/Chainlit/chainlit/issues/1140

It might also work towards solving: https://github.com/Chainlit/chainlit/pull/1056

Describe the solution you'd like Accept request : [starlette.requests.Request](https://www.starlette.io/requests/) as an optional parameter to on_chat_start().

Describe alternatives you've considered See issues/PR above. The proposed solution is more generic, requires less maintenance (e.g. no specific support for various use cases required).

Open question: are there any reasons (security or otherwise) not to do this?

dokterbob commented 2 months ago

@willydouhard Would love your ideas on this one, should be really easy to implement.

Are there any security or other implications?

cta2106 commented 2 months ago

Much needed feature imo. Only thing stopping me from deploying Chainlit for a number of clients at the enterprise level.

comediansd commented 2 months ago

I need this very urgend to send user links for special training bots

nethi commented 2 months ago

@willydouhard would this PR https://github.com/Chainlit/chainlit/pull/1260 help ? For HTTP hooks, I am using ContextVar like this in server.py

# Create a context variable to store the request
request_context_var: ContextVar[Request] = ContextVar("request_context_var")

def get_current_request() -> Request:
    return request_context_var.get()

class RequestContextMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request: Request, call_next):
        # Set the request in the context variable
        request_context_var.set(request)
        # Process the request and get the response
        response = await call_next(request)
        return response

# Add request contextvar middleware to the FastAPI application
app.add_middleware(RequestContextMiddleware)
dokterbob commented 2 months ago

@willydouhard would this PR #1260 help ? For HTTP hooks, I am using ContextVar like this in server.py

# Create a context variable to store the request
request_context_var: ContextVar[Request] = ContextVar("request_context_var")

def get_current_request() -> Request:
    return request_context_var.get()

class RequestContextMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request: Request, call_next):
        # Set the request in the context variable
        request_context_var.set(request)
        # Process the request and get the response
        response = await call_next(request)
        return response

# Add request contextvar middleware to the FastAPI application
app.add_middleware(RequestContextMiddleware)

I like this approach and hadn't thought of it!

@nethi I'd love to hear your thoughts about the pros/cons of using context variables here versus adding an optional request: Request parameter to on_chat_start().

My rationale for the optional parameter (only in on_chat_start()) is that on_chat_start() actually coincides with a HTTP request (the start of the WebSocket) whereas having it available elsewhere might lead to a confusing/counter-intuitive developer experience (e.g. the browser URL might diverge from the request path). In addition, having it as a parameter in on_chat_start() might read to more readable and testable code (by being functional rather than relying on context).

Love to hear your feedback on this, it's a feature many users ask for and from where I stand either the functional approach or the context var approach are acceptable.

Curious what @willydouhard's view is on this.

bfinamore commented 2 months ago

+1 for passing the Request object in on_chat_start(). seems more flexible and explicit than the context approach for this specific goal (ie outside of the asgi server goals that #1260 addresses).

That said, having reliable path/query access is a pretty important feature that a LOT of people seem to be waiting on so would be awesome to get Request/path/query param exposure added asap one way or another

nethi commented 2 months ago

@willydouhard would this PR #1260 help ? For HTTP hooks, I am using ContextVar like this in server.py

# Create a context variable to store the request
request_context_var: ContextVar[Request] = ContextVar("request_context_var")

def get_current_request() -> Request:
    return request_context_var.get()

class RequestContextMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request: Request, call_next):
        # Set the request in the context variable
        request_context_var.set(request)
        # Process the request and get the response
        response = await call_next(request)
        return response

# Add request contextvar middleware to the FastAPI application
app.add_middleware(RequestContextMiddleware)

I like this approach and hadn't thought of it!

@nethi I'd love to hear your thoughts about the pros/cons of using context variables here versus adding an optional request: Request parameter to on_chat_start().

My rationale for the optional parameter (only in on_chat_start()) is that on_chat_start() actually coincides with a HTTP request (the start of the WebSocket) whereas having it available elsewhere might lead to a confusing/counter-intuitive developer experience (e.g. the browser URL might diverge from the request path). In addition, having it as a parameter in on_chat_start() might read to more readable and testable code (by being functional rather than relying on context).

Love to hear your feedback on this, it's a feature many users ask for and from where I stand either the functional approach or the context var approach are acceptable.

Curious what @willydouhard's view is on this.

@dokterbob For my use case (of serving multiple no-code agents from single chainlit agent), I had two different issues to deal with:

a) Get access to agent_id in Http related call backs (such as set_chat_profiles decorator) b) Get access to agent_id in websocket related call backs (such as on_chat_start decorator)

For (b), I would agree that request parameter in on_chat_start would be ideal, but it may mean contract change today.

for (a), explicit request parameter works too. But again means change to existing contract.

Btw, in my PR, ContextVar approach is used only for (a). For (b), it exposes two session variables: http_path = cl.user_session.get("http_path") http_query = cl.user_session.get("http_query")

I am not very opinionated about either of these approaches at this point as I can understand backward compatibility angle - I can rework the PR based on comments/feedback accordingly.

one question though - on_chat_start callback is from within websocket context, which is handled by socketio.ASGIApp. Will it have access to starlette.requests.Request object ?

dokterbob commented 2 months ago

Having had initial discussion with Willy, my tentative view (anticipating more reflection) is: if we ensure there's a single hook (e.g. independent of auth) where we've access to the Request of the index view as well the user session, implementers can do whatever they like in terms of copying stuff from the request to the session -- without us having to provide explicit support for it.

Alternatively, we might always make the entire (initial) request available on the session, but as the WebSocket (corresponding with on_chat_start()) is a different request from both auth and index, this might confuse developers.

Happy to hear your comments and feedback. I will probably sleep a bit on it and take good care to extensively read and (in the process) clean up the code to make sure we really do this well in the first go.