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

Add child class for single callable #46

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/tqdm_publisher/_progress_subscriber.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
from ._publisher import TQDMPublisher


class TQDMProgressSubscriber(TQDMPublisher):
def __init__(self, iterable, *, announcer: "?", request_id: str, message: dict, **tqdm_kwargs):
super().__init__(iterable, **tqdm_kwargs)

def on_progress_update(format_dict) -> None:
"""
Describe what this announcer is all about...
"""
announcer.announce(dict(request_id=request_id, **message))

self.subscribe(callback=on_progress_update)
Comment on lines +4 to +14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I meant to splinter this to a different PR/branch, my bad

What would you think about this structure?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main question is, what is the announcer class (is it something from Flask?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the announcer is from the GUIDE server—so that has to be used within a user-provided callable to the TQDMProgressSubscriber.

Unless you want to bring assumptions about the server-side event system implemented in the GUIDE (in contrast to the WebSockets used in our demos) to tqdm_publisher.

  1. MessageAnnouncer class declaration: https://github.com/NeurodataWithoutBorders/nwb-guide/blob/main/pyflask/sse.py
  2. Flask SSE handler: https://github.com/NeurodataWithoutBorders/nwb-guide/blob/4ab4e1f682d9bd9e1b4b3d79f9c0e2e593bddea9/pyflask/manageNeuroconv/manage_neuroconv.py#L688

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fair amount of that listener logic seems quite general, so could very well be included along with this child class

What is the specific point at which it interacts with the GUIDE client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the two locations defined above. The SSE handler waits for new messages to be generated by calling MessageAnnouncer.announce in other endpoints.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response in NeurodataWithoutBorders/nwb-guide#695 and #49.

These introduce a tqdm_publisher global import, though, since the listen_to_neuroconv_events endpoint needs to have a reference to a single MessageAnnouncer instance used across progress updates when the user requests that they'd like to receive updates from that endpoint.

If we can't tolerate that, then this line of inquiry is infeasible since whatever queue we use has to be available in several endpoints and the simplest way to use this queue (i.e. with the least assumptions, most flexibility) would be in a callback as the initial commit for this PR was already doing.

7 changes: 0 additions & 7 deletions src/tqdm_publisher/_subscriber.py

This file was deleted.

Loading