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

[r] Use cached SOMA context rather than re-creating #2988

Merged
merged 2 commits into from
Sep 12, 2024
Merged

[r] Use cached SOMA context rather than re-creating #2988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb8ffe5
[r] Use cached SOMA context rather than re-creating
mojaveazure Sep 12, 2024
bb49e43
Update changelog
mojaveazure Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apis/r/DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Description: Interface for working with 'TileDB'-based Stack of Matrices,
like those commonly used for single cell data analysis. It is documented at
<https://github.com/single-cell-data>; a formal specification available is at
<https://github.com/single-cell-data/SOMA/blob/main/abstract_specification.md>.
Version: 1.13.99.7
Version: 1.13.99.8
Authors@R: c(
person(given = "Aaron", family = "Wolen",
role = c("cre", "aut"), email = "[email protected]",
Expand Down
1 change: 1 addition & 0 deletions apis/r/NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Simplify timestamp ranges; strengthen assumptions about `tiledb_timestamp`
* Use cached timestamps in `$write()` and `$create()`
* Fix bug in blockwise iteration
* Lay groundwork for cached SOMA contexts within objects rather than re-creating contexts

# tiledbsoma 1.13.0

Expand Down
19 changes: 11 additions & 8 deletions apis/r/R/TileDBGroup.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ TileDBGroup <- R6::R6Class(

spdl::debug("[TileDBGroup$create] Creating new {} at '{}' at {}",
self$class(), self$uri, self$tiledb_timestamp)

private$.soma_context <- soma_context() # FIXME via factory and paramater_config
c_group_create(self$uri, self$class(), private$.soma_context,
self$.tiledb_timestamp_range) ## FIXME: use to be added accessor

Expand Down Expand Up @@ -63,15 +61,23 @@ TileDBGroup <- R6::R6Class(
}
if (is.null(private$.group_open_timestamp)) {
spdl::debug("[TileDBGroup$open] Opening {} '{}' in {} mode", self$class(), self$uri, mode)
private$.tiledb_group <- c_group_open(self$uri, type = mode, ctx = soma_context())#private$.some_context)
private$.tiledb_group <- c_group_open(
uri = self$uri,
type = mode,
ctxxp = private$.soma_context
)
} else {
if (internal_use_only != "allowed_use") stopifnot("tiledb_timestamp not yet supported for WRITE mode" = mode == "READ")
spdl::debug("[TileDBGroup$open] Opening {} '{}' in {} mode at {} ptr null {}",
self$class(), self$uri, mode, private$.group_open_timestamp,
is.null(private$.soma_context))
## The Group API does not expose a timestamp setter so we have to go via the config
private$.tiledb_group <- c_group_open(self$uri, type = mode, ctx = soma_context(), #private$.soma_context,
self$.tiledb_timestamp_range)
private$.tiledb_group <- c_group_open(
uri = self$uri,
type = mode,
ctxxp = private$.soma_context,
timestamp = self$.tiledb_timestamp_range
)
}
private$update_member_cache()
private$update_metadata_cache()
Expand Down Expand Up @@ -322,9 +328,6 @@ TileDBGroup <- R6::R6Class(
# with a list that's empty or contains the group metadata.
.metadata_cache = NULL,

## soma_context
.soma_context = NULL,

# Instantiate a group member object.
# Responsible for calling the appropriate R6 class constructor.
construct_member = function(uri, type) {
Expand Down
10 changes: 10 additions & 0 deletions apis/r/R/TileDBObject.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ TileDBObject <- R6::R6Class(
private$.tiledbsoma_ctx <- tiledbsoma_ctx
private$.tiledb_ctx <- self$tiledbsoma_ctx$context()

# TODO: re-enable once new UX is worked out
# soma_context <- soma_context %||% soma_context()
Copy link
Member

Choose a reason for hiding this comment

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

I think we want this active to be able to test as we just did with different regions. So let's expand the signature, and label the accessor as temporary. It's not in the 'official' user facing factory method.

Copy link
Member

Choose a reason for hiding this comment

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

We may also want to add the 'existence proof test' in a test file, possibly commented out (and we don't call aws, usually) -- but for illustration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because soma_context() caches globally, my thought process was that we could use the global context for modifications without changing the signature

soma_context(config = c(vfs.s3.region = "us-west-2"))
grp <- TileDBGroup$new(uri, ...)

If you feel strongly otherwise, I can change this

Copy link
Member

Choose a reason for hiding this comment

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

Ah, fair, but that is kinda a hidden feature I will have to take out at some point?

But agreed, for now it can stay.

# stopifnot(
# "'soma_context' must be a pointer" = inherits(x = soma_context, what = 'externalptr')
# )
private$.soma_context <- soma_context() # FIXME via factory and paramater_config

if (!is.null(tiledb_timestamp)) {
stopifnot(
"'tiledb_timestamp' must be a single POSIXct datetime object" = inherits(tiledb_timestamp, "POSIXct") &&
Expand Down Expand Up @@ -186,6 +193,9 @@ TileDBObject <- R6::R6Class(
# checking if .mode is non-null.
.mode = NULL,

## soma_context
.soma_context = NULL,

# @description Contains TileDBURI object
tiledb_uri = NULL,

Expand Down
9 changes: 9 additions & 0 deletions apis/r/tests/testthat/test-TileDBGroup.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,12 @@ test_that("Metadata", {

group$close()
})

# Existence proof test via cached global context
# soma_context(config = c(vfs.s3.region = "us-west-2"))
# (grp <- TileDBGroup$new(
# uri = 's3://cellxgene-census-public-us-west-2/cell-census/2024-07-01/soma/',
# internal_use_only = 'allowed_use'
# ))
# grp$open(mode = 'READ', internal_use_only = 'allowed_use')
# grp$names()