[python] Re-introduce tiledb_ctx for SOMATileDBContext
#3335
Conversation
nguyenv
changed the title
tiledb_ctx for SOMATileDBContext
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3335 +/- ##
==========================================
+ Coverage 85.66% 85.79% +0.13%
==========================================
Files 57 55 -2
Lines 6180 6175 -5
==========================================
+ Hits 5294 5298 +4
+ Misses 886 877 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Couple of issues:
- The PR is missing the
tiledb_ctxproperty that was present in the API. This is the first point mentioned in the bug report. Previously it was possible to do the following:
context = soma.SOMATileDBContext()
A = tiledb.open(uri, ctx=context.tiledb_ctx)
We did deprecate this property in 1.14.5, so removing it is fair game. I'm just unsure what decision @aaronwolen took - the bug report suggests that he wanted to add both back, but I may be misreading it.
Recommend checking with him to confirm removal.
- There are some minor style/pattern comments inline.
| tiledb_config: Dict[str, Union[str, float]] | None = None, | ||
| tiledb_ctx: "tiledb.Ctx" | None = None, |
There was a problem hiding this comment.
| tiledb_config: Dict[str, Union[str, float]] | None = None, | |
| tiledb_ctx: "tiledb.Ctx" | None = None, | |
| tiledb_config: Dict[str, Union[str, float]] | "tiledb.Ctx" | None = None, |
I would slightly prefer this. Admittedly passing a context to the tiledb_config doesn't sound very good, but it eliminates the error case of passing both a context and a config. You could take a leap and say that a context is part of the "configuration".
There was a problem hiding this comment.
I did considering having tiledb_config also take in a tiledb.Ctx but decided to revert back to how we previously had it as separate arguments. But I'm open to further discussion about only having a tiledb_config argument now.
nguyenv
force-pushed
the
viviannguyen/optionally-use-tiledb-ctx
branch
from
c484029
to
17ce213
Compare
| # it should be impossible to enter into this block because | ||
| # we already check that in the constructor | ||
| assert tiledb is not None | ||
| cfg = self._tiledb_ctx().config().dict() |
There was a problem hiding this comment.
I don't think _tiledb_ctx is a method, so I suspect this should drop the parens, e.g.,
cfg = self._tiledb_ctx.config().dict()
There was a problem hiding this comment.
ps. assuming my comment is correct, does this imply a missing test case? (or my comment is wrong!)
There was a problem hiding this comment.
You are right that there should not be parens.
The missing test case brings up a another issue though. We have removed all tiledb-py usage from the unit tests. But should we temporarily re-introduce it for testing with tiledb.Ctx()? We would xskip the test if tiledb is not installed locally. But then do we need to install tiledb for CI runs? (cc @johnkerl)
There was a problem hiding this comment.
yeah, tough one. I don't see any great solution - options?
The best I can come up with is:
- Code the tests to work both with and without the package installed:
- if installed, test that it works as expected
- if not installed, test that it generates the right error
- Have two CI's: one with it installed, one with it not installed
That at least mimics real-world conditions.
| @@ -19,16 +19,30 @@ | |||
| from .._types import OpenTimestamp | |||
| from .._util import ms_to_datetime, to_timestamp_ms | |||
|
|
|||
| if TYPE_CHECKING: | |||
There was a problem hiding this comment.
We should not need this as all the type signatures are quoted.
Also, given that tiledb is no longer a dependency, shouldn't this fail in CI (I wonder why it did not fail?)
There was a problem hiding this comment.
It actually fails CI if this block is not included.
apis/python/src/tiledbsoma/options/_soma_tiledb_context.py:81: in __init__
tiledb_ctx: "tiledb.Ctx" | None = None,
E AttributeError: 'NoneType' object has no attribute 'Ctx'
There was a problem hiding this comment.
I had assumed if the modules included in TYPE_CHECKING are not found, then it treats the quoted types as string literals instead of types.
There was a problem hiding this comment.
It treats them as string literals for sure, but I don't think it has anything to do with whether the module is found. TYPE_CHECKING is a constant that mypy sets to true, and is false during actual "use" of the code.
Easiest solution is to probably just do: from tiledb import Ctx and use "Ctx" rather than "tiledb.Ctx".
Sorry this is such a PITA!
There was a problem hiding this comment.
This ended up being more complicated than expected, but I think I have something that works now.
Issue and/or context:
[sc-58711]
Changes:
tiledb_ctxargumentNotes for Reviewer:
We are not reintroducing tiledb-py as a dependency. The
tiledb_ctxis only usable when tiledb-py is installed.