Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ExperimentAxisQuery uses the thread pool from ContextBase #184

Merged
merged 8 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions python-spec/src/somacore/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from .query import AxisColumnNames
from .query import AxisQuery
from .query import ExperimentAxisQuery
from .types import ContextBase

try:
# This trips up mypy since it's a generated file:
Expand Down Expand Up @@ -59,4 +60,5 @@
"AxisColumnNames",
"AxisQuery",
"ExperimentAxisQuery",
"ContextBase",
)
15 changes: 11 additions & 4 deletions python-spec/src/somacore/query/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from .. import data
from .. import measurement
from .. import options
from ..types import ContextBase
from . import _fast_csr
from . import axis
from . import types
Expand Down Expand Up @@ -587,11 +588,13 @@ def _var_df(self) -> data.DataFrame:

@property
def _threadpool(self) -> futures.ThreadPoolExecutor:
"""Creates a thread pool just in time."""
"""
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return self.experiment.context._threadpool

if self._threadpool_ is None:
# TODO: the user should be able to set their own threadpool, a la asyncio's
# loop.set_default_executor(). This is important for managing the level of
# concurrency, etc.
self._threadpool_ = futures.ThreadPoolExecutor()
return self._threadpool_

Expand Down Expand Up @@ -797,6 +800,10 @@ def ms(self) -> Mapping[str, measurement.Measurement]:
def obs(self) -> data.DataFrame:
...

@property
def context(self) -> ContextBase:
...


class _HasObsVar(Protocol[_T_co]):
"""Something which has an ``obs`` and ``var`` field.
Expand Down
10 changes: 10 additions & 0 deletions python-spec/src/somacore/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"""

import sys
from concurrent import futures
from typing import TYPE_CHECKING, NoReturn, Optional, Sequence, Type, TypeVar

from typing_extensions import Protocol, TypeGuard
Expand Down Expand Up @@ -75,3 +76,12 @@ def is_slice_of(__obj: object, __typ: Type[_T]) -> TypeGuard[Slice[_T]]:
and (__obj.stop is None or isinstance(__obj.stop, __typ))
and (__obj.step is None or isinstance(__obj.step, __typ))
)


class ContextBase(Protocol):
"""A protocol for a context manager that can be used as a base class.

The only requirement for somacore is that it should contain a threadpool.
"""

_threadpool: futures.ThreadPoolExecutor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.