(feat): custom reopen with read_elem_as_dask for remote h5ad#1665
(feat): custom reopen with read_elem_as_dask for remote h5ad#1665ilan-gold wants to merge 3 commits into
reopen with read_elem_as_dask for remote h5ad#1665Conversation
reopen with read_elem_as_daskreopen with read_elem_as_dask for remote h5ad
|
I should note that I could not figure out how to extract the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1665 +/- ##
==========================================
- Coverage 86.87% 84.40% -2.47%
==========================================
Files 39 39
Lines 6033 6033
==========================================
- Hits 5241 5092 -149
- Misses 792 941 +149
|
| def maybe_open_h5( | ||
| path_or_group: Path | ZarrGroup, elem_name: str | ||
| ) -> Generator[StorageType, None, None]: | ||
| ) -> Callable[[], Iterator[StorageType]]: |
There was a problem hiding this comment.
The previous type was correct, the new one isn’t.
The way decorators interact with typing is that you normally type the decorated function (e.g. if it contains yield, the return type is Generator). The decorator than transforms the function from whatever it is to whatever the decorator wants.
I.e.
@contextmanager
def foo(*args: Unpack[Args]) -> Generator[Ret, None, None]: ...is the same as
def _foo(*args: Unpack[Args]) -> Generator[Ret, None, None]: ...
foo: Callable[Args, AbstractContextManager[Ret]] = contextmanager(_foo)(I’m not 100% sure I got the “unpack” syntax right, but you know what I mean)
| *, | ||
| wrap: Callable[[ArrayStorageType], ArrayStorageType] | ||
| | Callable[[H5Group | ZarrGroup], _CSRDataset | _CSCDataset] = lambda g: g, | ||
| reopen: None | Callable[[], Iterator[StorageType]] = None, |
There was a problem hiding this comment.
so the idea is “reopen is a callable that can be transformed into a context manager using contextlib.contextmanager.
Why not just “reopen is a callable that returns an contextlib.AbstractContextManager[StorageType]?
|
Do you know what the difference is between what |
|
@ivirshup My understanding is that |
|
@ivirshup any thoughts on this for 0.12? |
Benchmark changes
Comparison: https://github.com/scverse/anndata/compare/ccfb6e32806626765725ea9d1cec6eeede3176e3..a1be0be92a843312c3e7e500031ca5dfdfd1c218 More details: https://github.com/scverse/anndata/pull/1665/checks?check_run_id=39778000357 |
|
As an aside here: even if we use |
Not sure about the typing on
reopen(withGenerator[...]as before, it was complaining, but a normalCallablewithreturnfelt wrong); however, the following seems to work:The question is whether or not we'd want to internalize this - my guess is "no." I think we should only promise support for core zarr/hdf5. The flip side then is how do we test this....good questions to grapple with!
dask