-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
replay_lock: LockType = "shared", | ||
) -> None: | ||
""" | ||
Listen to a table and call a listener when the table updates. |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)
return partial(listener_with_ctx, make_user_exec_ctx(), listener) | |
return partial(listener_with_ctx, get_exec_ctx(), listener) |
There was a problem hiding this 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.
# Slight delay to make sure the listener has time to run | ||
sleep(1) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
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