-
Notifications
You must be signed in to change notification settings - Fork 11
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 input chunking to summarize task #157
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.
👍
The admin setting input could be moved in the "Text generation" section. Wdyt?
Maybe the chunk size could also be considered as a usage limit, but since we only use it on the summarize task, I think it would make sense to move it there too. |
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.
The code looks really good. The algorithm deviates a little from what llm2 does, though: In llm2 the summaries of the chunks are concatenated and fed through the same algorithm again (ie. chunked and summarized) until there is only one summary left.
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.
Oops, clicked the wrong button.
Now that you mention it, I do see the similar loop in llm2 now, fixed. |
I'm about to change the algorithm in llm2 slightly, so we should change it here as well a last time: When the input is shorter than the chunk size it should still go through the llm summarizer once. |
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.
👍 A few adjustments and good to go!
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
d9f8184
to
344c92c
Compare
Looks good and works in my tests. It seems we lost multilingual support though. When summarizing a German text that is longer than the chunk size the resulting summary was in english in my test. |
In my testing, summarizing English text produced output in either Spanish or Italian, hence the change in prompt. If you want me to revert that change for now, we can do that. |
Oh, that's not intended of course :/ Ideally we should find a prompt that works for all languages to produce the summary in the same language as the input. |
344c92c
to
2a45a02
Compare
Changed the prompt again after some basic prompt engineering and experimenting. Hopefully, this will produce more reliable results. |
31940fb
to
79189c6
Compare
Thanks for these adjustments, works well for me now! |
cc30634
to
fffe764
Compare
Signed-off-by: Edward Ly <[email protected]>
…ings service Signed-off-by: Edward Ly <[email protected]>
…nguage boundaries Signed-off-by: Edward Ly <[email protected]>
…PI, update prompts Addresses an issue where the input and output text are not always in the same language Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
fffe764
to
5c78917
Compare
Rebased and adjusted after #167. |
Woop woop 🎉 |
Closes #150. Lightly tested with gpt-4o, gpt-4o-mini, and gpt-3.5-turbo.