feat: bidirectional support by jnumainville · Pull Request #34 · deephaven/deephaven-plugins

jnumainville · 2023-09-12T17:52:04Z

Fixes #2

Adds python support for bidirectional communication and updating of plots with plot bys.
Tested with Matt's front-end changes (and will need to be merged with those).
Simple example plot:

import deephaven.plot.express as dx
from deephaven import time_table
import random 

sourceh = time_table("PT2S").update(formulas=[
    "X = (int)random.gauss(20, 5)", 
    "Z = (float)random.gauss(30, 3)",
    "C = i % 20"
    ])

fig = dx.scatter(sourceh, x="X", y="Z",
    by="C",
)

Also has some miscellaneous cleanup and refactoring.

jnumainville · 2023-10-04T19:03:17Z

+        """
+        # only one export can be done at a time to ensure that the references
+        # are correct
+        exporter.acquire()


Am going to move this to a context manager, although I wonder if I have a correct understanding that a TableListener can lead to a race condition. My understanding on how the java and python interact here is weak.

Do you need a lock, and not just get the execution context?: https://deephaven.io/core/docs/conceptual/execution-context/#when-an-executioncontext-is-needed

This lock may have been doing something else that is necessary. I'm getting fairly frequent deadlocks when plotting w/ the latest change. Usually on re-creating a figure, but have gotten it once on the first time I plotted something after a fresh server start

Nevermind I was able to deadlock even building off the commit before the lock was removed

I'm seeing those deadlocks too. I'm not sure where they're coming from. It seems to be the whole python interpreter that gets locked.

@mattrunyon It might have been some figure locks I had that shouldn't also be needed. I removed them and there doesn't seem to be deadlocks any more.
I am still seeing odd behavior when switching between figure tabs. Is there any possibility when switching on and off a figure that there is some lingering data in the window that could mess up the new figure creation? As far as I can tell the proper updates are being sent.

Pushed a fix for that stale data issue. Also fixed a double loader that appeared from some CSS changes to web-client-ui.

We'll need to update to non-beta packages on the next release to get TS fixes in for this. If there's no other issues w/ this PR though, could just ts-ignore the TS issue and fix it in a future PR when changing to a WidgetPlugin

mattrunyon · 2023-10-07T00:03:43Z

Added JS bidirectional support. Looks like I need to rebase on main though.

There are some issues w/ recreating a plot w/ the same name (i.e. run the same code twice in a row in the repl). There is an error printed to the user console because something is double closing I think and causing the error (no way to detect it's already been closed). This also seems to be an issue w/ our tables and charts, but it's a JS error/warning.

Turns out in Pending.cancel we call the cleanup on all promises, not just the pending promises. That causes the double close on unmount for everything I think.

mofojed · 2023-10-13T20:06:14Z

@@ -1,6 +1,6 @@
 {
  "name": "deephaven-js-plugins",
-  "lockfileVersion": 3,
+  "lockfileVersion": 2,


@jnumainville I think you may be using the wrong version of npm. Run nvm install to use the correct version, and/or you can set up your environment to automatically switch: https://github.com/nvm-sh/nvm#deeper-shell-integration

fixed I think?

mofojed · 2023-10-13T20:23:45Z

+        """
+        # only one export can be done at a time to ensure that the references
+        # are correct
+        exporter.acquire()


Do you need a lock, and not just get the execution context?: https://deephaven.io/core/docs/conceptual/execution-context/#when-an-executioncontext-is-needed

jnumainville · 2023-10-27T18:57:26Z

something funky is going on with removing points, especially in a layer
in a plot by, groups are being dropped so some aren't appearing
am investigating if that is a python issue

Example

import deephaven.plot.express as dx
from deephaven import time_table
import random 

sourceh = time_table("PT1S").update(formulas=[
    "X = (int)random.gauss(20, 5)", 
    "Y = (int)random.gauss(20, 5)", 
    "Z = (float)random.gauss(30, 3)",
    "l1 = i % 20",
    "l2 = i % 30",
    ]).tail(10)

fig1 = dx.scatter(sourceh, x="Z", y="Y",
    by="l1",
    color_discrete_sequence=["lemonchiffon"]
)

fig2 = dx.scatter(sourceh, x="Z", y="Y",
    by="l2",
    color_discrete_sequence=["salmon"]
)

layered = dx.layer(fig1, fig2)

it almost seems like figures aren't being updated properly if you're not viewing them?

jnumainville · 2023-10-27T19:27:11Z

@mattrunyon it seems like when I switch panels (from looking at one figure to not then back to looking at it) a new connection is set up but the old one is still there. Do you know how to stop that?

here's an example of the resulting new references, old references, connection id (I see the wrong ids being removed too possibly? investigating that)
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9] [] 11933186656
[10] [2] 11933186656
[11] [3] 11933186656
[12] [4] 11933186656
[13] [5] 11933186656
[14] [6] 11933186656
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9] [] 12224624144
[15] [7] 11933186656
[10] [7] 12224624144
[16] [8] 11933186656
[11] [8] 12224624144
[17] [9] 11933186656
[12] [9] 12224624144

code is in previous comment, but only ran figure 1 then switched off and back on the figure panel

jnumainville · 2023-10-31T21:59:11Z

Everything seems to be working properly after Matt's fixes

jnumainville · 2023-11-02T19:00:04Z

After discussion with Colin, the locks are something should be done, so I have added them back in.
The locks in the figure DAG (DeephavenLayerNode and DeephavenFigureNode) were causing a deadlock because some table operations in the figure regeneration needed to lock but couldn't because of the lock in the node.
I have worked around this by not locking the figure generation itself, only the update of the figure in the node, with a revision number to ensure that an older update doesn't overwrite a newer update.
I wrote a RevisionManager to deal with this issue, so I also converted the exporter-related locks to use the RevisionManager.

jnumainville · 2023-11-03T22:17:08Z

@mattrunyon I have (hopefully correctly) added the tables to the liveness scope
I also did something I should have already had, where each session/listener makes a copy of the deephaven figure and the listener updates its specific figure. I think I removed I removed it because it seemed like overkill (and I think even now some optimizations should be done eventually), but if you're going to have session specific liveness scopes that need to keep track of tables it makes sense anyways. This has the perhaps useful in the future benefit of making it much easier to make session specific updates to the figure, like one_clicks, if we ever wanted to implement those directly into the figure.
Essentially, the session liveness scope is carried along for the figure copy, adding any tables that are in the figure DAG. These are removed on del, either of the base figure or the copy (as well as the session, as before).

jnumainville · 2023-11-07T01:06:55Z

@mattrunyon fixed #2 with an empty default figure in the partition case.
The only weird thing is that I have to click and drag the figures to get them to start working

code I used:

import deephaven.plot.express as dx
from deephaven import time_table
import random 

sourceh = time_table("PT1S").update(formulas=[
    "X = (int)random.gauss(20, 5)", 
    "Y = (int)random.gauss(20, 5)", 
    "Z = (float)random.gauss(30, 3)",
    "l1 = i % 20",
    "l2 = i % 30",
    ]).tail(5)

fig1 = dx.scatter(sourceh, x="Z", y="Y",
    by="l1",
    color_discrete_sequence=["lemonchiffon"]
)

fig2 = dx.scatter(sourceh, x="Z", y="Y",
    by="l2",
    color_discrete_sequence=["salmon"]
)

layered = dx.layer(fig1, fig2)

mofojed · 2023-11-07T21:54:06Z

+    this.tableColumnReplacementMap.forEach((columnReplacements, tableId) => {
+      const tableData = this.tableDataMap.get(tableId);
+      if (tableData == null) {
+        // log.warn('No tableData for this event. Skipping update');


Uncomment? Or log.debug?

I changed it to throw. It shouldn't ever happen, but if it does then there's probably some out of sync state or stale state laying around

Can change back to a warning if you think that's right. It doesn't cause an issue to ignore it, but the plot might not be correct if this case were to happen. Prefer error over showing potentially incorrect data?

Yup error over showing incorrect.

I need to fix something for initial loading actually with this. The chart is finished loading after the model initializes, but before the tables are fetched which leads to this throwing

Ok I have changed this to throw and set an empty tableData when the table starts fetching (previously I set the data after the table was fetched). There could be states (like initial load) where the table is being fetched and doesn't have its data yet, so that's why I added the default empty tableData

The error should only throw if something bad happens or somehow gets out of sync. Mostly there so TS doesn't complain about the possibility of null

mofojed · 2023-11-07T22:02:06Z

+    // TODO: Add this back when JsWidget doesn't log a Python error when already closed
+    // This issue occurs when you recreate a plot in the REPL as the engine closes its side
+    // but the widget doesn't know and unsubscribe on panel unmount causes this close
+    // this.widget?.close();


Is there a ticket already for this? I know Colin said he was going to fix it, but did it slide off his radar?

deephaven/deephaven-core#4604

I'm going to remove this comment and uncomment the code. Either way there's errors printed to the user's console, but I think once that bug is fixed this will be the correct way to do it

jnumainville · 2023-11-09T21:18:39Z

At this point I have resolved all bugs and comments I am aware of in the python side of the application

mattrunyon · 2023-11-09T21:35:51Z

I believe the JS side has any known issues fixed as well

jnumainville added 3 commits September 12, 2023 12:45

bidirectional support

e184c92

cleanup

3676fc1

cleanup

1830363

jnumainville requested review from mattrunyon and mofojed September 12, 2023 20:47

jnumainville self-assigned this Sep 13, 2023

only emit new references

5ac4b2b

jnumainville changed the title ~~bidirectional support~~ feat: bidirectional support Sep 28, 2023

diff refferences mostly working

5373e27

jnumainville commented Oct 4, 2023

View reviewed changes

JS bidirectional communicaton

abdbd5e

mattrunyon added 2 commits October 8, 2023 17:39

Merge remote-tracking branch 'upstream/main' into pr/jnumainville/34

11c2e60

Update package-lock

feb2b74

jnumainville marked this pull request as ready for review October 9, 2023 14:59

jnumainville added 6 commits October 12, 2023 14:14

some fixes

e87c2e2

another fix

78664d5

test ip

e201240

finished tests

de28e2b

refactor

0ddb1a1

fixing order inconsistency

f7a92ed

mofojed reviewed Oct 16, 2023

View reviewed changes

inspection comments

acc1a1f

jnumainville and others added 3 commits October 31, 2023 10:20

removing figure locks

45752bf

Fix refetch and double loader

a2a7cdd

clean up

65d578c

new locks

d7a8838

mattrunyon and others added 2 commits November 3, 2023 12:22

Update to non-beta JS packages

b3e12f9

adding liveness scope

bfd9c5f

mattrunyon and others added 3 commits November 6, 2023 08:55

Merge remote-tracking branch 'upstream/main' into bidirectional

e8e9f0b

using correct function

a4e9791

fixing liveness scope

c912963

mattrunyon requested a review from mofojed November 6, 2023 20:58

fixed no figures error

d91bec3

mofojed requested changes Nov 7, 2023

View reviewed changes

mattrunyon and others added 10 commits November 8, 2023 15:09

Fix table listener sometimes getting garbage collected

b90af65

comments and fixes

01a78a6

Merge remote-tracking branch 'upstream/main' into bidirectional

4e0cb6f

Remove dashboardplugin

40bcb5c

Fix build errors

bc423c8

Address review comments

cc68593

snapshot error

f61fae2

bug fixes

0e601ac

removing unneeded code

6b900b8

Merge remote-tracking branch 'fork/bidirectional' into bidirectional

835866a

Fix plot initial load with no traces

76d6e46

mattrunyon requested a review from mofojed November 9, 2023 21:36

mofojed approved these changes Nov 10, 2023

View reviewed changes

jnumainville merged commit 9e868ab into deephaven:main Nov 10, 2023

Conversation

jnumainville commented Sep 12, 2023 • edited by mattrunyon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnumainville Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattrunyon commented Oct 7, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnumainville commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnumainville commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnumainville commented Oct 31, 2023

Uh oh!

jnumainville commented Nov 2, 2023

Uh oh!

jnumainville commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnumainville commented Nov 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattrunyon Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnumainville commented Nov 9, 2023

Uh oh!

mattrunyon commented Nov 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jnumainville commented Sep 12, 2023 •

edited by mattrunyon

Loading

jnumainville Oct 4, 2023 •

edited

Loading

jnumainville commented Oct 27, 2023 •

edited

Loading

jnumainville commented Oct 27, 2023 •

edited

Loading

jnumainville commented Nov 3, 2023 •

edited

Loading

mattrunyon Nov 9, 2023 •

edited

Loading