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.
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.
@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.
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) |
mofojed
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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