single-cell-data / TileDB-SOMA

Python and R SOMA APIs using TileDB’s cloud-native format. Ideal for single-cell data at any scale.
https://tiledbsoma.readthedocs.io
MIT License
87 stars 25 forks source link

[r,py,c++] Support dataframe write path #2704

Closed eddelbuettel closed 3 months ago

eddelbuettel commented 3 months ago

Issue and/or context:

This PR switches data frame write operations to using libtiledbsoma. It contains the branch viviannguyen/fix ordered dictionary flag as well.

Changes:

Enable schema creation and data.frame writes.

Notes for Reviewer:

~This PR is (for now) still flagged WIP and 'draft' as a pass or two for cleanup may be needed removing some comments and alike.~ However, as it passes all tests locally it is ready for CI.

The commit history is missing timestamps because of rebasing. The initial branch started a while ago, and Vivian's branch was merged another while back too. My predecessor branch is (for now) still up as is Vivian's branch merged here.

SC 44885

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.16%. Comparing base (6d6c90e) to head (f115d43). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2704 +/- ## ========================================== - Coverage 90.19% 90.16% -0.04% ========================================== Files 37 37 Lines 4019 4005 -14 ========================================== - Hits 3625 3611 -14 Misses 394 394 ``` | [Flag](https://app.codecov.io/gh/single-cell-data/TileDB-SOMA/pull/2704/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=single-cell-data) | Coverage Δ | | |---|---|---| | [python](https://app.codecov.io/gh/single-cell-data/TileDB-SOMA/pull/2704/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=single-cell-data) | `90.16% <100.00%> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=single-cell-data#carryforward-flags-in-the-pull-request-comment) to find out more. | [Components](https://app.codecov.io/gh/single-cell-data/TileDB-SOMA/pull/2704/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=single-cell-data) | Coverage Δ | | |---|---|---| | [python_api](https://app.codecov.io/gh/single-cell-data/TileDB-SOMA/pull/2704/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=single-cell-data) | `90.16% <100.00%> (-0.04%)` | :arrow_down: | | [libtiledbsoma](https://app.codecov.io/gh/single-cell-data/TileDB-SOMA/pull/2704/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=single-cell-data) | `∅ <ø> (∅)` | |
eddelbuettel commented 3 months ago

@nguyenv Thanks for the review! We could also split this off if your PR #2629 went in by itself I could rebase. I have no preference which way we do this but I do have a preference for this getting done sooner rather than later, if possible, as subsequent work of mine depends on both.

nguyenv commented 3 months ago

I am totally fine with leaving this is a single large PR. I think that would be easiest and fastest tbh. I just wanted to leave some context for my own changes for anyone else who is reviewing this.

eddelbuettel commented 3 months ago

@mojaveazure Give it anoother look, please -- your comment was spot and I addressed it in follow-up commit https://github.com/single-cell-data/TileDB-SOMA/pull/2704/commits/f115d43c770f19b449e1ca23f245f0673dae58c7.

mojaveazure commented 3 months ago

Thanks for adding in the error-on-exists! From an R standpoint, this looks good to me! I'll defer to others for Python and C++

eddelbuettel commented 3 months ago

Thanks for catching that. Much better with the explicit (earlier than in core) error.

github-actions[bot] commented 3 months ago

The backport to release-1.12 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-1.12 release-1.12
# Navigate to the new working tree
cd .worktrees/backport-release-1.12
# Create a new branch
git switch --create backport-2704-to-release-1.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 cca332611545ec734acf149436954b04ad095f37
# Push it to GitHub
git push --set-upstream origin backport-2704-to-release-1.12
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-1.12

Then, create a pull request where the base branch is release-1.12 and the compare/head branch is backport-2704-to-release-1.12.

johnkerl commented 3 months ago

Backport unneeded: error on my part: this was merged to main just before release-1.12 was branched off of main