Enhance Python LivenessScope API #4497

niloc132 · 2023-09-14T20:43:27Z

The existing Python APIs for Liveness were difficult to use correctly in cases more complex than a single script - this brings several parts of the Java LivenessScope type to Python to enable more advanced use cases. Rather than introduce a LivenessScopeStack, this adds an open() method to LivenessScope itself, and through deprecation we're encouraging its use over the liveness_scope() factory method being directly passed to a with block. Likewise, close() is deprecated in favor of release(), which has different semantics, and matches the name used in Java.

Also makes TableListenerHandle a JObjectWrapper so it can be more easily unwrapped, can't accidentally be managed as a LivePyObjectWrapper in Java.

Fixes #4471

niloc132 · 2023-09-14T20:44:01Z

As part of this PR, I'll add a how-to to our docs, providing some better introduction to using liveness scopes.

niloc132 · 2023-09-14T20:44:37Z

py/server/deephaven/liveness_scope.py

+        return True
+    if isinstance(referent, JObjectWrapper):
+        return is_liveness_referent(referent.j_object)
+    return False


 def liveness_scope() -> LivenessScope:


@jmao-denver would it make sense to take this away (or deprecate it) and just encourage the LivenessScope constructor instead?

I was going to ask something similar. How do we minimize the syntax complexity and length while still feeling Pythonic? Maybe we want to keep this method and have it take the open args. Then users could just do something like:

with liveness_scope(True); ...

Dropping this in favor of just a regular ctor call, updating class docs

py/server/deephaven/liveness_scope.py

chipkent · 2023-09-19T17:33:23Z

py/server/tests/testbase.py

@@ -35,9 +35,11 @@ def tearDownClass(cls) -> None:

    def setUp(self) -> None:
        self._liveness_scope = liveness_scope()


self._liveness_scope does not appear to be used. Can it be removed?

Yes, I missed that.

chipkent · 2023-09-19T17:37:33Z

py/server/deephaven/liveness_scope.py

@@ -27,44 +50,143 @@ def __init__(self, j_scope: jpy.JType):
        self.j_scope = j_scope

    def __enter__(self):
+        warn('Instead of passing liveness_scope() to with, call open() on it first',
+             DeprecationWarning, stacklevel=2)


I don't see where DeprecationWarning is being imported.

Welcome to python I guess? its already imported by default for some reason: https://docs.python.org/3/library/exceptions.html?highlight=deprecationwarning#DeprecationWarning

$ python Python 3.10.11 (main, May 22 2023, 14:37:10) [GCC 13.1.1 20230429] on linux Type "help", "copyright", "credits" or "license" for more information. >>> print(DeprecationWarning) <class 'DeprecationWarning'>

chipkent · 2023-09-19T17:39:59Z

py/server/deephaven/liveness_scope.py

+        warn('This function is deprecated, prefer release(). Use cases that rely on this are likely to now fail.',
+             DeprecationWarning, stacklevel=2)


If these use cases are likely to fail, should we just hard remove these warning cases?

The advantage (I leave it to you/Jianfeng if this is a good one or not) that the deprecation warning will tell you what to fix, but just removing the method will just break and let you sort it out yourself.

py/server/deephaven/liveness_scope.py

chipkent · 2023-09-19T17:49:18Z

py/server/deephaven/liveness_scope.py

+    Returns True if the provided object is a LivenessReferent, and so can be managed by a LivenessScope.
+    Args:


Suggested change

Returns True if the provided object is a LivenessReferent, and so can be managed by a LivenessScope.

Args:

Returns True if the provided object is a LivenessReferent, and so can be managed by a LivenessScope.

Args:

chipkent · 2023-09-19T17:49:30Z

py/server/deephaven/liveness_scope.py

+    """
+    Returns True if the provided object is a LivenessReferent, and so can be managed by a LivenessScope.
+    Args:
+        referent: the object that maybe a LivenessReferent


missing type info

chipkent · 2023-09-19T17:50:51Z

py/server/deephaven/liveness_scope.py

+        return True
+    if isinstance(referent, JObjectWrapper):
+        return is_liveness_referent(referent.j_object)
+    return False


 def liveness_scope() -> LivenessScope:


I was going to ask something similar. How do we minimize the syntax complexity and length while still feeling Pythonic? Maybe we want to keep this method and have it take the open args. Then users could just do something like:

with liveness_scope(True); ...

chipkent · 2023-09-19T18:10:54Z

py/server/deephaven/liveness_scope.py

+        self.release()
+
+    @contextlib.contextmanager
+    def open(self, release_after_block: bool = True) -> "LivenessScope":


Reading this, I have some concerns. It seems like both

with LivenessScope(): ...

and

with LivenessScope().open(): ...

work, since open returns a LivenessScope, but only the second releases properly. I'm concerned that this design may create a construct that doesn't force the user to always do the right / smart thing.

At the same time, the syntax feels more awkward.

Both "work" (after s/LivenessScope/livenessScope/), in that the first one will warn "you're doing it wrong, this will not work correctly", thus "strongly encouraging" the user to call open(). We can outright remove this right away and do away with deprecation warnings, or wait a version or two for anyone who happens to be using this and needs to transition away.

The new version is strictly more powerful, as it doesn't conflate creating/releasing a scope with adding/removing it on the stack. We can't really have it both ways... The other option (discussed with Ryan) is to make open() an optional step so that you can call it either way, but this makes the Java and Python usage different/confusing going back and forth.

One other option is that we deprecate liveness_scope() itself, and make the LivenessScope() constructor usable, so we can leave liveness_scope() intact.

chipkent · 2023-09-19T18:12:48Z

py/server/tests/testbase.py

@@ -35,9 +35,11 @@ def tearDownClass(cls) -> None:

    def setUp(self) -> None:
        self._liveness_scope = liveness_scope()
+        self.opened_scope = liveness_scope().open()


This syntax feels worse than what we had before. Can it be improved?

What we had before was too simple, and prevented an important use case: Keeping a LivenessScope around in order to fully control the life cycle of the objects created under it, and/or re-using the same LivenessScope on multiple calls. That is, the old API was simply not fit for purpose.

This particular case is bad, because apparently the unittest "fixtures" can't be wrapped in contextmanagers or a with expr, etc - you can only explicitly open something, then close it later. That isn't a pattern we want to encourage here, since you might screw up and not close it. Apparently pytest does this better, closer to how junit solves this.

The other changes made in this patch's tests are more representative of how we expect this to work.

rcaudy

.

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>

niloc132

Summary of call with @chipkent and @jmao-denver:

Two use cases to support, named "A2" and "B2" on the call:

Simplest example, where a scope is created+opened for a with block, and closed+released at the end:

with liveness_scope(): # optional "as scope"
    # do things here that will be managed by the scope
    ...

This is the existing factory method, and the use case that it was intended for. The return value of that method is a LivenessScope, and can only be used to manage(...)/unmanage(...) or preserve(...) during that block.

Richer case, where the lifetime of the scope is longer than the time in which it was at the top of the liveness stack":

scope = LivenessScope()
with scope.open():
    # do things here that will be managed by the scope
    ...

# later at some point release the scope (can be re-opened in the meantime)
scope.release()

Instead of the factory method, this uses a constructor and an open() instance method. Scopes can be opened more than once, and must then be released when the user is finished with them.

The liveness_scope() method will return a specific type that only supports manage/unmanage/preserve, whereas the LivenessScope() constructor will also support the open() and release() methods.

py/server/deephaven/liveness_scope.py

chipkent · 2023-10-06T15:33:21Z

py/server/tests/test_liveness.py

+        self.assertTrue(must_keep.j_table.tryRetainReference())
+        # drop extra reference
+        must_keep.j_table.dropReference()
+


The liveness_scope pydoc indicates that @liveness_scope can be used to control scope, but I don't see a test for this case.

added (and otherwise cleaned up tests a bit).

typehints were wrong for both contextmanager usages, i fixed them as well.

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>

deephaven-internal · 2023-10-06T20:39:17Z

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/3277
Reference: https://github.com/deephaven/deephaven.io/issues/3276

niloc132 added 5 commits September 14, 2023 14:46

Quick hacked up learning attempt

e143677

Draft that looks more like java's api

6276e80

Update docs, make wrapper usage more consistent

81eeae0

Tidy up for review

7b1ee54

Self-review before PR

0b252f2

niloc132 added DocumentationNeeded python-server-side plug-ins ReleaseNotesNeeded Release notes are needed labels Sep 14, 2023

niloc132 added this to the September 2023 milestone Sep 14, 2023

niloc132 requested review from chipkent, jmao-denver and rcaudy as code owners September 14, 2023 20:43

niloc132 commented Sep 14, 2023

View reviewed changes

py/server/deephaven/liveness_scope.py Outdated Show resolved Hide resolved

niloc132 and others added 2 commits September 18, 2023 09:37

Update py/server/deephaven/liveness_scope.py

13dd0df

Merge branch 'main' into 4471-liveness-and-listeners

939ae34

niloc132 commented Sep 18, 2023

View reviewed changes

py/server/deephaven/liveness_scope.py Outdated Show resolved Hide resolved

niloc132 commented Sep 18, 2023

View reviewed changes

py/server/deephaven/liveness_scope.py Show resolved Hide resolved

rcaudy reviewed Sep 18, 2023

View reviewed changes

py/server/deephaven/liveness_scope.py Outdated Show resolved Hide resolved

py/server/deephaven/liveness_scope.py Show resolved Hide resolved

py/server/deephaven/liveness_scope.py Show resolved Hide resolved

niloc132 added 2 commits September 18, 2023 18:42

Draft at fixing py api, tests

9721921

Cleanup docs

32dd881

chipkent reviewed Sep 19, 2023

View reviewed changes

rcaudy reviewed Sep 19, 2023

View reviewed changes

niloc132 and others added 4 commits September 20, 2023 10:23

Update py/server/deephaven/liveness_scope.py

3079dde

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>

Update py/server/deephaven/liveness_scope.py

7a94403

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>

Enhance docs

d0e8c83

Improve tests

5a800b1

niloc132 commented Sep 26, 2023

View reviewed changes

niloc132 added 2 commits September 28, 2023 08:24

Merge branch 'main' into 4471-liveness-and-listeners

3dd3774

Updated api, docs to match new discussed patterns

fdf1ca4

niloc132 requested review from rcaudy and chipkent October 4, 2023 21:12

jmao-denver previously approved these changes Oct 5, 2023

View reviewed changes

chipkent reviewed Oct 6, 2023

View reviewed changes

Update py/server/deephaven/liveness_scope.py

9115098

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>

niloc132 dismissed jmao-denver’s stale review via 9115098 October 6, 2023 18:37

niloc132 and others added 3 commits October 6, 2023 13:37

Update py/server/deephaven/liveness_scope.py

f1c90ef

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>

Update py/server/deephaven/liveness_scope.py

b8c890a

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>

Added a test, corrected contextmanager typehints

7e456b7

niloc132 requested review from chipkent and jmao-denver October 6, 2023 18:49

correct indentation

5ddb965

niloc132 force-pushed the 4471-liveness-and-listeners branch from b7c21e1 to 5ddb965 Compare October 6, 2023 19:19

chipkent approved these changes Oct 6, 2023

View reviewed changes

niloc132 merged commit 03da796 into deephaven:main Oct 6, 2023

github-actions bot locked and limited conversation to collaborators Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Python LivenessScope API #4497

Enhance Python LivenessScope API #4497

niloc132 commented Sep 14, 2023

niloc132 commented Sep 14, 2023

niloc132 Sep 14, 2023

chipkent Sep 19, 2023

niloc132 Sep 26, 2023

chipkent Sep 19, 2023

niloc132 Sep 20, 2023

chipkent Sep 19, 2023

niloc132 Sep 20, 2023

chipkent Sep 19, 2023

niloc132 Sep 20, 2023

chipkent Sep 19, 2023

chipkent Sep 19, 2023

chipkent Sep 19, 2023

chipkent Sep 19, 2023

niloc132 Sep 20, 2023

chipkent Sep 19, 2023

rcaudy Sep 19, 2023

niloc132 Sep 20, 2023

rcaudy left a comment

niloc132 left a comment

chipkent Oct 6, 2023

niloc132 Oct 6, 2023

deephaven-internal commented Oct 6, 2023

		@@ -35,9 +35,11 @@ def tearDownClass(cls) -> None:

		def setUp(self) -> None:
		self._liveness_scope = liveness_scope()

		warn('This function is deprecated, prefer release(). Use cases that rely on this are likely to now fail.',
		DeprecationWarning, stacklevel=2)

		Returns True if the provided object is a LivenessReferent, and so can be managed by a LivenessScope.
		Args:

Enhance Python LivenessScope API #4497

Enhance Python LivenessScope API #4497

Conversation

niloc132 commented Sep 14, 2023

niloc132 commented Sep 14, 2023

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

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

rcaudy left a comment

Choose a reason for hiding this comment

niloc132 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deephaven-internal commented Oct 6, 2023