listdir in HttpStore and use consolidated metadata #82

dblodgett-usgs · 2024-05-21T19:56:58Z

As discussed in #79 -- this PR includes a rudimentary listdir for HttpStore that accesses and parses consolidated metadata.

I've introduced vcr for testing and memoise to avoid repeated requests to http resources.

This PR now also includes use of consolidated metadata for httpstore and other reads. Testing could be a bit more complete, but I feel pretty good about it. Performance is WAY WAY better when trying to access all the metadata for a store.

Happy to refactor or rework this if the pattern I put in isn't in line with your vision here @keller-mark -- R6 is totally new to me so this is a bit hacky -- hopefully I am seeing the forest for the trees!

…ration

Merge branch 'http' # Conflicts: # R/attributes.R # R/stores.R # R/zarr-array.R # R/zarr-group.R # man/Attributes.Rd # man/Store.Rd # man/ZarrArray.Rd # man/ZarrGroup.Rd # man/as.array.ZarrArray.Rd # man/sub-.ZarrArray.Rd # man/subset-.ZarrArray.Rd

dblodgett-usgs · 2024-05-23T13:50:37Z

.github/workflows/deploy.yml

@@ -39,7 +39,7 @@ jobs:
        uses: actions/cache@v2
        with:
          path: ${{ env.R_LIBS_USER }}/*
-          key: ${{ steps.get-version.outputs.os-version }}-${{ steps.get-version.outputs.r-version }}-${{ env.cache-version }}-deps
+          key: ${{ hashFiles('DESCRIPTION') }}-${{ steps.get-version.outputs.os-version }}-${{ steps.get-version.outputs.r-version }}-${{ env.cache-version }}-deps


this ensures the cache gets cleared when DESCRIPTION changes

dblodgett-usgs · 2024-05-23T13:53:21Z

R/creation.R

    if(is_na(mode)) {
        mode <- "a"
+        if(inherits(store, "HttpStore"))
+          mode <- "r"


changes here get zarr_open working properly with HttpStore

dblodgett-usgs · 2024-05-23T13:54:02Z

R/normalize.R

@@ -245,6 +245,9 @@ normalize_store_arg <- function(store, storage_options=NA, mode=NA) {
  }

  if(is.character(store)) {
+    if(grepl("^https?://", store)) {
+      return(HttpStore$new(store))
+    }


needed to explicitly check for http / https to get HttpStores to open with convenience functions

dblodgett-usgs · 2024-05-23T13:54:51Z

R/stores.R

+    #' Get consolidated metadata if it exists.
+    get_consolidated_metadata = function() {
+      return(private$zmetadata)
+    }


This just returns NULL unless the initialize function sets zmetadata to something.

dblodgett-usgs · 2024-05-23T13:55:10Z

R/stores.R

-      } else {
-        key <- item
-      }
+      key <- item_to_key(item)


this was used in a couple places so I move it to utils

dblodgett-usgs · 2024-05-23T13:56:13Z

R/stores.R

-      if(substr(url, length(url), length(url)) == "/") {
-        private$url <- substr(url, 1, length(url)-1)
+      if(substr(url, nchar(url), nchar(url)) == "/") {
+        private$url <- substr(url, 1, nchar(url)-1)


This just stuck out as obviously wrong

Thanks for catching this

dblodgett-usgs · 2024-05-23T13:56:40Z

R/stores.R

@@ -395,6 +418,8 @@ HttpStore <- R6::R6Class("HttpStore",
        opts = private$options,
        headers = private$headers
      )
+
+      private$zmetadata <- private$get_zmetadata()


replaces NULL with .zmetadata if it can be retrieved.

dblodgett-usgs · 2024-05-23T13:58:11Z

R/stores.R

+    #' Set cache time of http requests.
+    #' @param seconds number of seconds until cache is invalid -- 0 for no cache
+    set_cache_time_seconds = function(seconds) {
+      private$cache_time_seconds <- seconds


Should we support environment variables / setting / clearing with some global action? It would be nice to be able to manually modify cache behavior from way up the calling stack where you don't have access to modify the store.

I like this idea - it could be left as a follow-up issue to complete or done here, either way is fine with me

I'm tight on time today -- I'll open an issue to follow up on it.

dblodgett-usgs · 2024-05-23T13:59:07Z

R/zarr-array.R

+        meta_bytes <- private$store$get_item(mkey)
+        meta <- private$store$metadata_class$decode_array_metadata(meta_bytes)
+      }
+


Put this here since this is where a list is retrieved and the zmeta cache is already in that form.

dblodgett-usgs · 2024-05-23T13:59:53Z

R/zarr-group.R

+        private$meta <- private$store$metadata_class$decode_group_metadata(meta_bytes)
+
+      }
+


this is mostly reindentation -- just trying to pull the info from zmeta.

dblodgett-usgs · 2024-05-23T15:27:30Z

tests/testthat/test-http-store.R

+
+  test_that("http listdir and zmeta", {
+
+    url<- "https://raw.githubusercontent.com/DOI-USGS/rnz/main/inst/extdata/bcsd.zarr"


Once we merge this PR, this url can switch to this repository.

keller-mark · 2024-05-23T15:46:11Z

R/stores.R

+      key <- item_to_key(item)
+
+      # per-session http get cache
+      mem_get <- memoise::memoise(\(client, path) client$get(path), 


Will this get re-memoized on each call to make_request()? Can we create mem_get in the constructor, and then store it as a variable on the instance?

That's a good question -- probably. I'll make that change quick.

keller-mark · 2024-05-23T15:46:26Z

R/stores.R

-      if(substr(url, length(url), length(url)) == "/") {
-        private$url <- substr(url, 1, length(url)-1)
+      if(substr(url, nchar(url), nchar(url)) == "/") {
+        private$url <- substr(url, 1, nchar(url)-1)


Thanks for catching this

keller-mark · 2024-05-23T15:50:34Z

R/stores.R

+    #' Set cache time of http requests.
+    #' @param seconds number of seconds until cache is invalid -- 0 for no cache
+    set_cache_time_seconds = function(seconds) {
+      private$cache_time_seconds <- seconds


I like this idea - it could be left as a follow-up issue to complete or done here, either way is fine with me

dblodgett-usgs added 12 commits May 8, 2024 15:07

show how to listdir to find arrays in a store for keller-mark#75

92d6aed

show how to get attributes fixes keller-mark#75

170c5ec

pretty terminal output in vignette

e331db4

start documentation clean up

ef6697f

fixes keller-mark#60

247d8e5

add vcr and memoise for httpstore listdir (keller-mark#79)

1093637

move memoise into store object and provide getter/setter for cache du…

29d8787

…ration

Merge branch 'main' of https://github.com/keller-mark/pizzarr

03d4436

merge conflicts

f5a4476

Merge branch 'http' # Conflicts: # R/attributes.R # R/stores.R # R/zarr-array.R # R/zarr-group.R # man/Attributes.Rd # man/Store.Rd # man/ZarrArray.Rd # man/ZarrGroup.Rd # man/as.array.ZarrArray.Rd # man/sub-.ZarrArray.Rd # man/subset-.ZarrArray.Rd

test http cache time

9b9b557

zarr_open with http store

25cb4a8

fix up caching for dependency changes in DESCRIPTION

58286b7

dblodgett-usgs had a problem deploying to github-pages May 22, 2024 14:43 — with GitHub Actions Failure

proposed pattern for caching consolidated zmetadata (keller-mark#77)

2fa35a1

dblodgett-usgs force-pushed the http branch from 44bb5d8 to 2fa35a1 Compare May 22, 2024 20:02

dblodgett-usgs had a problem deploying to github-pages May 22, 2024 20:07 — with GitHub Actions Failure

dblodgett-usgs added 6 commits May 22, 2024 22:51

complete use of consolidated metadata for gets (keller-mark#77)

43805d5

tests for consolidated metadata (keller-mark#77)

1c24e2d

add consolidated metadata test data

dd54b1d

test covergage for keller-mark#77

0515f8e

nchar not length for strings

559cef3

rebuild documentation

b5fb774

dblodgett-usgs changed the title ~~listdir in HttpStore~~ listdir in HttpStore and use consolidated metadata May 23, 2024

dblodgett-usgs commented May 23, 2024

View reviewed changes

dblodgett-usgs had a problem deploying to github-pages May 23, 2024 13:52 — with GitHub Actions Failure

dblodgett-usgs commented May 23, 2024

View reviewed changes

zmeta clean up

a072824

dblodgett-usgs had a problem deploying to github-pages May 23, 2024 14:22 — with GitHub Actions Failure

dblodgett-usgs commented May 23, 2024

View reviewed changes

keller-mark approved these changes May 23, 2024

View reviewed changes

move mem_get to a private method on the store object

6fd6178

dblodgett-usgs had a problem deploying to github-pages May 23, 2024 16:14 — with GitHub Actions Failure

keller-mark merged commit 6920e83 into keller-mark:main May 27, 2024
1 of 2 checks passed

dblodgett-usgs mentioned this pull request May 28, 2024

Read and use consolidated metadata when available #77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

listdir in HttpStore and use consolidated metadata #82

listdir in HttpStore and use consolidated metadata #82

dblodgett-usgs commented May 21, 2024 •

edited

Loading

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

keller-mark May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

keller-mark May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

dblodgett-usgs May 23, 2024

keller-mark May 23, 2024

dblodgett-usgs May 23, 2024

keller-mark May 23, 2024

keller-mark May 23, 2024

		private$meta <- private$store$metadata_class$decode_group_metadata(meta_bytes)

		}


		test_that("http listdir and zmeta", {

		url<- "https://raw.githubusercontent.com/DOI-USGS/rnz/main/inst/extdata/bcsd.zarr"

listdir in HttpStore and use consolidated metadata #82

listdir in HttpStore and use consolidated metadata #82

Conversation

dblodgett-usgs commented May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblodgett-usgs commented May 21, 2024 •

edited

Loading