Conversation
|
Can one of the admins verify this patch? |
hospexplorer/ask/llm_connector.py
Outdated
| settings.LLM_HOST + settings.LLM_QUERY_ENDPOINT, | ||
| json=payload, | ||
| headers=headers, | ||
| timeout=60 |
There was a problem hiding this comment.
the timeout needs to be configurable (60 seconds might not be enough).
hospexplorer/ask/views.py
Outdated
| return JsonResponse({"error": f"Failed to connect to server: {e}"}, status=500) No newline at end of file | ||
| return render(request, "_response.html", {"message": content}) | ||
| except Exception: | ||
| return render(request, "_response.html", {"message": "Something went wrong. Please try again."}) No newline at end of file |
There was a problem hiding this comment.
I don't think this is right, is it? Doesn't the api call from the frontend to the backend still wait for the server to respond? And if that takes too long, then the webserver might still time out. The frontend needs to poll the backend to see if the LLM has responded yet and if not then keep polling.
|
Resolve conflicts please |
|
Resolve conflicts please |
jdamerow
left a comment
jdamerow
left a comment
There was a problem hiding this comment.
We just talked about this. Either Thread or asyncio should be enough.
|
Thread vs asyncio: https://www.geeksforgeeks.org/python/asyncio-vs-threading-in-python/ |
|
Resolve conflicts please |
hospexplorer/ask/views.py
Outdated
| query_text = request.GET.get("query", "") | ||
| record = QARecord.objects.create( | ||
| question_text=query_text, | ||
| @require_GET |
There was a problem hiding this comment.
I think we want this to be a POST request. This seems to be the typically way to do it and is required should we send lots of data.
hospexplorer/ask/views.py
Outdated
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _run_llm_task(task_id): |
There was a problem hiding this comment.
let's not put this in views.py. I think this deserves it's own file where we keep functions that the all the stuff around making a query.
jdamerow
left a comment
jdamerow
left a comment
There was a problem hiding this comment.
same issue as with the other prs. migrations need to be added as new migrations, not changing the initial one.
|
I matched the migrations for this PR, HOP-17 and HOP-18, all three files have identical migrations from develop (001 and 002), when either branch will be merged to develop it will cause conflict with migrations as django detects two 0003 migration file. It will be a simple fix in each conflict |
|
@Girik1105 I believe you will have to run the merge migrations command and then commit the resulting migration file once this code has been merged into develop. |
|
Make it so, Jenkins. |
3 participants
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]into[x]).Please provide a brief description of your ticket
chat request from frontend needs to happen asynchronously
Description
otherwise the backend times out. so maybe some “thinking” feedback in the frontend while the request is made and being polled.
HOP-11
Are there any other pull requests that this one depends on?
Anything else the reviewer needs to know?
... describe here ...