[python] Support H5AD ingest directly from S3 in backed mode #1629
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thetorpedodog
approved these changes
Aug 22, 2023
There was a problem hiding this comment.
Looks good! Based on the changes you made, it seems like we should maybe update tiledb.VFS.open to also accept pathlib.Paths as well. Doing so would make the changes you just did unnecessary (and make the TileDB VFS API play better with the rest of the Python world).
One trivial nit that you are free to ignore.
| @@ -219,16 +219,16 @@ def test_resume_mode(adata, resume_mode_h5ad_file): | |||
| tempdir1 = tempfile.TemporaryDirectory() | |||
| output_path1 = tempdir1.name | |||
| tiledbsoma.io.from_h5ad( | |||
| output_path1, resume_mode_h5ad_file, "RNA", ingest_mode="write" | |||
| output_path1, resume_mode_h5ad_file.as_posix(), "RNA", ingest_mode="write" | |||
There was a problem hiding this comment.
I think str(resume_mode_h5ad_file) might technically be a better way to express this, since there is a theoretical world where tiledbsoma runs on non-POSIX machines. str(some_pathlib_path) turns it into the system’s default representation. (I’m pretty sure I have myself written .as_posix() elsewhere in tiledbsoma without thinking that part through, so it basically Does Not Matter.)
|
Thanks @thetorpedodog ! I think the follow-on for TileDB-Py (accepting Pathlib) is a good one, thanks! |
thetorpedodog
added a commit
to TileDB-Inc/TileDB-Py
that referenced
this pull request
Aug 22, 2023
When working on a change in tiledbsoma [1], we ran into somewhat mysterious error messages where things had worked before. This was because, unlike `open` (and other libraries which accept path-likes), `tiledb.VFS` only accepts `str` (and undocumented `bytes`) paths. Accepting path-like objects (like `pathlib.Path`s) lets us play better with other libraries and user code. [1]: single-cell-data/TileDB-SOMA#1629
thetorpedodog
added a commit
to TileDB-Inc/TileDB-Py
that referenced
this pull request
Aug 22, 2023
When working on a change in tiledbsoma [1], we ran into somewhat mysterious error messages where things had worked before. This was because, unlike `open` (and other libraries which accept path-likes), `tiledb.VFS` only accepts `str` (and undocumented `bytes`) paths. Accepting path-like objects (like `pathlib.Path`s) lets us play better with other libraries and user code. [1]: single-cell-data/TileDB-SOMA#1629
thetorpedodog
added a commit
to TileDB-Inc/TileDB-Py
that referenced
this pull request
Aug 22, 2023
When working on a change in tiledbsoma [1], we ran into somewhat mysterious error messages where things had worked before. This was because, unlike `open` (and other libraries which accept path-likes), `tiledb.VFS` only accepts `str` (and undocumented `bytes`) paths. Accepting path-like objects (like `pathlib.Path`s) lets us play better with other libraries and user code. [1]: single-cell-data/TileDB-SOMA#1629
thetorpedodog
added a commit
to TileDB-Inc/TileDB-Py
that referenced
this pull request
Aug 23, 2023
When working on a change in tiledbsoma [1], we ran into somewhat mysterious error messages where things had worked before. This was because, unlike `open` (and other libraries which accept path-likes), `tiledb.VFS` only accepts `str` (and undocumented `bytes`) paths. Accepting path-like objects (like `pathlib.Path`s) lets us play better with other libraries and user code. [1]: single-cell-data/TileDB-SOMA#1629
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue and/or context:
@thetorpedodog has a marvelous change which lets us ingest H5AD -- with memory-friendly "backed" mode -- directly from S3 without needing to make a temp copy to local. Let's use there here too.
Changes:
Notes for Reviewer:
I'm having a problem with type checking.