ObjectType and Plugins #1787

devinrsmith · 2021-12-30T21:29:47Z

devinrsmith · 2021-12-30T21:44:45Z

To try out locally, modify dockerfile docker/server/src/main/docker/Dockerfile and then prepareCompose:

# To test out the deephaven-plugin-matplotlib, uncomment the following:
#RUN set -eux; \
#    python3 -m pip install -q --no-cache-dir \
#    importlib-metadata \
#    https://github.com/deephaven/deephaven-plugin-matplotlib/releases/download/v0.0.1.dev1/deephaven_plugin_matplotlib-0.0.1.dev1-py3-none-any.whl

Start server, run script:

import matplotlib.pyplot as plt

x = [0, 2, 4, 6]
y = [1, 3, 4, 8]
plt.plot(x, y)
plt.xlabel('x values')
plt.ylabel('y values')
plt.title('plotted x and y values')
plt.legend(['line 1'])

m_figure = plt.gcf()

Client tests:

./gradlew java-client-session-examples:installDist
./java-client/session-examples/build/install/java-client-session-examples/bin/fetch-object --variable m_figure > /tmp/m_figure.png
./java-client/session-examples/build/install/java-client-session-examples/bin/subscribe-fields

…n tests

engine/table/src/main/java/io/deephaven/engine/util/PythonDeephavenSession.java

niloc132 · 2022-01-03T21:14:54Z

engine/table/src/main/java/io/deephaven/engine/util/PythonDeephavenSession.java

+
+        private final PyDictWrapper dict;
+
+        public PythonSnapshot(PyDictWrapper dict) {


aren't these snapshot types just wrappers around the getVariables/getVariable calls? why does each lang need to reimplement the diff?

The old version was very inefficient (getVariables is very inefficient for python). In general, I think our ScriptSession interface as a top-level programmatic object leaves a lot to be desired, esp wrt python. Pushing some of the compute changes logic down into python allows us to avoid crossing the JNI boundary as much. I'd like to take it further at some point, as I think more logic can/should be moved into python.

Okay, I'll let this be deferred, but while I also dislike the current AbstractScriptSession/QueryScope abstraction/relationship, I dont think we're tearing out enough in this patch, or designing (i.e. with review) the new approach/api being put here.

If it is a good idea to remove, I'd rather do it in a complete way.

plugin/figure/src/main/java/io/deephaven/figure/FigureWidgetTranslator.java

plugin/src/main/java/io/deephaven/plugin/Plugin.java

plugin/src/main/java/io/deephaven/plugin/type/Exporter.java

settings.gradle

niloc132 · 2022-01-03T22:53:53Z

server/src/main/java/io/deephaven/server/plugin/PluginModule.java

+import dagger.Module;
+
+@Module(includes = {ObjectTypesModule.class})
+public interface PluginModule {


does this serve a purpose on its own? it seems like it just pulls in the plugin.ObjectTypesModule?

I've got a follow up PR for an application plugin; instead of refactoring the structure in the application plugin, I'm providing the follow-up structure for it here.

niloc132 · 2022-01-03T22:56:58Z

server/src/main/java/io/deephaven/server/plugin/ObjectTypes.java

+        return Optional.empty();
+    }
+
+    public synchronized void register(ObjectType type) {


is there no public API to programmatically register a new ObjectType? This class is package-protected, and ObjectTypeLookup doesnt declare this method

Right now, I'm thinking that it's only via plugin mechanisms. We could expose the register, but I think it has some non-obvious implications for the script session (and application fields) changeListener wrt "displayble" types.

I'm definitely imagining a use case where a java/groovy user would want to iterate without knocking over the server with every change - just keep calling register (and a matching unregister?) as you tweak the class definition in the console, fetch from the client to work on it. Probably matters less locally, but maybe more on a server deployment.

This might just be in my head - but with such a flexible mechanism (and already threadsafe etc), it seems like a shame to keep it from power users.

punting for now #1809

server/src/main/java/io/deephaven/server/plugin/ObjectTypesModule.java

docker/server/src/main/docker/Dockerfile

engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java

engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java

engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java

engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java

rcaudy · 2022-01-03T23:28:09Z

plugin/src/main/java/io/deephaven/plugin/type/ObjectTypeClassBase.java

+    @Override
+    public boolean equals(Object o) {
+        if (this == o)
+            return true;
+        if (o == null || getClass() != o.getClass())
+            return false;
+        ObjectTypeClassBase<?> that = (ObjectTypeClassBase<?>) o;
+        if (!name.equals(that.name))
+            return false;
+        return clazz.equals(that.clazz);
+    }
+
+    @Override
+    public int hashCode() {
+        int result = name.hashCode();
+        result = 31 * result + clazz.hashCode();
+        return result;
+    }


These seem unnecessary to define.

If ObjectTypeClassBase instances can handle subtypes (as they seem to be able to do, except in the actual registry impl), this could be necessary... something to this effect anyway, traverse the already assembled instances to figure out which one applies to a given object (imagine registering, for example, Object, Number, and Double, make sure String gets the Object instance, Long gets the Number instance, etc)

I don't mean for it to handle subtypes ATM. We can update the impl in the future if necessary. I think Ryan is correct, will remove equals/hashcode atm.

proto/proto-backplane-grpc/src/main/proto/deephaven/proto/application.proto

niloc132 · 2022-01-04T01:09:36Z

Sorry I ran out of time to write a summary, and wanted to get these earlier thoughts to you. My main worry is that there is too much implementation and not enough API - there are roughly two APIs here, a gRPC/pb one for clients, and a java/python api to declare/instantiate/register the handlers. I mostly would like some improvement at this point on the gRPC/pb side, as I think that is the part we've thought out the most from a consumption perspective the most (and the case where we most want to avoid more than one way to do something).

My main points to stick to:

console and application should use the same (in terms of 'shape', not actually the same stream, probably) "This is a named thing with a type", "get this thing, assuming it is this type", "tell me when any bindings change" services.
Ditto table attributes, so we can fetch complex objects (including tables) in a consistent way across these 2.5 cases.
dh-written client apis should be able to (but not required to) recognize and deserialize the data, but must handle turning internal objects into a real thing. Figures are the main discussed example of this - the js api for example will decode the figure and plug in the various tables, but other APIs might not actually care about the figure contents, and just want to turn internal tickets into tables.

Let's try to chat a bit tomorrow, I'll generate the gwt bindings so that the client refactor can get an update and we can give it a shakedown on figures, expose matlibplot to js?

server/src/main/java/io/deephaven/server/session/SessionState.java

niloc132 · 2022-01-21T20:33:08Z

server/src/main/java/io/deephaven/server/plugin/type/ObjectTypes.java

+@Singleton
+public final class ObjectTypes implements ObjectTypeLookup, ObjectTypeRegistration {
+
+    private static final Set<String> RESERVED_TYPE_NAMES_LOWERCASE = Set.of("table", "tablemap", "treetable", "");


should we reserve figure in some way? not sure of the mechanics of how this works, if it makes any more sense to reserve tablemap than figure...

I'm more inclined to want to namespace Figure as something like io.deephaven.Figure (doesn't have to be the full Figure class, but something that could be stable and survive implementation refactorings). As it is now, if somebody else tries to install a "Figure" object type, we'll blow up at startup.

Should verify that "tablemap" is going to be a thing (with that word) - but while i agree that our "table" should win over other tables, i'm not sure why our tablemap or treetable would be any more essential than our figures when it comes to serialization to a client.

Assuming we can revisit this later.

server/build.gradle

proto/proto-backplane-grpc/src/main/proto/deephaven/proto/application.proto

server/src/main/java/io/deephaven/server/object/ObjectServiceGrpcImpl.java

java-client/session-examples/src/main/java/io/deephaven/client/examples/FetchObject.java

server/src/main/java/io/deephaven/server/object/ObjectServiceGrpcImpl.java

ObjectType and Plugins

a24ec60

devinrsmith added DocumentationNeeded plug-ins labels Dec 30, 2021

devinrsmith added this to the Jan 2022 milestone Dec 30, 2021

devinrsmith requested a review from rcaudy December 30, 2021 21:29

devinrsmith self-assigned this Dec 30, 2021

devinrsmith added 2 commits December 30, 2021 13:54

Remove old custom type name

ecf7e80

Fix python integration tests

91bd98a

This was referenced Dec 31, 2021

Move Figure protobuf into Figure plugin #1790

Open

Create python plugin ABCs #1791

Closed

devinrsmith added 3 commits December 31, 2021 10:25

Move logic into deephaven.server.plugin

74aec62

spotless

ebda9d1

Move deephaven.server into deephaven2.server so fix broken integratio…

968ec24

…n tests

niloc132 reviewed Jan 3, 2022

View reviewed changes

rcaudy reviewed Jan 4, 2022

View reviewed changes

devinrsmith added 3 commits January 4, 2022 17:26

Review responses

bb638e6

f

fe8414f

f

e4da7bb

devinrsmith mentioned this pull request Jan 6, 2022

Support runtime registration for plugins #1809

Open

devinrsmith added 4 commits January 6, 2022 12:03

better dagger and docs

8ee1947

f

354a5a7

f

edc0617

Merge remote-tracking branch 'upstream/main' into fetch-object

ca1875d

devinrsmith mentioned this pull request Jan 6, 2022

Use service loader to abstract the different plugin auto-discovery methods #1810

Open

devinrsmith added 2 commits January 6, 2022 14:27

Stuff

781cbf2

remove test from dockerfile

9de9a46

devinrsmith requested a review from niloc132 January 6, 2022 22:30

wrong cast

44bec3b

devinrsmith added 2 commits January 20, 2022 06:43

spotless

fe57ee7

Copy regenerated _pb2

eb9863c

devinrsmith requested review from JamesXNelson, jcferretti and mofojed as code owners January 20, 2022 22:37

devinrsmith added 2 commits January 21, 2022 09:06

Final fixes

1811bb3

Merge remote-tracking branch 'upstream/main' into fetch-object

33cf3d2

devinrsmith force-pushed the fetch-object branch from cc1a9c9 to 33cf3d2 Compare January 21, 2022 17:08

niloc132 reviewed Jan 21, 2022

View reviewed changes

server/src/main/java/io/deephaven/server/session/SessionState.java Outdated Show resolved Hide resolved

niloc132 reviewed Jan 21, 2022

View reviewed changes

server/build.gradle Outdated Show resolved Hide resolved

niloc132 reviewed Jan 21, 2022

View reviewed changes

proto/proto-backplane-grpc/src/main/proto/deephaven/proto/application.proto Outdated Show resolved Hide resolved

devinrsmith and others added 7 commits January 21, 2022 13:16

plumb TypedTicket

b298d47

regen and copy python proto

cf36e5b

Update test

e492010

Update gwt bindings

95ada5c

Update JS API to use typedticket for object fetch

15296cb

Remove Reference.ticket

d28c82d

fix python side for Reference

a995b11

niloc132 reviewed Jan 24, 2022

View reviewed changes

FetchObject client don't try Table. Better exceptions for FetchObject

d736c85

devinrsmith requested a review from niloc132 January 24, 2022 17:21

This was referenced Jan 24, 2022

StatusRuntimeException from inside submit not propogated #1871

Closed

Optimize ObjectTypeLookup #1872

Open

devinrsmith added 2 commits January 24, 2022 10:15

Add reference to deephaven#1872

580b272

add because notes for api dagger dependencies

ddfcdaa

niloc132 approved these changes Jan 24, 2022

View reviewed changes

devinrsmith merged commit 9853bda into deephaven:main Jan 24, 2022

devinrsmith deleted the fetch-object branch January 24, 2022 21:15

github-actions bot locked and limited conversation to collaborators Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ObjectType and Plugins #1787

ObjectType and Plugins #1787

devinrsmith commented Dec 30, 2021

devinrsmith commented Dec 30, 2021

niloc132 Jan 3, 2022

devinrsmith Jan 6, 2022

niloc132 Jan 10, 2022

niloc132 Jan 3, 2022

devinrsmith Jan 4, 2022

niloc132 Jan 3, 2022

devinrsmith Jan 4, 2022

niloc132 Jan 4, 2022

devinrsmith Jan 6, 2022

rcaudy Jan 3, 2022

niloc132 Jan 4, 2022

devinrsmith Jan 6, 2022

niloc132 commented Jan 4, 2022

niloc132 Jan 21, 2022

devinrsmith Jan 21, 2022

niloc132 Jan 21, 2022

devinrsmith Jan 21, 2022


		private final PyDictWrapper dict;

		public PythonSnapshot(PyDictWrapper dict) {

ObjectType and Plugins #1787

ObjectType and Plugins #1787

Conversation

devinrsmith commented Dec 30, 2021

devinrsmith commented Dec 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niloc132 commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment