Merge branch 'develop' into fix/3882-sa-median-manifest-lookup

Author: infotroph

.github/workflows/docker-build-pr.yml

@@ -0,0 +1,76 @@
+name: Docker build (fork PRs)
+
+# A fast check for Docker build errors introduced by the current PR.
+# Implemented separately from the full-stack build in docker.yml because 
+# pull requests opened from forks get a read-only GITHUB_TOKEN and cannot push
+# to ghcr.io/pecanproject, so the normal Docker GHA workflow (docker.yml) skips
+# them (see the gate on its `rversion` job). This workflow gives those PRs a
+# real "does the image stack still build" signal without pushing anything.
+#
+# The images build on top of each other (depends -> base -> models -> sipnet).
+# The normal workflow runs each as a separate job and pulls the parent back from
+# ghcr, which needs a registry we can't push to from a fork. Here we build the
+# whole chain in a single job with plain `docker build`, so each image stays in
+# the runner's local image store and the next `FROM pecan/<parent>:latest`
+# resolves locally. No registry, no push, no secrets.
+#
+# Scope is the core linear chain only (depends -> base -> models -> sipnet). It
+# fits the default runner disk (~19 GB free; the resident stack is ~5-6 GB). The
+# other model binaries, baseplus (docs/executor/api) and extras images are not
+# built here to keep the job within disk and time budget.
+
+on:
+  pull_request:
+  workflow_dispatch:
+
+permissions:
+  contents: read
+
+jobs:
+  build-stack:
+    # Only fork PRs. Same-repo PRs are already covered by docker.yml, which can
+    # push pr-* tags to ghcr because they run with a writable token.
+    if: github.event.pull_request.head.repo.full_name != github.repository
+    runs-on: ubuntu-latest
+    env:
+      R_VERSION: "4.4"
+    steps:
+      - uses: actions/checkout@v6
+
+      - name: Disk space before build
+        run: df -h /
+
+      # NOTE: do not add docker/setup-buildx-action here. Plain `docker build`
+      # uses the default docker-engine builder, which resolves each
+      # `FROM pecan/<parent>:latest` from the local image store built by the
+      # previous step. The docker-container buildx driver would not see those
+      # local images and every FROM after depends would fail.
+      - name: Build depends -> base -> models -> sipnet (no push)
+        run: |
+          set -euxo pipefail
+
+          # depends: FROM rocker/tidyverse:${R_VERSION}
+          docker build -f docker/depends/Dockerfile \
+            --build-arg R_VERSION="${R_VERSION}" \
+            -t pecan/depends:latest \
+            docker/depends
+
+          # base: FROM pecan/depends:latest (resolved from the local store)
+          docker build -f docker/base/Dockerfile \
+            -t pecan/base:latest \
+            .
+
+          # models: FROM pecan/base:latest
+          docker build -f docker/models/Dockerfile \
+            -t pecan/models:latest \
+            docker/models
+
+          # sipnet: FROM pecan/models:latest
+          docker build -f models/sipnet/Dockerfile \
+            --build-arg MODEL_VERSION=git \
+            -t pecan/model-sipnet-git:latest \
+            models/sipnet
+
+      - name: Disk space after build
+        if: always()
+        run: df -h /

.github/workflows/docker.yml

@@ -42,6 +42,15 @@ jobs:
   # As an ugly workaround, we assign it to a job output instead.
   # ----------------------------------------------------------------------
   rversion:
+    # Every build job below needs this job, so gating it here skips the whole
+    # push-based Docker stack for pull requests opened from forks. Forks get a
+    # read-only GITHUB_TOKEN and cannot push to ghcr.io/pecanproject, so those
+    # jobs always failed at the push step. Fork PRs are instead built (without
+    # pushing) by docker-build-pr.yml. Non-fork events (push, merge_group,
+    # schedule, workflow_dispatch) and same-repo PRs still run the full stack.
+    if: >-
+      github.event_name != 'pull_request' ||
+      github.event.pull_request.head.repo.full_name == github.repository
     runs-on: ubuntu-latest
     steps:
       - id: mon

.github/workflows/integration-test.yml

@@ -5,21 +5,31 @@ on :
 
   schedule:
     # run Thursday 4:30 AM UTC
-  - cron: '30 4 * * 4'    
+  - cron: '30 4 * * 4'
 jobs:
   test:
     runs-on: ubuntu-latest
 
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
-    
-    container: 
+      PGHOST: postgres
+
+    services:
+      postgres:
+        image: mdillon/postgis:9.5
+        options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5
+
+    container:
       image: pecan/base:develop
 
     steps:
     - name: Checkout source code
       uses: actions/checkout@v4
 
+    # initialize database
+    - name: db setup
+      uses: docker://pecan/db:ci
+
     - name: Run tests
       run: |
         for FILE in modules/data.atmosphere/inst/integrationTests/*; do
@@ -29,3 +39,6 @@ jobs:
             Rscript "$FILE"
           fi
         done
+
+    - name: Run modular pipeline integration test
+      run: Rscript modules/uncertainty/inst/integrationTests/test.modular.chain.R

CHANGELOG.md

@@ -13,6 +13,7 @@ For more information about this file see also [Keep a Changelog](http://keepacha
     with units attached as an attribute.
 - New package `PEcAn.RothC` runs the RothC soil carbon model.
 - Added `inst/ilamb/` pipeline in PEcAn.benchmark to convert downscaled SDA reanalysis GeoTIFFs into ILAMB-compatible CF netCDF for carbon-cycle benchmarking (#4019).
+- Added multi-model benchmarking scripts to `inst/ilamb/` in PEcAn.benchmark: build the CMIP6 and TRENDY ensembles, score the PEcAn reanalysis and its individual SDA members against observational benchmarks over two evaluation windows, and summarize ensemble spread (#4038).
 - Added PEcAn.PEPRMT model, including a demo run with example data
 - Add `format_try_for_ma()` and `try_trait_mapping()` to `PEcAn.data.remote` to convert trait data from the external TRY database into the tabular format required by the PEcAn meta-analysis module (#3717).
 - Add function `qsub_sda()` for submitting SDA batch jobs by splitting a large number of sites into multiple small groups of sites (#3634).
@@ -30,11 +31,13 @@ For more information about this file see also [Keep a Changelog](http://keepacha
 - Added `PEcAn.data.land::event_parquet_to_json` for generating PEcAn `event.json` files from well-formatted event parquet files, with support for ensembles of events.
 
 ### Fixed
+- Docker GHA workflow no longer fails on pull requests opened from forks (#3618).
 - Removed unused `grid2netcdf()` from `PEcAn.data.remote` and fixed R CMD check reference notes for `download.LandTrendr.AGB()` (#2758).
 - Fixed broken pecanproject.github.io, pecan.gitbooks.io, and other outdated documentation links across book_source, tutorials, models, modules, web, and shiny files (#3710).
 - Added note to DEV-INTRO.md documenting Traefik workaround for Apple Silicon (ARM64) Macs: use `traefik:v2.11` with `platform: linux/arm64` to fix 404 errors (#3910)
 - Fixed `web/08-finished.php`: show database info instead of "Still running" when workflow folder doesn't exist locally (#3501).
 - `PEcAn.utils::transformstats()`: corrected the LSD-to-SE conversion. The previous implementation included an extra `sqrt(n)` factor, causing SE estimates derived from LSD to appear `sqrt(n)` times smaller than they should be, non-conservatively over-weighting those observations in meta-analysis. (#3998)
+- `segment_dataframe()` now returns an empty dataframe when date filtering removes all crop-cycle segments, instead of a single row with NA columns that caused downstream segment config errors (#4007).
 
 ### Changed
 - `PEcAn.uncertainty::get.parameter.samples()`: replaced the `save_to_disk` flag (from #3860) with an `outdir` argument (default `settings$outdir`) controlling whether `samples.Rdata` is written; `outdir = NULL` skips the save. Existing callers are unaffected (@omkarrr2533, #4016)

base/db/R/get_trait_data_pft.R

@@ -19,7 +19,10 @@
 #' @param modeltype character or NULL. Disambiguates PFTs that share a name
 #'   across model types (e.g. \code{"SIPNET"}, \code{"ED2"}).
 #' @param dbcon database connection from \code{\link[PEcAn.DB]{db.open}}.
-#' @param trait_names character vector of trait names to retrieve.
+#' @param trait_names character vector of trait names to retrieve. If
+#'   \code{NULL} (default), priors for every trait associated with the PFT
+#'   are returned and the trait list is derived from those priors. Supply a
+#'   vector to restrict the query to specific traits.
 #' @param constants named list from \code{pft$constants} in the settings.
 #'   Traits named here are excluded from the returned priors because their
 #'   values are fixed rather than sampled by the meta-analysis.
@@ -71,16 +74,19 @@
 get_trait_data_pft <- function(pft_name,
                                modeltype,
                                dbcon,
-                               trait_names,
+                               trait_names = NULL,
                                constants = list()) {
 
   # ---- Input validation (cheap checks before any DB call) ----
   if (!is.character(pft_name) || length(pft_name) != 1L) {
     PEcAn.logger::logger.severe("'pft_name' must be a single character string")
   }
-  if (!is.character(trait_names) || length(trait_names) == 0L) {
+  # trait_names is optional: NULL means "all priors for this PFT". If the
+  # caller does supply it, it must still be a non-empty character vector.
+  if (!is.null(trait_names) &&
+      (!is.character(trait_names) || length(trait_names) == 0L)) {
     PEcAn.logger::logger.severe(
-      "'trait_names' must be a non-empty character vector"
+      "'trait_names', when supplied, must be a non-empty character vector"
     )
   }
   if (!inherits(dbcon, "DBIConnection")) {
@@ -149,9 +155,13 @@ get_trait_data_pft <- function(pft_name,
 
   # ---- Query prior distributions ----
   # format() prevents integer64 from being silently coerced to double in SQL.
+  # NULL trait_names -> trstr = NULL (NOT vecpaste(NULL), which yields "" and
+  # would inject an empty `IN ()` clause). query.priors drops the trait filter
+  # when trstr is NULL and returns every prior for the PFT.
+  trstr <- if (is.null(trait_names)) NULL else PEcAn.utils::vecpaste(trait_names)
   prior_distns <- query.priors(
     pft   = format(pft_id, scientific = FALSE),
-    trstr = PEcAn.utils::vecpaste(trait_names),
+    trstr = trstr,
     con   = dbcon
   )
 

base/db/man/get_trait_data_pft.Rd

@@ -116,6 +116,31 @@ test_that("prior_distns is a data frame with the required columns", {
   expect_true(all(rownames(result$prior_distns) %in% std_traits))
 })
 
+test_that("trait_names = NULL returns priors for all of the PFT's traits", {
+  test_dbcon <- check_db_test()
+  withr::defer(PEcAn.DB::db.close(test_dbcon))
+
+  # trait_names omitted -> NULL -> query.priors returns every prior for the PFT
+  result_all <- get_trait_data_pft(
+    pft_name  = std_pft,
+    modeltype = std_modeltype,
+    dbcon     = test_dbcon
+  )
+
+  result_subset <- get_trait_data_pft(
+    pft_name    = std_pft,
+    modeltype   = std_modeltype,
+    dbcon       = test_dbcon,
+    trait_names = "SLA"
+  )
+
+  expect_s3_class(result_all$prior_distns, "data.frame")
+  # the unfiltered call must be a superset of any single-trait filter
+  expect_gte(nrow(result_all$prior_distns), nrow(result_subset$prior_distns))
+  expect_true(all(rownames(result_subset$prior_distns) %in%
+                    rownames(result_all$prior_distns)))
+})
+
 test_that("pft_info contains expected fields and posteriorid is NULL", {
   test_dbcon <- check_db_test()
   withr::defer(PEcAn.DB::db.close(test_dbcon))
@@ -303,24 +328,3 @@ test_that("errors when query_pfts returns multiple rows (multi-modeltype case)",
     "Multiple PFTs"
   )
 })
-
-test_that("errors when query_pfts returns zero rows", {
-  empty_record <- data.frame(
-    id       = integer(0),
-    pft_type = character(0),
-    name     = character(0)
-  )
-  mockery::stub(get_trait_data_pft, "query_pfts", empty_record)
-
-  fake_dbcon <- structure(list(), class = c("PostgreSQLConnection",
-                                            "DBIConnection"))
-  expect_error(
-    get_trait_data_pft(
-      pft_name    = "DoesNotExist",
-      modeltype   = "SIPNET",
-      dbcon       = fake_dbcon,
-      trait_names = "SLA"
-    ),
-    "PFTs were not found"
-  )
-})

base/db/tests/testthat/test-get_trait_data_pft.R

@@ -1,6 +1,5 @@
 #' Write model-specific run scripts and configuration files
 #'
-#' @md
 #' Generates run scripts and configuration files for all analyses (ensemble
 #' and/or sensitivity analysis) specified in the provided settings. Delegates
 #' the model-specific config writing to the appropriate `write.config.*`
@@ -43,6 +42,8 @@
 #'    appended. This forces use of only files within this workflow, to avoid
 #'    confusion.
 #'
+#' @md
+#'
 #' @param settings a PEcAn settings list
 #' @param ensemble.size number of ensemble runs
 #' @param input_design Input design data.frame coordinating input files across
@@ -53,6 +54,11 @@
 #'   ensemble and sensitivity analysis (e.g. `post.distns.Rdata`, or
 #'   `prior.distns.Rdata`).
 #' @param overwrite logical: Replace output files that already exist?
+#' @param samples Optional pre-computed parameter samples (a list with
+#'   `trait.samples`, `sa.samples`, `ensemble.samples`). When supplied, these
+#'   are used directly and `samples.Rdata` is not read from disk. When `NULL`
+#'   (default), the function falls back to loading `samples.Rdata` from
+#'   `settings$outdir`.
 #'
 #' @return The `settings` list (invisibly), updated with ensemble IDs for SA
 #'   and ensemble analysis (e.g. `settings$sensitivity.analysis$ensemble.id`,
@@ -63,7 +69,7 @@
 
 run.write.configs <- function(settings, ensemble.size, input_design, write = TRUE,
                               posterior.files = rep(NA, length(settings$pfts)),
-                              overwrite = TRUE) {
+                              overwrite = TRUE, samples = NULL) {
 
   # Validate that input_design matches ensemble.size for ensemble runs
   # Note: for SA, ensemble.size is not meaningful; SA design size is determined by
@@ -158,36 +164,43 @@ run.write.configs <- function(settings, ensemble.size, input_design, write = TRU
   scipen <- getOption("scipen")
   options(scipen = 12)
 
-  samples.file <- file.path(settings$outdir, "samples.Rdata")
-  if (file.exists(samples.file)) {
-    existing_data <- new.env()
-    load(samples.file, envir = existing_data) ## loads ensemble.samples, trait.samples, sa.samples, runs.samples, env.samples
-    trait.samples <- existing_data$trait.samples
-    sa.samples <- existing_data$sa.samples
-    
-    # build ensemble.samples only for ensemble runs
-    # SA runs use sa.samples directly (quantile-based), not ensemble.samples
-    if ("ensemble" %in% names(settings) && 
-        !is.null(input_design) && 
-        "param" %in% colnames(input_design)) {
-      trait_sample_indices <- input_design[["param"]]
-      ensemble.samples <- list()
-      for (pft in names(trait.samples)) {
-        pft_traits <- trait.samples[[pft]]
-        ensemble.samples[[pft]] <- as.data.frame(
-          lapply(
-            names(pft_traits),
-            function(trait) pft_traits[[trait]][trait_sample_indices]
-          )
-        )
-        names(ensemble.samples[[pft]]) <- names(pft_traits)
-      }
+  ## Resolve parameter samples: use the in-memory bundle when passed,
+  ## otherwise fall back to loading samples.Rdata from disk.
+  if (!is.null(samples)) {
+    existing_data <- samples
+  } else {
+    samples.file <- file.path(settings$outdir, "samples.Rdata")
+    if (file.exists(samples.file)) {
+      existing_data <- new.env()
+      load(samples.file, envir = existing_data) ## loads ensemble.samples, trait.samples, sa.samples, runs.samples, env.samples
     } else {
-      # use pre-generated samples
-      ensemble.samples <- existing_data$ensemble.samples
+      PEcAn.logger::logger.error(samples.file, "not found, this file is required by the run.write.configs function")
+    }
+  }
+
+  trait.samples <- existing_data$trait.samples
+  sa.samples <- existing_data$sa.samples
+
+  # build ensemble.samples only for ensemble runs
+  # SA runs use sa.samples directly (quantile-based), not ensemble.samples
+  if ("ensemble" %in% names(settings) &&
+      !is.null(input_design) &&
+      "param" %in% colnames(input_design)) {
+    trait_sample_indices <- input_design[["param"]]
+    ensemble.samples <- list()
+    for (pft in names(trait.samples)) {
+      pft_traits <- trait.samples[[pft]]
+      ensemble.samples[[pft]] <- as.data.frame(
+        lapply(
+          names(pft_traits),
+          function(trait) pft_traits[[trait]][trait_sample_indices]
+        )
+      )
+      names(ensemble.samples[[pft]]) <- names(pft_traits)
     }
   } else {
-    PEcAn.logger::logger.error(samples.file, "not found, this file is required by the run.write.configs function")
+    # use pre-generated samples
+    ensemble.samples <- existing_data$ensemble.samples
   }
 
   ## remove previous runs.txt