7 detail backend for qa on html #29
Conversation
|
I totally messed up here, since I didn't recognize, that the url upload works, but the file upload not anymore. Currently working on a fix, but until now wasn't able to resolve the 422 Unprocessible Entity. I Found out that it is not possible to combine Uploadfile and string url in one Basemodel and also as separate route function parameters it does not work, yet ... working on this ... |
|
OK, fixed it, ready to merge. |
|
@Zaubeerer I am ready to merge from my perspective and waiting for your review/ approve. |
|
Awesome that you found the bug and fixed it :) Do we already have a follow-up issue to create tests? Whenever you find such a bug, it is important to create a test that would have caught the bug. |
backend/.pre-commit-config.yaml
Outdated
| @@ -0,0 +1,17 @@ | |||
| repos: | |||
There was a problem hiding this comment.
this file should be deleted (pre commit config needs to be at root of project directory)
There was a problem hiding this comment.
yes, I don't know how it went to the backend directory but I put it to root now(again).
There was a problem hiding this comment.
Seems like you did not push the changes yet?
There was a problem hiding this comment.
Now I pushed everything ...
backend/fastapi_app.py
Outdated
| except Exception as e: | ||
| return TextSummaryModel( | ||
| file_name=file.filename, | ||
| file_name="", | ||
| text_category="", | ||
| summary=f"There was an error on uploading the file: {e.args}", |
There was a problem hiding this comment.
Do you really want the error to pass semi-silently?
This may introduce some errors downstream, unless you do something with the exceptions that are logged via the return message 🤔
There was a problem hiding this comment.
See comment below, I detailed the Exception handling and removed the silencing of Exceptions in general.
backend/fastapi_app.py
Outdated
| except Exception as e: | ||
| return TextResponseModel(message=f"Error: {e.args}") |
There was a problem hiding this comment.
This is not good practice and will likely be complained about by code quality checks.
Are there some specific exceptions that you want to accept?
There was a problem hiding this comment.
I removed the whole try - except handling here, since it is not really needed ...
|
Let me know, when the conflicts are resolved :) |
| except HTTPException as e: | ||
| message = (f"There was an error on uploading your text/ url: {e.args}",) | ||
| except MissingSchema as e: | ||
| message = f"There was a problem with the provided url: {e.args}" | ||
| except OSError as e: | ||
| message = f"""There was an unexpected OSError on saving the file: | ||
| {e.args}, please ask the admin for write permissions | ||
| """ | ||
| return TextSummaryModel( | ||
| file_name=file_name, | ||
| text_category=document.text_category, | ||
| summary=document.text_summary, | ||
| text_category=text_category, | ||
| summary=message, | ||
| used_tokens=token_counter.total_llm_token_count, | ||
| ) |
There was a problem hiding this comment.
@Zaubeerer : I detailed the Exception block and removed the silencing of Exceptions in general. Nevertheless, the error message was and is printed in the chat on the frontend, so it never really went through silently.
There was a problem hiding this comment.
@Zaubeerer : The conflicts are resolved and I implemented your suggested adjustments. Thank you :) !
introduced new class AIHtmlDocument which inherits from AITextDocument, while the _load_document method is overwritten to load a html file via url. On the backend the upload route is extended to receive an Uploadfile or a str (url).
The test implementation is postponed to a new issue #28
from my perspective ready to merge