-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 multi-document answers #63
Conversation
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.
Great PR! I left some comments
Co-authored-by: Candido Sales Gomes <candidosg@gmail.com>
Co-authored-by: Candido Sales Gomes <candidosg@gmail.com>
Co-authored-by: Candido Sales Gomes <candidosg@gmail.com>
Co-authored-by: Candido Sales Gomes <candidosg@gmail.com>
Hey, thanks for taking your time and the one fix. I will remove the type check for list, since it's not required anymore. I'm not sure about the typing you add. I'm a big fan, I like strong typing a lot, but it just feels weird to have typing for one function and no where else in the code. There should be a full refactor that adds typing. What do you think? |
@candidosales I get a linting error for the typing (I'm using Python 3.8.10). And it's also throwing an error when I run it.
I did some research. The problem is that we require python >=3.8. According to my linter and my research, your suggestion is only for Python >=3.9, so it's not fully compatible with Python 3.8 anymore. I suggest we bump the required version to >=3.9. Then we can use the current (simple) typing, that you suggested. But that's something that should not be decided in this PR. I will remove the typing changes. @taranjeet please note.
|
@cachho I'm using Python What do you think? |
I agree, once again the only problem with it in this PR is that we lose compatibility with 3.8. We need to decide if that's worth it, and that's out of scope for this PR.
I agree, but I would talk to @taranjeet on discord first, before you go all in and then we decide to keep 3.8 compatibility. That's the version I use btw, and I don't think I'm the only one. |
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.
I left a comment
resolved the merge conflicts. |
adjusted to new config |
example: import os
from embedchain import App
from embedchain.config import AddConfig, QueryConfig
naval_chat_bot = App()
add_config = AddConfig() # Currently no options
naval_chat_bot.add("youtube_video", "https://www.youtube.com/watch?v=3qHkcs3kG44", add_config)
naval_chat_bot.add("pdf_file", "https://navalmanack.s3.amazonaws.com/Eric-Jorgenson_The-Almanack-of-Naval-Ravikant_Final.pdf", add_config)
naval_chat_bot.add("web_page", "https://nav.al/feedback", add_config)
naval_chat_bot.add("web_page", "https://nav.al/agi", add_config)
naval_chat_bot.add_local("qna_pair", ("Who is Naval Ravikant?", "Naval Ravikant is an Indian-American entrepreneur and investor."), add_config)
query_config = QueryConfig(number_documents=1)
print(naval_chat_bot.dry_run("What unique capacity does Naval argue humans possess when it comes to understanding explanations or concepts?", query_config))
query_config = QueryConfig(number_documents=5)
print(naval_chat_bot.dry_run("What unique capacity does Naval argue humans possess when it comes to understanding explanations or concepts?", query_config)) returns
versus
|
@taranjeet changed readme text to |
du to the custom prompt, the idea of changing the prompt based on plural and singular numbers was ditched. I think that's okay. |
Adds number of documents as an optional argument. This is a very important feature for databases, that embed highly fractured, (short) documents (like my QnA).
The number 1 is used as a default so it's fully backwards compatible.
For this to work, context is now a list instead of a string.
Open questions:
|
the right delimiter?number_documents
tonumber_chunks
. This is just a naming/correctness issue, because we can't change anything about it anyways.Followup