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

deephaven.ui stock rollup example doesn't work #265

Closed
mofojed opened this issue Feb 6, 2024 · 0 comments · Fixed by #268
Closed

deephaven.ui stock rollup example doesn't work #265

mofojed opened this issue Feb 6, 2024 · 0 comments · Fixed by #268
Labels
bug Something isn't working triage

Comments

@mofojed
Copy link
Member

mofojed commented Feb 6, 2024

Description

Run the Stock Rollup example from https://github.com/deephaven/deephaven-plugins/tree/main/plugins/ui/examples#stock-rollup, and you get some weird errors. You can also do a minimal example to reproduce:

from deephaven import empty_table, ui

t1 = empty_table(100).update("x=i")
t2 = empty_table(100).update("y=i")

@ui.component
def bad_component():
    clicked, set_clicked = ui.use_state(False)

    return [
        ui.toggle_button("Switch Table", on_change=set_clicked),
        t2 if clicked else t1
    ]

bc = bad_component()

Steps to reproduce

  1. Run the following snippet:
from deephaven import empty_table, ui

t1 = empty_table(100).update("x=i")
t2 = empty_table(100).update("y=i")

@ui.component
def bad_component():
    clicked, set_clicked = ui.use_state(False)

    return [
        ui.toggle_button("Switch Table", on_change=set_clicked),
        t2 if clicked else t1
    ]

bc = bad_component()
  1. Click on the "Switch Table" button to switch between two tables

Expected results
2. Table should switch

Actual results
2. Table switches once but then does not switch back. An error in the browser console logs:

An unexpected error occurred while executing "documentUpdated" JSON-RPC method: Error: Invalid exported object key 0
    at Array.eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:5926:19)
    at JSON.parse (<anonymous>)
    at eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:5913:31)
    at eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:5977:29)
    at eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4787:24)
    at callMethod (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4892:18)
    at noopMiddleware (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4924:10)
    at JSONRPCServer2.callMethod (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4904:51)
    at JSONRPCServer2.eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4853:31)
    at step (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4727:19)
    at Object.eval [as next] (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4674:14)
    at eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4660:67)
    at new Promise (<anonymous>)
    at __awaiter$1 (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4642:10)
    at JSONRPCServer2.receiveSingle (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4843:14)
    at JSONRPCServer2.receive (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4816:21)
    at JSONRPCServerAndClient2.eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:5120:38)
    at step (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:5045:19)
    at Object.eval [as next] (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4992:14)
    at eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4978:67)
    at new Promise (<anonymous>)
    at __awaiter (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:4960:10)
    at JSONRPCServerAndClient2.receiveAndSend (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:5108:14)
    at receiveData (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:5996:48)
    at eval (eval at <anonymous> (loadRemoteModule.ts:42:20), <anonymous>:6004:9)
    at Object.$lambda$3 (dh-core.js:8217:5)
    at Function.onInvoke_12 (dh-core.js:8935:16)
    at lambda (dh-core.js:174:22)
    at Array.forEach (<anonymous>)
    at Object.$fireEvent_0 (dh-core.js:8193:15)
    at Object.$lambda$6_5 (dh-core.js:28705:11)
    at Function.apply_170 (dh-core.js:28932:10)
    at lambda (dh-core.js:174:22)
    at dh-internal.js:1:818486
    at Array.forEach (<anonymous>)
    at dh-internal.js:1:818465
    at dh-internal.js:1:43546
    at Array.forEach (<anonymous>)
    at e.rawOnMessage (dh-internal.js:1:43508)
    at dh-internal.js:1:41321
    at Array.forEach (<anonymous>)
    at e.onTransportChunk (dh-internal.js:1:41196)
    at Object.$onMessage (dh-core.js:21056:35)
    at MultiplexedWebsocketTransport$3methodref$onMessage$Type.handleEvent_2 [as handleEvent] (dh-core.js:21200:10)

Additional details and attachments
It seems client is throwing the object out because it no longer had a reference to it, but server still had it cached. Should be more explicit about when an exported object is no longer needed or is re-used. Probably just need the server to only keep the object IDs from the last render, and not current renders.
Only matters for exported objects, since we need the actual object client side to fetch the child data. Callables should be safe to use the weak key, since the client will just call the callback by ID.

Versions

deephaven.ui v0.6.0

@mofojed mofojed added bug Something isn't working triage labels Feb 6, 2024
mofojed added a commit to mofojed/deephaven-plugins that referenced this issue Feb 7, 2024
- Referenced a table `t` that did not exist, corrected to reference `formatted_table`
- Needed to refactor how exported objects were being handled - server was just using a WeakKeyDictionary, but the objects were not guaranteed to get cleared out on the server and were being reused between re-renders. The client would always dump old objects after a render, so just always clear out the objects on the server from the previous render. Note that the WeakKeyDictionary is still safe for callables; the client just references them by ID and does not hold a reference to an object, so they can be cleared by the server GC.
- Fixes deephaven#265
- Tested using the Stock Rollup example fixed in this PR, it now works correctly.
- Also tested with snippet from deephaven#128, ensuring the table still does not flicker.
- Added another test to test more than one iteration
mofojed added a commit that referenced this issue Feb 8, 2024
- Referenced a table `t` that did not exist, corrected to reference
`formatted_table`
- Needed to refactor how exported objects were being handled - server
was just using a WeakKeyDictionary, but the objects were not guaranteed
to get cleared out on the server and were being reused between
re-renders. The client would always dump old objects after a render, so
just always clear out the objects on the server from the previous
render. Note that the WeakKeyDictionary is still safe for callables; the
client just references them by ID and does not hold a reference to an
object, so they can be cleared by the server GC.
- Fixes #265
- Tested using the Stock Rollup example fixed in this PR, it now works
correctly.
- Also tested with snippet from #128, ensuring the table still does not
flicker.
- Added another test to test more than one iteration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant