Skip to content
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

Inspect progress bars with a global handler #695

Merged
merged 15 commits into from
May 30, 2024

Conversation

garrettmflynn
Copy link
Member

This PR implements what was discussed in catalystneuro/tqdm_publisher#46 where the MessageAnnouncer class used by the GUIDE is superceded by a queue manager provided by tqdm_publisher.

This update allows utilities that ingest the handler (e.g. NWB Inspector, DANDI, etc.) to simple expect a dictionary of additional_metadata to pass alongside the update provided by TQDMProgressSubscriber.

Uses catalystneuro/tqdm_publisher#49

@garrettmflynn garrettmflynn self-assigned this Mar 20, 2024
@garrettmflynn garrettmflynn changed the base branch from main to inspect-progress-bars March 20, 2024 18:35
@CodyCBakerPhD
Copy link
Collaborator

In general looks much simpler for the GUIDE side, and I like the abstraction

Some comments on the other repo about final refinement

@CodyCBakerPhD CodyCBakerPhD marked this pull request as draft March 22, 2024 22:10
Base automatically changed from inspect-progress-bars to main May 14, 2024 18:15
@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn This has been replaced by another, right?

@garrettmflynn
Copy link
Member Author

This feature is implemented without this PR. Though this particular addition has not been replaced.

We are currently using a GUIDE-internal MessageAnnouncer class, whereas this PR uses the TQDMProgressHandler class from tqdm_publisher to ensure that messages are handled serially.

The intention of this PR was to move more responsibilities to tqdm_publisher, though I'm not personally invested in which solution we decide to use.

@CodyCBakerPhD
Copy link
Collaborator

This appears to break the parallel bars; I'm unsure if it's parallelizing without informing, but I no longer see the sub-progress bars completing per file over a directory

@garrettmflynn
Copy link
Member Author

Good catch, thank you. There was still a leftover announcer declaration in the NeuroConv namespace.

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review May 30, 2024 17:17
@CodyCBakerPhD CodyCBakerPhD enabled auto-merge May 30, 2024 17:17
@CodyCBakerPhD CodyCBakerPhD merged commit 2c15f7c into main May 30, 2024
22 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the inspect-progress-bars-handler branch May 30, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants