Skip to content

Initial code review requested #1

Open
Open
Initial code review requested#1
Assignees
adamhsparks

Description

@max578
max578
opened on Apr 11, 2026

Hi @adamhsparks,

The geefetch package has been pushed to AAGI-AUS as a private repo. This is the Python-free R interface for extracting spatio-temporal environmental covariates from Google Earth Engine, following the nert architecture.

Requesting your review of the initial codebase. Key areas:

  • Overall package structure and API design (dispatcher pattern mirroring nert)
  • Authentication flow (gargle-based, REST-first with optional rgee backend)
  • Handler registry and convenience aliases (read_modis_ndvi(), read_era5(), etc.)
  • Documentation and vignettes

The package is at early development stage. Looking forward to your feedback.

— Max

Activity

  1. max578
    assigned
    adamhsparks
    on Apr 11, 2026

  2. adamhsparks commented on Apr 17, 2026

    @adamhsparks
    adamhsparks
    on Apr 17, 2026
    Member

    I started working/reviewing on a new branch, https://github.com/AAGI-AUS/geefetch/tree/review/ahs.

    My first comment is that the R package checks won't pass, tests fail.

    ── Warning: read_gee routes to cache on repeated call ─────────────────────────────────────
! Failed to write to disk cache.
i Result is cached in memory only for this session.
i Error: [writeRaster] there are no cell values
Backtrace:
    ▆
 1. └─geefetch:::.cache_set("modis_ndvi", cache_key, fake_result) at T/R.nvim-283204f/Rsource-88861:13:3
 2.   └─base::tryCatch(...) at geefetch/R/cache.R:188:3
 3.     └─base (local) tryCatchList(expr, classes, parentenv, handlers)
 4.       └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 5.         └─value[[3L]](cond)
 6.           └─cli::cli_warn(...) at geefetch/R/cache.R:199:7
── Error: read_gee routes to cache on repeated call ───────────────────────────────────────
Error in `.rest_request("image:computePixels", body = body, max_tries = max_tries, initial_delay = initial_delay, raw = TRUE)`: GEE REST API error (HTTP 401).
x Request had invalid authentication credentials. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project.
i Your authentication token may have expired.
i Run `gee_auth()` to re-authenticate.
i Endpoint: "https://earthengine.googleapis.com/v1/projects/earthengine-legacy/image:computePixels"
Backtrace:
    ▆
 1. └─geefetch::read_gee(...) at T/R.nvim-283204f/Rsource-88861:16:3
 2.   └─geefetch:::.read_gee_modis_ndvi(dots, backend, max_tries, initial_delay) at geefetch/R/read_gee.R:102:3
 3.     └─geefetch:::.rest_extract_raster_expr(...) at geefetch/R/handlers.R:32:5
 4.       └─geefetch:::.rest_compute_pixels(...) at geefetch/R/handlers.R:219:3
 5.         └─geefetch:::.rest_request(...) at geefetch/R/backend_rest.R:470:3
 6.           └─cli::cli_abort(...) at geefetch/R/backend_rest.R:428:5
 7.             └─rlang::abort(...)
Error:
! Test failed with 1 failure and 0 successes.
Backtrace:
     ▆
  1. ├─nvimcom::Rnvim.source()
  2. │ └─base::source(...)
  3. │   ├─base::withVisible(eval(ei, envir))
  4. │   └─base::eval(ei, envir)
  5. │     └─base::eval(ei, envir)
  6. └─testthat::test_that(...) at T/R.nvim-283204f/Rsource-88861:1:1
  7.   └─testthat:::test_code(code, parent.frame())
  8.     ├─withr (local) `<fn>`()
  9.     └─reporter$end_test(context = reporter$.context, test = test)
 10.       └─cli::cli_abort(...)
 11.         └─rlang::abort(...)

    We should not be hitting the server for tests. As I understand it, this test hits the API for the data. We should be using mocking, either {httptest2} or {vcr} should be used to create fixtures to use in testing.

    This is a great resource, https://books.ropensci.org/http-testing/.

  3. adamhsparks commented on Apr 17, 2026

    @adamhsparks
    adamhsparks
    on Apr 17, 2026
    Member

    Depending on the size of the json files, {yyjsonr} is much faster at ingesting and creating json files than {jsonlite} is and pretty much just drops in if you replace the fn names.

  4. adamhsparks commented on Apr 17, 2026

    @adamhsparks
    adamhsparks
    on Apr 17, 2026
    Member

    There should be an option for users to turn on and off caching via options(). Many would prefer to use {targets} or {memoise} in their workflow.

  5. max578 commented on Apr 19, 2026

    @max578
    max578
    on Apr 19, 2026
    CollaboratorAuthor

    Thanks for the detailed review @adamhsparks — three substantive
    comments, addressed below. Status summary first, then per-comment
    detail.

    # Your comment (paraphrased) Status
    1 Tests shouldn't hit the live server — use httptest2 or vcr. ✅ Addressed
    2 yyjsonr is faster than jsonlite; drops in by fn-name. ⚠️ Deferred to performance PR
    3 options() switch to turn caching on/off for targets / memoise users. ✅ Addressed

    1. HTTP mocking (addressed)

    New test file tests/testthat/test-rest-httptest2.R — six tests
    covering 200 / 403 / 429 REST paths, GeoJSON round-trip through
    .rest_compute_features(), unauthenticated state, and a
    httptest2::without_internet() boundary assertion. httptest2 is now
    both declared and used. Test suite goes from 0 → 6 httptest2-backed
    tests, 0 → 4 cli snapshot tests.

    Also added tests/testthat/setup.R (scrubs GOOGLE_APPLICATION_CREDENTIALS,
    GEE_SERVICE_ACCOUNT_KEY, GARGLE_OAUTH_CACHE, isolates disk cache to
    a tempdir, pins LC_COLLATE=C / TZ=UTC) so no future test can leak
    auth state onto the host machine or into CI logs.

    One thing beyond your comment: a Layer-1 live-GEE integration suite —
    tests/testthat/test-live-gee.R (7 opt-in end-to-end tests, one per
    handler tier) + .github/workflows/live-gee-integration.yaml (weekly
    cron, fork-guarded, concurrency-grouped, key redacted on teardown).
    Skipped by default on CRAN / covr / absence of
    GEEFETCH_LIVE_TESTS=1, so CI stays green until a service-account
    secret is provisioned.

    2. yyjsonr (deferred, not dropped)

    Agreed — the JSON serialisation in R/backend_rest.R is hot-path
    enough to matter at scale, and the fn-name drop-in you describe
    makes it a low-risk swap. Deferred to a dedicated performance PR
    rather than bundled with this audit; flagged in audit_2026-04-19.md
    under "Archetype-6a specific validation" so it can't get lost. Happy
    to pick up as the next piece of work after the current two PRs land.

    3. options(geefetch.cache) switch (addressed)

    Every reader's cache argument now defaults to
    getOption("geefetch.cache", TRUE) — covers read_gee(),
    read_modis_ndvi(), read_modis_lst(), read_era5(), read_chirps(),
    read_srtm(), read_slga(), read_sentinel2(), read_landsat(),
    read_worldclim(), and collect_gee_data(). Belt-and-braces:
    .cache_get() and .cache_set() also early-return on the option, so
    any internal caller that bypasses the per-call argument still honours
    the opt-out.

    A targets / memoise user now just needs:

    options(geefetch.cache = FALSE)         # once, in _targets.R or .Rprofile
list(
  tar_target(ndvi, geefetch::read_modis_ndvi(
    date   = "2024-06-15",
    region = terra::ext(138, 140, -36, -34)
  ))                                    # targets caches the result instead
)

    Documented in ?geefetch under "Package options" (with the
    targets example), ?read_gee under "Caching", and NEWS.md.
    Two new tests in test-cache.R prove options(geefetch.cache = FALSE)
    produces zero cache files and NULL reads even when a file already
    exists on disk; the default TRUE path still round-trips.

    Where to find the work

    Full point-by-point change catalogue lives in two files on the audit
    branch: audit_2026-04-19.md (pre-fix Pass / Gap / N/A rubric scoring)
    and CHANGES_AUDIT_2026-04-19.md (every fix grouped A-K with
    before / after, file inventory, deferred items, and the 10-commit
    grouping).

    Happy to keep this issue open until the yyjsonr PR also lands if
    you'd prefer — or close after the two current PRs merge. Your call.

  6. adamhsparks commented on Jul 7, 2026

    @adamhsparks
    adamhsparks
    on Jul 7, 2026
    Member

    I've fixed the failing test.

  7. adamhsparks commented on Jul 7, 2026

    @adamhsparks
    adamhsparks
    on Jul 7, 2026
    Member

    My next comment is that the vignettes should be precompiled so that the examples are known to work.

    See: https://github.com/ropensci/nasapower/blob/main/vignettes/!precompile.R and https://github.com/ropensci/nasapower/blob/main/vignettes/nasapower.Rmd.orig for examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions