ExperimentAxisQuery uses the thread pool from ContextBase #184

ebezzi · 2024-01-10T00:19:44Z

Companion PR to: single-cell-data/TileDB-SOMA#2001

bkmartinjr · 2024-01-10T00:40:52Z

python-spec/src/somacore/types.py

+    The only requirement for somacore is that it should contain a threadpool.
+    """
+
+    _threadpool: futures.ThreadPoolExecutor


I'm a bit confused which values are required and which are optional (aka could be None).

As coded in query.py, it appears that all objects must have a context, but a context may optionally have a threadpool. I.e., _threadpool may equal to None. But this code declares the type as non-optional.

Suggest clarifying what is optional and what is required, and having the types and tests match.

Good call, I switched the context to be Optional (as it is required by the definition of Experiment). For the threadpool, it can be either. The current implementation forces the threadpool to be not None, but I think it makes sense to leave that Optional as well, in case any other implementation prefers not to provide a thread pool. Regardless, the ExperimentAxisQuery code has a fallback path that manages its own threadpool in case either the context or the pool is missing.

bkmartinjr · 2024-01-10T00:41:25Z

python-spec/src/somacore/query/query.py

+        """
+        Returns the threadpool provided by the experiment's context.
+        If not available, creates a thread pool just in time."""
+        if self.experiment.context._threadpool is not None:


the Experiment-ish protocol types _threadpool as a required value, i.e., won't be None. Which is correct?

See comment above - I put all of them as Optional.

bkmartinjr

One typing nit - otherwise looks good.

thetorpedodog · 2024-01-10T22:28:54Z

If we're going to do this, we should also change the places where it is accepted in parameters / returned from accessors to be this base context protocol.

I also feel like the threadpool member should be public (i.e., un-prefixed), since it is intended for semi-public consumption (within SOMA libraries, but still).

ebezzi · 2024-01-10T23:59:21Z

If we're going to do this, we should also change the places where it is accepted in parameters / returned from accessors to be this base context protocol.

I also feel like the threadpool member should be public (i.e., un-prefixed), since it is intended for semi-public consumption (within SOMA libraries, but still).

Good points. I implemented both. Let me know if you know other usages I missed, and if you want a different naming/location for ContextBase.

thetorpedodog

Thought about this a bit more and realized some annoying caveats (which exist for legitimate reasons) that would come from the original way I suggested doing this.

thetorpedodog · 2024-01-16T17:27:14Z

python-spec/src/somacore/base.py

@@ -10,6 +10,7 @@
 from typing_extensions import LiteralString, Self

 from . import options
+from .types import ContextBase


minor nit: in somacore import modules, not things inside modules, so from . import types and use types.ContextBase below

thetorpedodog · 2024-01-16T17:39:38Z

python-spec/src/somacore/base.py

@@ -24,7 +25,7 @@ def open(
        uri: str,
        mode: options.OpenMode = "r",
        *,
-        context: Optional[Any] = None,
+        context: Optional[ContextBase] = None,


I was thinking about this a little further and it turns out there is a deeper wrinkle to this: we wouldn’t be able to override

class SOMAObject(...): @classmethod def open(..., context: Optional[ContextBase] = None) -> Self: ...

with

class ImplObject(somacore.SOMAObject): @classmethod def open(..., context: Optional[ImplContext] = None) -> Self: ... # not allowed, because `ImplContext` is a narrowing of `ContextBase`, # which would violate the Liskov principle: # https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

We could do a bunch of chicanery with another entry in the list of generics, so it would look something like:

_CtxT = TypeVar("_CtxT", bound=ContextBase) class SOMAObject(Generic[_CtxT]): def open(..., context: Optional[_CtxT] = None) -> Self: ... # elsewhere: class ImplObject(somacore.SOMAObject[ImplContext]): def open(..., context: Optional[ImplContext] = None) -> Self: ...

but that would also require adding a _CtxT to every SOMA object, which would be particularly unwieldy on the already highly-genericised Measurement type:

SOMA/python-spec/src/somacore/measurement.py

Lines 26 to 29 in 61f6cc9

class Measurement(

collection.BaseCollection[_RootSO],

Generic[_DF, _NDColl, _DenseNDColl, _SparseNDColl, _RootSO],

):

so instead, I think the easier thing to do is to opt out of type-checking for this parameter when being passed in, which, while imperfect, will allow it to be overridden with type information in the implementation. So the end result looks like:

class SOMAObject(metaclass=abc.ABCMeta): @classmethod def open(..., context: Optional[Any] = None) -> Self: ... # Ignore type-checking the specific type allowed in `open` @property def context(self) -> Optional[types.ContextBase]: ... # Returning a subclass is OK, though.

Then, at the implementation, we can say:

class ImplObject(SOMAObject): @classmethod def open(..., context: Optional[ImplContext] = None) -> Self: ... # Allowed since the superclass `context` parameter type is unchecked. @property def context(self) -> Optional[ImplContext]: ... # Allowed since you can narrow returned types.

thetorpedodog · 2024-01-16T17:40:01Z

python-spec/src/somacore/ephemeral/collections.py

+    def exists(
+        cls, uri: str, *, context: Optional[ContextBase] = None
+    ) -> Literal[False]:


In light of the above, this can be left as context: Any = None (i.e. no changes to this file at all).

thetorpedodog · 2024-01-16T17:42:46Z

python-spec/src/somacore/query/query.py

+            self.experiment.context is not None
+            and self.experiment.context.threadpool is not None


nit: since context and context.threadpool will be nonzero if they are set at all, this can be

context = self.experiment.context if context and context.threadpool: return context.threadpool

without the need for is not None.

thetorpedodog · 2024-01-16T17:44:27Z

python-spec/src/somacore/types.py

+    If a threadpool is specified as part of the context, it will be used by the
+    implementer. Otherwise, the implementer will use its own threadpool.


"it will be used by experiment queries", maybe? the implementer would be the one providing it

thetorpedodog

Looks good! Thanks for the bump.

bkmartinjr

LGTM

ContextBase

64f5043

ebezzi requested review from bkmartinjr and thetorpedodog January 10, 2024 00:19

ebezzi mentioned this pull request Jan 10, 2024

[python] Add default thread pool in SOMATileDBContext, and use somacore 1.0.8 single-cell-data/TileDB-SOMA#2001

Merged

bkmartinjr reviewed Jan 10, 2024

View reviewed changes

bkmartinjr requested changes Jan 10, 2024

View reviewed changes

Types -> optional

35db360

ebezzi requested a review from bkmartinjr January 10, 2024 21:48

Clarify docstring

dd49428

Add ContextBase to other types

74e750e

thetorpedodog reviewed Jan 16, 2024

View reviewed changes

ebezzi added 4 commits January 22, 2024 10:42

switch some typing to Any; more PR comments

4d40250

More changes

56ec49a

Fix

ed19b02

Formatting

82782c0

thetorpedodog approved these changes Jan 30, 2024

View reviewed changes

bkmartinjr approved these changes Jan 30, 2024

View reviewed changes

ebezzi merged commit 9b282eb into main Feb 12, 2024
6 checks passed

ebezzi deleted the ebezzi/threadpool-context branch February 12, 2024 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExperimentAxisQuery uses the thread pool from ContextBase #184

ExperimentAxisQuery uses the thread pool from ContextBase #184

ebezzi commented Jan 10, 2024

bkmartinjr Jan 10, 2024

ebezzi Jan 10, 2024

bkmartinjr Jan 10, 2024

ebezzi Jan 10, 2024

bkmartinjr left a comment

thetorpedodog commented Jan 10, 2024

ebezzi commented Jan 10, 2024

thetorpedodog left a comment

thetorpedodog Jan 16, 2024

thetorpedodog Jan 16, 2024

thetorpedodog Jan 16, 2024

thetorpedodog Jan 16, 2024

thetorpedodog Jan 16, 2024

thetorpedodog left a comment

bkmartinjr left a comment

	class Measurement(
	collection.BaseCollection[_RootSO],
	Generic[_DF, _NDColl, _DenseNDColl, _SparseNDColl, _RootSO],
	):

		self.experiment.context is not None
		and self.experiment.context.threadpool is not None

		If a threadpool is specified as part of the context, it will be used by the
		implementer. Otherwise, the implementer will use its own threadpool.

ExperimentAxisQuery uses the thread pool from ContextBase #184

ExperimentAxisQuery uses the thread pool from ContextBase #184

Conversation

ebezzi commented Jan 10, 2024

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

bkmartinjr left a comment

Choose a reason for hiding this comment

thetorpedodog commented Jan 10, 2024

ebezzi commented Jan 10, 2024

thetorpedodog left a 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

thetorpedodog left a comment

Choose a reason for hiding this comment

bkmartinjr left a comment

Choose a reason for hiding this comment