Issue #241: Added progress bar updates and streaming to the summarize tool

[TikaaVo]

Refers to #241 .

This is a summary and the images, see MARKUP.md for the full report.

I set up the environment locally, then worked on the summarize tool. I made it accept nc and task_id since those are necessary to call set_progress, then I extracted max_tokens from the model config and passed it as well to use as the estimate. While max_tokens is a very conservative estimate, as most responses never hit it, meaning that most of the time a response will jump from a few percentage points to completeness or to the next split boundary, it is a safe estimate to use, although it could be altered in the future. I changed the generation to streaming, so that progress can be measured, and wrapped it in a helper function that updates the progress, then I tested it and found that the progress bar was moving successfully and text was being summarized correctly. However, this doesn't take the time required to merge splits into account, which can stall the progress bar at 100%.

The GUI seemed to have an issue, as while my code passed progress in a range of 0.0 to 100.0, those numbers show up as divided by 100 in the GUI, i.e. a progress of 25% show up as 0.25% (see screenshot).

[DaphneMuller]

Just a note for information of anyone viewing this PR: This is a PR made as part of a job application process and will be reviewed by @oleksandr-nc , according to the candidate done in a 4-hour time block.

[oleksandr-nc]

@TikaaVo, thank you for this pull request, and for the detailed writeup

A couple of questions before we go further:

1. Did you have a chance to test the other task types (e.g. free prompt, chat, headline, reformulate) with these changes? Do you think these changes could potentially introduce any regressions?
2. In _invoke_progress, the set_progress is called inside the streaming loop. Do you see any potential downside to that - and if so, how would you approach it?

[TikaaVo]

@TikaaVo, thank you for this pull request, and for the detailed writeup

A couple of questions before we go further:

1. Did you have a chance to test the other task types (e.g. free prompt, chat, headline, reformulate) with these changes? Do you think these changes could potentially introduce any regressions?

2. In `_invoke_progress`, the `set_progress` is called inside the streaming loop. Do you see any potential downside to that - and if so, how would you approach it?

@oleksandr-nc, thank you for the review and comment.

1. I didn't have a chance to change the other task types since setting up an environment took a long time (it was essentially my first time using Nextcloud or docker apps in general), but I think that since other tasks are similar, they could be applied in similar ways. If I remember correctly, all of the other tasks don't include splitting, which makes it easier and quicker to implement. However, in certain ones, such as chat and chatwithtools, generation is done differently, so the streaming would need to be adapted. Also, a tool like headline generates few tokens, so the estimate would be even more inaccurate and it may not be worth adding the streaming and progress bar overhead for a short task. invoke may also have specific error messages or error handling, and replacing it could change the error landscape.

2. While calling set_progress inside the training loop allows for detailed progress, calling it for every token adds latency and unnecessary granularity, which can slow down generation in exchange for little or no additional value. This can be improved in multiple ways, such as updating the progress bar every N tokens instead of every token, only updating it when progress crosses an integer threshold (or any other kind of threshold, depending on how many significant figures want to be kept), or calling it based on a timestamp, such as every M milliseconds. Alternatively, for lower granularity but to avoid or minimize calls inside a streaming process, the simplest solution for splits would be to use invoke and update the progress bar at the end of every split.

[oleksandr-nc]

Thanks @TikaaVo, can I ask you a bit more about both points?

Regarding question 1 - your thoughts on chat/chatwithtools needing a different streaming path, and on short tasks like headline not being worth the overhead, all make sense as future work. Let me narrow it to the diff exactly as it stands today, though: in main.py you now create the processor with task_processor_loader(nc, task["id"]), and that line runs for every task type, not just summary. If you look at how the other entries in task_processors.py are defined, what would you expect to happen when a headline or chat task gets picked up with the current code? And if you can trigger one in your environment, what actually happens?

Regarding question 2 - agreed, and your throttling options all help.
Two follow-ups: (a) what specifically makes a single set_progress call expensive - what is it doing under the hood? And (b) notice that each of your options still issues that call from the same thread that's generating tokens, so generation pauses until Nextcloud responds each time. Is there a way to structure things so that generating tokens never has to wait on hat call at all? What would that look like - and what new problems would it introduce?

[TikaaVo]

@oleksandr-nc Thanks for the engagement, I'd be happy to develop further.

1. You're absolutely correct, I had changed it for all the tasks but didn't actually have time to make all the tasks accept it, and I didn't notice it since I was focusing on making summarize work. Calling the other tasks would give an error, since their lambdas expect 0 positional inputs but 2 were given. To fix this, all the task processor lambdas would need to be altered to accept nc and task_id, which would in any case be required for their own implementation of set_progress. However, if certain tasks were to not require these or require a different structure, the code could make them accept it even if they're not used to avoid these errors, or main.py could be altered to have a check and call the task processor differently depending on the tool.

2. A single set_progress call is expensive because each call is an HTTP POST request to Nextcloud's OCS endpoint, which includes serialization, opening or using a connection, waiting for the server to respond, deserialization, and similar. Therefore, in order to structure it in a better way, you could use a separate thread (or generally a fire and forget type approach) in order to allow the generation to keep running. However, if one thread finished before a previous one, a user would see the progress bar regressing. Furthermore, creating a new thread for every token would be infeasible at best. Both of these can be addressed by adding a background worker queue or similar, but this adds complexity and introduces the risk that if one request fails, all the others are stuck in the queue, so the progress bar could stop without a clear error message. So the best approach would be to combine the previously mentioned throttling with a separate thread system controlled by some kind of process manager.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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.)