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

feat: add use_table_listener hook #115

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

jnumainville
Copy link
Collaborator

Fixes #96

Adds a hook that uses a table listener
Here's an example that will monitor added or removed rows in the most recent update, depending on if the appropriate checkboxes are checked

import deephaven.ui as ui
from deephaven.table import Table
from deephaven import time_table, empty_table, merge
from deephaven.table_listener import listen
from deephaven import pandas as dhpd
import pandas as pd

def to_table(update):
    return dhpd.to_table(pd.DataFrame.from_dict(update)) 

def add_as_op(ls, t, op):
    t = t.update(f"type=`{op}`")
    ls.append(t)

@ui.component
def monitor_changed_data(source: Table):

    changed, set_changed = ui.use_state(empty_table(0))

    show_added, set_show_added = ui.use_state(True)
    show_removed, set_show_removed = ui.use_state(True)

    def listener(update, is_replay):

        to_merge = []

        if (added_dict := update.added()) and show_added:
            added = to_table(added_dict)
            add_as_op(to_merge, added, "added")

        if (removed_dict := update.removed()) and show_removed:
            removed = to_table(removed_dict)
            add_as_op(to_merge, removed, "removed")

        if to_merge:
            set_changed(merge(to_merge))
        else:
            set_changed(empty_table(0))

    ui.use_table_listener(source, listener)

    added_check = ui.checkbox("Show Added", isSelected=show_added, on_change=set_show_added)

    removed_check = ui.checkbox("Show Removed", isSelected=show_removed, on_change=set_show_removed)

    return [added_check, removed_check, changed]

t = time_table("PT1S").update(formulas=["X=i"]).tail(5)

monitor = monitor_changed_data(t)

@jnumainville jnumainville self-assigned this Nov 13, 2023
@jnumainville jnumainville marked this pull request as ready for review November 13, 2023 22:20
replay_lock: LockType = "shared",
) -> None:
"""
Listen to a table and call a listener when the table updates.
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs more red flashing lights and warning tape - by using this on a single table in a component, the rest of the component is restricted from snapshotting (i.e. calling to_pandas, among other things) any table that isn't this table or one of its ancestors. The listen() is probably best used directly when the updates will be consumed, but nothing else will happen - and since almost any user operation here would need to manipulate some other use_state managed values, it will probably cause a re-render. Safe enough if there is only one table, or nothing will get a snapshot or attempt to read values from anything other than an upstream of what the listener was attached to...

Could we have a safeguard of some kind, ensure that a component either has a listener or has a snapshot, but never both?

Or, could we have an explicit "call set on this use_state, but only render in another thread (after the current update cycle is finished)" - state changes could accumulate, and run "later" to actually re-render and notify the client? There is still risk with this - an update could be missed (but a snapshot of table state will always be consistent), though as I understand it this is consistent with the react model?

(This probably invites another discussion of "what if I have two different to_pandas() calls, how do I know to use the appropriate lock for the entire component", but we could handle that separately as this evolves?)

Copy link
Member

Choose a reason for hiding this comment

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

@niloc132 All these caveats you mention - we could really use more documentation on using table listeners, if that's the case: https://deephaven.io/core/docs/how-to-guides/table-listeners-python/
It doesn't mention anything there about snapshots or locks. On the docs for the listen method itself: https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.listen
It mentions a replay_lock, but doesn't really link to the locking docs: https://deephaven.io/core/docs/conceptual/query-engine/engine-locking/

Anyways - for use_state set updates, we should absolutely be queuing those updates up and then re-rendering "later" (after the current call returns). And yes that would be consistent with the React model. #116

I think that would address the main part of "red flashing lights" part you're talking about? I think the first usage of this use_table_listener hook was to build a use_viewport_data hook afterwards (@jnumainville is currently working on spec'ing that out), so if we can at least get that case wired up, that's all we need at the moment.

Returns:
partial[[TableUpdate, bool], None]: The wrapped listener.
"""
return partial(listener_with_ctx, make_user_exec_ctx(), listener)
Copy link
Member

Choose a reason for hiding this comment

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

unless you're passing args to it, looks like this should be get_exec_ctx() instead (suggestion for demonstration purposes, import has to be adjusted too)

Suggested change
return partial(listener_with_ctx, make_user_exec_ctx(), listener)
return partial(listener_with_ctx, get_exec_ctx(), listener)

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Add an example in the examples/README.md file.

Comment on lines 170 to 171
# Slight delay to make sure the listener has time to run
sleep(1)
Copy link
Member

Choose a reason for hiding this comment

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

Not crazy about having a sleep in here. Unsure if there's a way you can force the table replayer to replay one more step in tests, or if we can mock out the table. @niloc132 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I switched to a DynamicTableWriter as it seemed a little cleaner. I also switched the sleep for a wait using threading. This at least will run the test as fast as possible without needing to mock.


def listener(update: TableUpdate, is_replay: bool) -> None:
nonlocal event
event.set()
Copy link
Member

Choose a reason for hiding this comment

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

Nice, yea I like this way better.

@ui.component
def monitor_changed_data(source: Table):

changed, set_changed = ui.use_state(empty_table(0))
Copy link
Member

Choose a reason for hiding this comment

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

This would look smoother using a TablePublisher, as we wouldn't have to recreate the table each time. Would've also been fine to just output text, as creating another table isn't really the focus of this example. This is fine though.

@jnumainville jnumainville merged commit bbb0a65 into deephaven:main Nov 30, 2023
11 checks passed
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.

Create use_table_listener hook
3 participants