deephaven / deephaven-core

Deephaven Community Core
Other
255 stars 81 forks source link

Re-running perfmon functions do not re-open closed tables #3097

Open dsmmcken opened 1 year ago

dsmmcken commented 1 year ago
  1. Run the perfmon function below
  2. Click close on the table in the UI console
  3. Re-run the command

Expected: Table re-opens

Actual: Table is not re-created

v0.18

import deephaven.perfmon as pm

# table doesn't re-open if table is closed and re-ran
qopl = pm.query_operation_performance_log()
rcaudy commented 1 year ago

I think the issue is in the nature of how the UI chooses which tables to open automatically.

nbauernfeind commented 1 year ago

This is because it is the same instance of the table. We check for state changes on the boundaries of a REPL execution. However if the variable does not change values, then it cannot be detected. The methods return singleton instances of the result tables (including cached derivatives from the singleton sources).

To detect this as a change, the methods must return a new table instance on every invocation. Is this a desirable alternative?

rcaudy commented 1 year ago

Not really, no.

nbauernfeind commented 1 year ago

I looked into the groovy repl interface and recall that there didn't seem to be any way to see variables that were touched during the groovy execution -- only that we could see the state before and the state after.

@jmao-denver Can we detect variables that were "touched" but not "changed" from a python execution? @jcferretti says he feels that Jupyter has a more natural behavior here; so I wonder if python offers this feature. Since it wasn't available in groovy I felt that I had looked far enough to create a cohesive experience for both languages; but maybe this is something nice the python users could have that groovy ones don't.

mofojed commented 1 year ago

I think the issue is in the nature of how the UI chooses which tables to open automatically.

The Web UI decides which tables to open based on the results of the command. The result of this command is showing no variables updated or changed, so we don't have any indication of something to show.

Reason is that the variable isn't actually "changed". qoplLogger gets created in MemoryTableLoggers at initialization, then pm.query_operation_performance_log() just calls static function TableLoggers.queryOperationPerformanceLog. Variable changes in Python are detected by calling PythonDeephavenSession.takeSnapshot() and applying the diff in AbstractScriptSession. While perfmon.py does make a new Table python wrapper for each performance table returned, PythonDeephavenSession unwraps variables before comparing so that wrapper doesn't matter, and the underlying Java variable stays the same and therefore doesn't count as a change.

So we could change the Python session to not unwrap the variable before comparing, but that likely has some other undesirable side effects.

I think ultimately we do want to just return a copy() of the table in TableLoggers, and then that will resolve this issue. Either there or in MemoryTableLoggers... is there much concern returning a copy there instead of a shared instance of the table?

rcaudy commented 1 year ago

copy() is not free. We should not use it just to tickle the scope and trick it into thinking that something changed.

mofojed commented 1 year ago

Then I'm not sure there's anything we can change to address this ticket, other than recommend changing their query when working with static tables like this, by adding a cheap operation, e.g.:

import deephaven.perfmon as pm
qopl = pm.query_operation_performance_log().update_view("x=i")

Though we also would have an issue with memoized results (like sort) as we do with static tables. E.g. this is producing the same table when run twice:

import deephaven.perfmon as pm
qopl = pm.query_operation_performance_log().sort("StartTime")
mofojed commented 1 year ago

We should at least update the documentation here to indicate these tables are static: https://deephaven.io/core/docs/how-to-guides/performance-tables/#creating-the-tables

Or change the API to fetch a new copy of these tables rather than a static one.

jmao-denver commented 1 year ago

Just to think out loud here, after the rerun, variable 'qopl' is reassigned to a new Table instance, although the j_table attribute points to the same Java table and it is wha the script session uses . The refcount on j_table remains the same, so we can't rely on that. Maybe the script session should instead check the wrapped vars in the scope? BTW I am referring to PythonDeephavenSession.java

mofojed commented 1 year ago

Maybe the script session should instead check the unwrapped vars in the scope?

I think you mean wrapped vars, but either way that does not address the Groovy scope.

jmao-denver commented 1 year ago

Maybe the script session should instead check the unwrapped vars in the scope?

I think you mean wrapped vars, but either way that does not address the Groovy scope.

Corrected, thanks. Yes, the idea definitely can't be applied to Groovy.

mofojed commented 1 year ago

I'm not sure what other side effects that would have, but I'm not opposed to it

rcaudy commented 1 year ago

Really, applying any table op (and adding a link to the listener chain, etc) feels like a heavyweight solution. I'm not sure a good solution exists, to be fair.

mofojed commented 1 year ago

Let's just add a note to the docs then, it's somewhat misleading calling it "creating the tables" when it doesn't actually "create" them when you call the function, just retrieves them: https://deephaven.io/core/docs/how-to-guides/performance-tables/#creating-the-tables

rcaudy commented 1 year ago

I wonder if we could have some kind of explicit advertisement message sent along one of the existing streams to the client. I think we have enough thread local context to know if we're doing work on behalf of a particular client.

mofojed commented 1 year ago

Advertisement message, as in printing something to console?

rcaudy commented 1 year ago

No, as in a gRPC message. We have an export notification stream already, I believe. Could overload that.

mofojed commented 1 year ago

Hopefully in such a way that it's transparent for clients? E.g. The changes in the ExecuteCommandResponse are filled in with these perf tables: https://github.com/deephaven/deephaven-core/blob/4da3f557843aa00cc4a0da55fd53a71cc29fe3cb/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java#L178

dsmmcken commented 1 year ago

For reference, Jupyter has a display function that is used to trigger displaying an object in the UI, that implements custom repr()s for formatting.