-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add JsonStorage and ability to chat with References #127
feat: add JsonStorage and ability to chat with References #127
Conversation
I have a preference for small PRs and it feel like this one is already getting pretty large. In order to move this PR forward, I've created three follow-up issues for myself.
I think there's an open question on how and whether we want to handle chat interactions that are not questions, but IMO that might be something that is informed after spending some time with this implementation. |
def get_top_n(self, query: str, limit: int = 5) -> list: | ||
""" | ||
Rank documents based on input text | ||
:param input_text: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the actual param name is query, not input_text
with open(self.filepath, 'r') as f: | ||
data = json.load(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a try except here, in case the filepath or the file content is incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good. My only comment is for the chat return type being an array instead of an object that contains an array.
@@ -13,9 +14,15 @@ | |||
"items": { | |||
"$ref": "#/definitions/RewriteChoice" | |||
} | |||
}, | |||
"chat": { | |||
"type": "array", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we return an object in the root instead of array. That would be better for evolvability.
fixes #102 and fixes #82
Note that you will need to upload some PDFs and rerun PDF ingestion first. That will create a
references.json
which is needed for chat interactions.There is more backend work to do related to chat, but I've captured that in other issues (see comment below). I didn't want this PR to continue to increase in scope and also think it'd be good to have this piece in so that we can begin to demo it.
chat.py
ranker.py
storage.py