feat: enter user location before chat #106
Conversation
|
Here's one optional part of the tool API that I found that might be useful to set to |
OK that's exactly what I was looking for, thank you! |
|
Lol, nvm, requiring tool calls isn't supported with o3. |
|
Pardon to be a backseat driver here, but my 2 cents is that we should just use the IP address of the user to categorize it into "Portland", "Eugene", "Other Oregon", "Non-oregon" as part of the system prompt. I think the less clicks the better but I'm also just guessing what people want. |
I think this intent is good but I'm not confident that we can assume that users will be on their home Internet where their IP address geo locates their home. They could be on a mobile phone or a public terminal (e.g. library) or being helped by a 3rd-party (e.g. clinic). VPNs also confuse geo location. Geo locating the IP could be used as a first guess but would need to be confirmed and be able to be overridden through some interaction. |
If they're using it from any location other than their home we can't assume their IP represents their home city, so this seemed like the best alternative. It's also possible they're asking a question about a city they want to move to but don't currently live in. |
|
@leekahung made changes from your review. |
| "filters": { | ||
| "type": "or", | ||
| "filters": [ | ||
| { | ||
| "type": "and", | ||
| "filters": [ | ||
| { | ||
| "type": "eq", | ||
| "key": "city", | ||
| "value": current_session["city"], | ||
| }, | ||
| { | ||
| "type": "eq", | ||
| "key": "state", | ||
| "value": current_session["state"], | ||
| }, | ||
| ], | ||
| } | ||
| if current_session["city"] != "null" | ||
| else None, | ||
| { | ||
| "type": "and", | ||
| "filters": [ | ||
| { | ||
| "type": "eq", | ||
| "key": "city", | ||
| "value": "null", | ||
| }, | ||
| { | ||
| "type": "eq", | ||
| "key": "state", | ||
| "value": current_session["state"], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Doesn't this simplify to this? If current_session["state"] == "null", then the first filter (city) will be assigned "null". I think this avoids ORing filter[None, nested_filter].
| "filters": { | |
| "type": "or", | |
| "filters": [ | |
| { | |
| "type": "and", | |
| "filters": [ | |
| { | |
| "type": "eq", | |
| "key": "city", | |
| "value": current_session["city"], | |
| }, | |
| { | |
| "type": "eq", | |
| "key": "state", | |
| "value": current_session["state"], | |
| }, | |
| ], | |
| } | |
| if current_session["city"] != "null" | |
| else None, | |
| { | |
| "type": "and", | |
| "filters": [ | |
| { | |
| "type": "eq", | |
| "key": "city", | |
| "value": "null", | |
| }, | |
| { | |
| "type": "eq", | |
| "key": "state", | |
| "value": current_session["state"], | |
| }, | |
| ], | |
| }, | |
| ], | |
| }, | |
| "filters": { | |
| "type": "and", | |
| "filters": [ | |
| { | |
| "type": "eq", | |
| "key": "city", | |
| "value": current_session["city"], | |
| }, | |
| { | |
| "type": "eq", | |
| "key": "state", | |
| "value": current_session["state"], | |
| }, | |
| ], | |
| }, |
There was a problem hiding this comment.
No, it doesn't, because if the user is in Portland, we want to match documents that match both Portland AND Oregon and null and Oregon (Oregon Laws).
There was a problem hiding this comment.
Okay, then it might be worth a lengthier comment above and/or pulling out the filtering values/logic into separate named helper functions.
backend/tenantfirstaid/session.py
Outdated
| return json.loads(self.db_con.get(session_id) or "[]") | ||
| print("SESSION_ID", session_id) | ||
| return json.loads( | ||
| self.db_con.get(session_id) or '{"city": "", "state": "", "messages": []}' |
There was a problem hiding this comment.
Would it flag bugs earlier if this returned None or raised an error if the session_id is not found in the valkey DB? With None, static type-checking could flag when this branch is possible (so there is handling code). With raising an error, tests would flag this branch, requiring code coverage to see when handling code is missing)
There was a problem hiding this comment.
Possibly, but for right now the easiest thing to do was just to init a new session, since the chat history itself isn't terribly important. Error handling for session retrieval should probably be raised in a different PR, since this one just changes the structure of the session data.
backend/tenantfirstaid/app.py
Outdated
| session_id = session.get("session_id") | ||
| if not session_id: | ||
| return jsonify([]), 200 | ||
| saved_session = tenant_session.get(session_id) | ||
| if not saved_session: | ||
| return jsonify({"error": "No session found"}), 404 | ||
| print(saved_session) |
There was a problem hiding this comment.
feels like this logic should be in session.py
| app.add_url_rule( | ||
| "/api/init", | ||
| view_func=InitSessionView.as_view("init", tenant_session), | ||
| methods=["POST"], | ||
| ) |
There was a problem hiding this comment.
It's not clear what the life-cycle of a Flask session and a TenantSession is. But maybe I'm the only one confused about this.
There was a problem hiding this comment.
Agreed, it's a little confusing. Flask session would be the flask-supplied session ID and cookie (used to generate an ID), TenantSession is the user's info and chat history stored in Valkey.
I plan to do some refactoring of
chat.pyto extract core functionality to a separate module, as well as add some tests for this later this week.create_vector_store.pyto delete all files in store if it exists, and upload files from new directory structure with city and state metadata