Feature: Store chat history in Cosmos DB #2063
Conversation
|
Woot! I know this is a very popular request, thanks for the PR, I'll review it this week. In the meantime, if anyone subscribed tries it out as well, please report on the PR with your feedback as well. I'll also ask Cosmos DB team to take a look. |
app/backend/app.py
Outdated
| @@ -397,6 +407,129 @@ async def list_uploaded(auth_claims: dict[str, Any]): | |||
| return jsonify(files), 200 | |||
|
|
|||
|
|
|||
| @bp.post("/chat_history") | |||
There was a problem hiding this comment.
Do you think the chat history endpoints could be in a different file/blueprint? That would make it to bring in additional providers (I've been asked about Azure SQL, for example) without bloating app.py.
I know we only have a single file/blueprint for routes now though, so it's possible that I haven't organized it in such a way that it's feasible.
There was a problem hiding this comment.
Certainly, app.py is getting too large, so I think it's good to separate the blueprint. I'll try to restructure it to separate it.
There was a problem hiding this comment.
Configure separate blueprint in 44bfc40
app/frontend/src/api/models.ts
Outdated
| id: string; | ||
| entra_id: string; | ||
| title?: string; | ||
| _ts: number; |
There was a problem hiding this comment.
What/why is _ts? It's a funny variable name, so it could benefit from either an inline comment or a rename.
There was a problem hiding this comment.
_ts is a timestamp record that CosmosDB assigns by default.
We can define and assign a timestamp record defined in our application, but which is better?
https://learn.microsoft.com/en-us/rest/api/cosmos-db/documents
There was a problem hiding this comment.
Hm, maybe we should change it in the server-side when we pass back the results? I'm imagining that if we had an Azure SQL history provider, then we wouldn't want to use _ts, we'd want them both to pass back timestamp. And ideally use the same TypeScript models. Does that make sense?
There was a problem hiding this comment.
Updated in 11c4d9f
app/frontend/src/api/models.ts
Outdated
| export type HistoryListApiResponse = { | ||
| items: { | ||
| id: string; | ||
| entra_id: string; |
There was a problem hiding this comment.
This is oid, right? Maybe entra_oid would be slightly clearer (versus an Entra group id)
There was a problem hiding this comment.
This is a typo. I will fix it.
Use the timestamp property instead of _ts
Check Broken URLsWe have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue. Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.
|
|
I'm reviewing now, have made a few cosmetic changes and added a test. I'll add a few more tests tomorrow and test my deploy. Have also asked a Cosmos DB colleague to review. |
Check Broken URLsWe have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue. Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.
|
Check Broken URLsWe have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue. Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.
|
|
Update: I found an issue with the authentication decorator being used for the /items/id get and delete routes and have fixed it. It's working well for me locally now. |
|
|
||
| return jsonify({"items": items, "continuation_token": continuation_token}), 200 | ||
|
|
||
| except Exception as error: |
There was a problem hiding this comment.
Consider using more specific cosmos exceptions, e.g. https://learn.microsoft.com/en-us/python/api/azure-cosmos/azure.cosmos.exceptions.cosmoshttpresponseerror?view=azure-python.
There was a problem hiding this comment.
Nice suggestion! I've looked into it and I think that we do want the general Exception catching here, to make sure that we always pass JSON down to the frontend if the server errors, so the user sees that there's been an error. But we might add special handling for the CosmosDB exceptions in future if it makes our logs easier to grok.
|
|
||
| # Get the first page, and the continuation token | ||
| try: | ||
| page = await pager.__anext__() |
There was a problem hiding this comment.
Consider iterating over the by_page result directly and avoiding explicit calls to await pager.anext(), i.e. process each page as soon as it’s available without awaiting when there are no more pages.
There was a problem hiding this comment.
I think you mean to just do:
async for page in pager:
async for item in page:
As we use similar code for other Azure Python SDKs elsewhere in this repo. That wouldn't give us the continuation token, right, as that would exhaust all the pages?
There was a problem hiding this comment.
Yes, but you should still be able to get the continuation token. Something like (untested):
items = []
async for page in container.query_items(
query="SELECT c.id, c.entra_oid, c.title, c.timestamp FROM c WHERE c.entra_oid = @entra_oid ORDER BY c.timestamp DESC",
parameters=[{"name": "@entra_oid", "value": entra_oid}],
max_item_count=count,
).by_page(continuation_token):
async for item in page:
items.append(
{
"id": item.get("id"),
"entra_oid": item.get("entra_oid"),
"title": item.get("title", "untitled"),
"timestamp": item.get("timestamp"),
}
)
# Update continuation token after processing the page
continuation_token = page.continuation_token if hasattr(page, "continuation_token") else None
# Break if no continuation token (i.e., last page)
if not continuation_token:
breakJust a suggestion. I think its fine as it is :-)
| if not current_app.config[CONFIG_CHAT_HISTORY_COSMOS_ENABLED]: | ||
| return jsonify({"error": "Chat history not enabled"}), 405 | ||
|
|
||
| container: ContainerProxy = current_app.config[CONFIG_COSMOS_HISTORY_CONTAINER] |
There was a problem hiding this comment.
This is called in every function and I assume it never changes. Could this be cached or defined globally?
There was a problem hiding this comment.
current_app.config is basically our global dict, it's how we access objects that were setup at the beginning of the app start. I don't think there's a performance hit, since it should be O(1) retrieval.
| {"id": id, "entra_oid": entra_oid, "title": title, "answers": answers, "timestamp": timestamp} | ||
| ) | ||
|
|
||
| return jsonify({}), 201 |
There was a problem hiding this comment.
wonder whether the status code here should be based on the response from container.upsert_item - if the item already already exists, then it will be 200 instead of 201
There was a problem hiding this comment.
That's a nice idea! I can't figure out from the SDK return types what in the response would indicate that however, as all the function signatures just say that they return a dict: https://learn.microsoft.com/en-us/python/api/azure-cosmos/azure.cosmos.container.containerproxy?view=azure-python#azure-cosmos-container-containerproxy-upsert-item
And this example doesn't do anything with the dict: https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/cosmos/azure-cosmos/samples/document_management.py#L149-L156
So is there some listing of what key in the dict would indicate it already existing?
There was a problem hiding this comment.
I've taken a look at the dicts for a first-time ID versus a second-time ID, and I can't find any key that'd indicate whether the item already existed:
https://www.diffchecker.com/Fs0ECaUa/
So I don't think we should change the status code currently, as I'm not sure what condition to use, but happy to change in the future if you have suggestions.
Check Broken URLsWe have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue. Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.
|
Check Broken URLsWe have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue. Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.
|
|
@fujita-h I added pytest for 97% coverage of CosmosDB. While adding the tests, I made a few changes:
|
Check Broken URLsWe have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue. Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.
|
|
@pamelafox Thank you for adding the test, I have read and understood your corrections. |
There was a problem hiding this comment.
I am approving this PR, as I've both tested manually and with pytest unit tests, and the CosmosDB team has also reviewed it, with no blocking issues. I'll check if @mattgotteiner wants to take a look before we merge.
Thanks @fujita-h for the great work!
Check Broken URLsWe have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue. Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.
|
|
I looked at the Bicep validation error but I think that is an unrelated issue, likely due to a newer version of Bicep that's stricter. |
Purpose
This PR extends the chat history feature added in #1988 to store chat history server-side (Cosmos DB) when user authentication is enabled.
Does this introduce a breaking change?
When developers merge from main and run the server, azd up, or azd deploy, will this produce an error?
If you're not sure, try it out on an old environment.
Does this require changes to learn.microsoft.com docs?
This repository is referenced by this tutorial
which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial,
check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.
Type of change
Code quality checklist
See CONTRIBUTING.md for more details.
python -m pytest).python -m pytest --covto verify 100% coverage of added linespython -m mypyto check for type errorsruffandblackmanually on my code.