Add support for SSE (WagtailVectorIndexSSEConsumer) and async querying #28
Conversation
Morsey187
changed the title
| # Finish the response | ||
| await self.send_body(b"") | ||
|
|
||
| async def process_prompt(self, query, vector_index): |
There was a problem hiding this comment.
we've been type hinting from the onset in this package. Any chance you can continue the tradition consistently in your changes?
There was a problem hiding this comment.
I've had to update this to any, although I know it should be a VectorIndex type there are some import errors raise AppRegistryNotReady("Apps aren't loaded yet.") with using it. Theres also quite a few existing type errors which open another PR to resolve some of them.
| Note: | ||
| This consumer expects the following query parameters in the URL: | ||
| - 'query': The search query. | ||
| - 'page_type': The type of Wagtail page to search. |
There was a problem hiding this comment.
Suggestion: Vector indexes may not to be related to Wagtail pages - they could be Documents, other Django models, or some completely independent data source. If we can take the index name here and look it up in the registry, that would enable this endpoint to work across all defined indexes.
There was a problem hiding this comment.
Had a look into this, however, could do with some pointers as I can't figure out why self.object_type.bulk_from_documents in aquery errors.
Morsey187
force-pushed
the
feature/event-stream-http-consumer
branch
from
f1fa5e8 to
6b370e9
Compare
| @@ -21,6 +27,25 @@ class QueryResponse(Generic[VectorIndexableType]): | |||
| sources: Iterable[VectorIndexableType] | |||
|
|
|||
|
|
|||
| @dataclass | |||
| class AsyncQueryResponse(Generic[VectorIndexableType]): | |||
There was a problem hiding this comment.
Note: I have to return the whole response object to access it's iterator (response chunks). I’d prefer to avoid this and instead update QueryResponse to return the whole response instead of a string too.
Theres also the case that users might want to access the response object as a whole and for other attributes and methods like json().
If so maybe we can rename response as "llm_response"?
| """ | ||
| Async version of the query method. | ||
| """ | ||
| if not self.chat_backend.can_stream(): |
There was a problem hiding this comment.
Note: I've added this here as I think there will likely be an issue with developers expecting streaming support when using async, especially since we're abstracting from the LLM package, so theres no obvious way for them to know without tracing errors back.
I don't think this is the correct solution if it is a problem, but have added it here to highlight the potential issue and get some thoughts?
| ] | ||
| ``` | ||
|
|
||
| Next, you will need to define a new consumer inheriting from `WagtailVectorIndexSSEConsumer`, and assign a Wagtail page model for the vector index you'd like to use. |
There was a problem hiding this comment.
Issue: I believe this is no longer true now we use a single consumer across all indexes?
| form = WagtailVectorIndexQueryParamsForm(query_dict) | ||
| if not form.is_valid(): | ||
| # Ignore "TRY301 Abstract `raise` to an inner function" | ||
| # So we can insure the event-stream is closed and no other code is executed |
There was a problem hiding this comment.
nitpick (non-blocking): insure -> ensure
Changes
WagtailVectorIndexSSEConsumera AsyncHttpConsumer which supports persistent HTTP connections using the event-stream format, where query responses can be streamed (resulting in that typewriter effect).Testing
Requires following instructions in the added documentation including setting up an ASGI server.