Python server sync - Fixes #1480 by SuperTails · Pull Request #2456 · deephaven/deephaven-core

SuperTails · 2022-06-01T15:17:55Z

This PR adds three things.

Sessions now request the list of tables when connecting to the server, so tables created before the session starts (from previous sessions, from the web UI, etc.) can be opened.
session.subscribe_fields() now starts up a thread that checks for new changes to tables, so tables created/deleted in other sessions/the web UI become visible automatically.
Tests for the above two features.

Fixes #1480

github-actions · 2022-06-01T15:18:12Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

SuperTails · 2022-06-01T15:18:48Z

I have read the CLA Document and I hereby sign the CLA

jmao-denver

Very solid, needs only minor changes

devinrsmith

This is a good first pass, welcome to Deephaven!

devinrsmith · 2022-06-01T17:15:50Z

+        self._grpc_app_stub = application_pb2_grpc.ApplicationServiceStub(session.grpc_channel)
+
+    def list_fields(self):
+        try:
+            fields = self._grpc_app_stub.ListFields(
+                application_pb2.ListFieldsRequest(),
+                metadata=self.session.grpc_metadata
+            )
+            return fields
+        except Exception as e:
+            raise DHError("failed to list fields.") from e


Not to distract from this PR - but I think we might want to look into the python grpc asyncio APIs - it might be a more natural fit for streaming responses like this. @jmao-denver

devinrsmith · 2022-06-01T17:24:57Z

+            self._list_fields = self.app_service.list_fields()
+            self._parse_fields_change(next(self._list_fields))


Couple things:

I don't think we should be subscribing on connect.

As is, it's not correct - we've starting a list_fields subscription, but we haven't started the threading handler for it. I don't know exactly how python gRPC will handle this (either a memory leak in list_fields, or eventually an error b/c it's not being consumed).

I think we need this logic self-contained within the subscribe_fields method.

devinrsmith · 2022-06-01T17:34:44Z

+    def _parse_script_response(self, response):
+        self._parse_fields_change(response.changes)


I think we may be doubling up on our response handling for code we execute via scripts, in the case where we are also subscribing to list fields. We may want to ignore list_field changes that apply to the global script scope (FieldInfo.application_id == "scope"), or choose to only handle list_field changes (ignore script responses) when they are active.

The problem w/ the former is if we ignore the "scope" changes for list_field, that will miss updates executed in the web UI instead of via the python client.

The problem w/ the latter is that updates to the fields will be async with the response to script execution response.

I decided to go with the former option, because otherwise the user can't rely on any of the tables they create in a script being usable until after some arbitrary delay, which seems like a significant usability problem.
Plus, ignoring global-script-scope changes doesn't harm existing functionality.

SuperTails · 2022-06-02T21:52:14Z

There have been a number of changes made. The two new functions have been collapsed into one: sync_fields(repeating=bool). This is to make it more clear that calling it multiple times is redundant.
Session now takes an optional argument that effectively just defers to a sync_fields call; this is solely so that this new feature can be found easier when looking through docs.
In order to avoid problems with duplicate handling of table changes, however, the repeating/background syncing doesn't register changes made at the global scope, so it's not super useful. This does not affect the one-time sync: all tables/plots/etc are picked up and are usable as request in the issue.

SuperTails · 2022-06-03T20:07:19Z

By canceling the ListFields request before a script is run, then restarting it afterwards, it is possible to sidestep the sync/async changes issue entirely at the cost of needing to request the full list of tables again. Since this is a significant usability improvement for a mode that may or may not be very popular, this seems reasonable.

devinrsmith · 2022-06-03T21:31:01Z

+                # We can ignore the script response because
+                # all the new tables are added by this call anyways
+                self._fields = {}
+                self.sync_fields(repeating=True)


So this isn't actually true if there are other list field subscribers at the time run_script is called. If so, run_script changes get batched with a certain frequency (250ms by default), so they aren't necessarily visible on the first return from sync_fields. I think just applying both will "work".

devinrsmith · 2022-06-03T21:33:11Z

+    def sync_fields(self, repeating: bool):
+        """ Check for fields that have been added/deleted by other sessions and add them to the local list
+
+        This will start a new background thread when `repeating=True`.


It should be noted - when repeating, only the first response will modify global "scope" - future responses against global "scope" will not be handled.

Since the issue with script response/listfields response conflicts is fixed, can the repeating sync_fields be switched back to handling all changes?

Yes, I think that's correct. But my other comment is still relevant - we want run_script changes to be visible upon return.

jmao-denver · 2022-06-06T23:33:35Z

+    def __init__(self, host: str = None, port: int = None, never_timeout: bool = True, session_type: str = 'python', sync_fields: int = NO_SYNC):
        """ Initialize a Session object that connects to the Deephaven server


I'd prefer the default to be SYNC_ONCE. @jakemulf what do you think?

I think NO_SYNC might actually be the most reasonable default. I generally find that no-ops are the best defaults

Thanks @jakemulf

jmao-denver

Approve based on Jake's feedback

SuperTails added 4 commits May 31, 2022 16:02

WIP fix for deephaven#1480

0790f5c

Reorganized app service into new file

214e152

Added tests

a4b3f45

Removed extraneous imports

9fdeb0f

jmao-denver requested changes Jun 1, 2022

View reviewed changes

Comment thread py/client/pydeephaven/session.py Outdated

Comment thread py/client/tests/test_sync.py Outdated

jmao-denver assigned SuperTails Jun 1, 2022

jmao-denver added NoDocumentationNeeded python-client labels Jun 1, 2022

jmao-denver added this to the May 2022 milestone Jun 1, 2022

Minor naming and formatting fixes

3a6f665

devinrsmith requested changes Jun 1, 2022

View reviewed changes

SuperTails added 5 commits June 2, 2022 13:30

Added a way to sync tables a single time

f6b2b7d

Collapsed subscribe_fields and sync_tables into one function

3082a3c

Removed sleep from test

cb302ba

Now ignores updates from global scope

039668f

Changed many 'table' vars to 'fields' to be more accurate

3588d3e

devinrsmith reviewed Jun 2, 2022

View reviewed changes

Comment thread py/client/pydeephaven/session.py

Comment thread py/client/pydeephaven/session.py Outdated

Comment thread py/client/tests/test_multi_session.py Outdated

SuperTails added 3 commits June 3, 2022 08:53

Removed unnecessary test imports

10f6a54

Fields are now identified using scope too

2929375

SYNC_REPEATED mode no longer conflicts with script responses

ee2fd2b

Added missing dependency to CI

da55c3f

devinrsmith reviewed Jun 3, 2022

View reviewed changes

SuperTails added 3 commits June 6, 2022 09:23

Added script response check to prevent missing script tables

51f6532

Removed unnecessary scope check

45c62de

Fixed issue that prevented repeated updates from working

8c2d6d1

devinrsmith self-requested a review June 6, 2022 22:39

devinrsmith approved these changes Jun 6, 2022

View reviewed changes

jmao-denver self-requested a review June 6, 2022 23:32

jmao-denver reviewed Jun 7, 2022

View reviewed changes

jmao-denver self-requested a review June 7, 2022 03:04

jmao-denver approved these changes Jun 7, 2022

View reviewed changes

jmao-denver merged commit 9dafae1 into deephaven:main Jun 8, 2022

github-actions Bot locked and limited conversation to collaborators Jun 8, 2022

		self._list_fields = self.app_service.list_fields()
		self._parse_fields_change(next(self._list_fields))

		def _parse_script_response(self, response):
		self._parse_fields_change(response.changes)

		def __init__(self, host: str = None, port: int = None, never_timeout: bool = True, session_type: str = 'python', sync_fields: int = NO_SYNC):
		""" Initialize a Session object that connects to the Deephaven server

Conversation

SuperTails commented Jun 1, 2022 • edited by jmao-denver Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SuperTails commented Jun 1, 2022

Uh oh!

jmao-denver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devinrsmith left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SuperTails commented Jun 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SuperTails commented Jun 3, 2022

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!

jmao-denver left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SuperTails commented Jun 1, 2022 •

edited by jmao-denver

Loading

github-actions Bot commented Jun 1, 2022 •

edited

Loading