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.

Sign up for GitHub

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

Jump to bottom

✨ Add Artifact$open() method #117

Merged
merged 13 commits into from
Dec 2, 2024
Merged

✨ Add Artifact$open() method #117

merged 13 commits into from
Dec 2, 2024

Conversation

lazappi
Copy link
Collaborator

@lazappi lazappi commented Nov 26, 2024 •
edited
Loading

Related to: Fixes #114

Description

  • Add a Artifact$open() method to access TileDB-SOMA stores
  • Update check_requires() to handle extra repositories (i.e. R-Universe)
  • Update the "Get started" vignette to replace the {cellxgene.census} example with an example using Artifact$open()

Checklist

Before review

  • Update and regenerate man pages
  • Add/update tests
  • Add/update examples in vignettes
  • Pass CI checks

Before merge

  • Update architecture vignette
  • Update development vignette
  • Update features in README
  • Update CHANGELOG

@lazappi
Add Artifact$Open() method
cfc279b
@lazappi
Modify check_requires() to handle extra repos
ad1e016 
Also adjust alert types and add tests
@lazappi
Install {tiledbsoma} on CI
95a7f9a 
If it works...
@lazappi
Merge remote-tracking branch 'origin/main' into issue-114/add-artifac…
b8ca5a8 
…t-open

* origin/main:
  update roadmap (#112)
  Update `get-started` with resolutions of questions from demo (#113)
@lazappi
Only try to install {tiledbsoma} on Ubuntu GHA
7940721
@lazappi
Replace {cellxgene.census} example in Get Started
8a284fa 
Use Artifact$open() to access a store instead
@falexwolf falexwolf changed the title Add Artifact$Open() method Add Artifact$open() method Nov 26, 2024
@lazappi
Copy link
Collaborator Author

lazappi commented Nov 26, 2024

@rcannood This works but requires {tiledbsoma} which is only on R-Universe. I'm not sure how that would go with CRAN so at the moment I haven't included it in DESCRIPTION. Can you check if you are ok with this?

@lazappi
Copy link
Collaborator Author

lazappi commented Nov 26, 2024

@falexwolf Can you please check if you are ok with the new example in the vignette here https://github.com/laminlabs/laminr/pull/117/files#diff-0c3baf1323c3304811b8f4f0f91c300e9242052855269e47afdb5276a7dd6c18? I tried to replicate this Python example https://docs.lamin.ai/cellxgene#slice-a-concatenated-array.

@falexwolf falexwolf requested a review from Koncopd November 26, 2024 16:07
falexwolf
falexwolf approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@falexwolf falexwolf left a comment

Choose a reason for hiding this comment

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

This looks great to me on a high level. @Koncopd, can you skim through this for a cross-check with the Python implementation?

Koncopd
Koncopd reviewed Nov 26, 2024
View reviewed changes
R/Artifact.R
#' @return A [tiledbsoma::SOMACollection] or [tiledbsoma::SOMAExperiment]
#' object
open = function() {
is_tiledbsoma <- private$get_value("suffix") == ".tiledbsoma" ||
Copy link
Member

Choose a reason for hiding this comment

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

We also allow name=="soma" https://github.com/laminlabs/lamindb/blob/39e0f529a41d9cbc475e52fe3aa0b8095af21494/lamindb/core/storage/_backed_access.py#L84

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I'll have to see if we have access to that.

Copy link
Member

Choose a reason for hiding this comment

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

We also allow name=="soma"

What does this mean? And why do we allow this?

Copy link
Member

Choose a reason for hiding this comment

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

(This is not a blocker for merging this here)

Koncopd
Koncopd reviewed Nov 26, 2024
View reviewed changes
R/Artifact.R
extra_repos = "https://chanzuckerberg.r-universe.dev"
)

artifact_uri <- paste0(
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that it won't work for s3 paths i think. At least the correct region should be provided, we have additional setup here https://github.com/laminlabs/lamindb/blob/39e0f529a41d9cbc475e52fe3aa0b8095af21494/lamindb/core/storage/_tiledbsoma.py#L33 but not sure what is available in R.

Copy link
Member

Choose a reason for hiding this comment

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

Was this resolved at the bottom of this PR or not?

Koncopd
Koncopd reviewed Nov 26, 2024
View reviewed changes
vignettes/laminr.Rmd
```{r slice-tiledbsoma, eval = requireNamespace("tiledbsoma", quietly = TRUE)}
# Set some environment variables to avoid an issue with {tiledbsoma}
# https://github.com/chanzuckerberg/cellxgene-census/issues/1261
Sys.setenv(TILEDB_VFS_S3_REGION = "us-west-2")
Copy link
Member

Choose a reason for hiding this comment

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

aha, ok, the region is provided here, i see. But it is probably much better to provide via tiledb context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was just a hacky workaround because there is a bug in the {tiledbsoma} package. I'll look into the context stuff but I'm not sure if we get all the necessary information from the API.

Copy link
Member

@falexwolf falexwolf Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that work-around is OK for now given you clearly marked it in the comments!

Sergei can help find a cleaner solution over the next weeks & months; it's not so urgent yet that this is 100% polished.

I think this is good to merge!

@falexwolf falexwolf changed the title Add Artifact$open() method ✨ Add Artifact$open() method Dec 1, 2024
@rcannood
Copy link
Collaborator

rcannood commented Dec 2, 2024 •
edited
Loading

@rcannood This works but requires {tiledbsoma} which is only on R-Universe. I'm not sure how that would go with CRAN so at the moment I haven't included it in DESCRIPTION. Can you check if you are ok with this?

I'm expecting CRAN to not like this. I'll run a check to see what CRAN will flag by default

@rcannood rcannood merged commit 2df5993 into main Dec 2, 2024
7 checks passed
@rcannood rcannood deleted the issue-114/add-artifact-open branch December 2, 2024 11:33
@lazappi lazappi mentioned this pull request Dec 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@falexwolf falexwolf falexwolf approved these changes

@Koncopd Koncopd Koncopd left review comments

@rcannood rcannood rcannood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable artifact.open() for tiledbsoma backends
4 participants
@lazappi @rcannood @Koncopd @falexwolf