Skip to content

Smoother progress reporting#3116

Closed
dzenanz wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
dzenanz:betterProgress
Closed

Smoother progress reporting#3116
dzenanz wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
dzenanz:betterProgress

Conversation

@dzenanz
Copy link
Copy Markdown
Member

@dzenanz dzenanz commented Jan 17, 2022

This is a follow-up to #3014 and #3052.

@hjmjohnson
Copy link
Copy Markdown
Member

@dzenanz Can this be completed or closed?

This enables progress updates from all threads
in progress accumulator and progress transformer.
@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Oct 8, 2025

Force-push is a straight rebase. I needed to resolve some conflicts. I might have some time to continue this PR, this week or the next.

@blowekamp
Copy link
Copy Markdown
Member

I do have many concerns for the implication of this change, and using by default with these methods. These include increasing the number of event when the call backs may be expensive, to a change in the threading behavior of the events. For example with this change multiple progress events can be occurring at the same time! And this could pause the progress of the algorithm.

A more robust approach would be using an event based messaging queue. Where any thread adds an event to the queue, and the main/calling thread's (only?) job it is to invoke these event call backs. Options can be added to consolidate multiple progress or iteration events or similar event processing. This approach continues to only have event call backs in the main thread, and only one thread event call-back happening at once, and does not pause the algorithm progress.

Perhaps further thought and discussion should take place before more work is done.

@dzenanz
Copy link
Copy Markdown
Member Author

dzenanz commented Oct 8, 2025

Good thoughts. As there is not a lot of code here, abandoning it is not a big loss. I will close this PR.

The queue that is processed by the invoking thread sounds like the right solution. Could you turn it into an issue, so we don't forget about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants