JGCRI / hector

The Hector Simple Climate Model
http://jgcri.github.io/hector/
GNU General Public License v3.0
111 stars 47 forks source link

Spurious errors showing up in GA tests, failing pkgdown #692

Closed bpbond closed 1 year ago

bpbond commented 1 year ago

Summarizing what I know.

Error in pkgdown GA

The v3 pkgdown GitHub Action is failing:

! path[1]="tables/ssp245_emiss-constraints_rf.csv": No such file or directory

The source of the 'error'

This isn't an error; it's a message originating from the intended functionality of ini_to_core_reader.cpp:

            #ifdef USE_RCPP
            // ANS: This is the algorithm used if Hector is compiled
        // as an R package.
            //
        // If the csvFileName normalizes to a real path, use that.
            // Otherwise, assume that it is pointing to a file in the
            // same directory as the INI file.
            try {
                // Second argument is "winslash", which is only used
                // on Windows machines and is ignored for Unix-based
                // systems. "\\" (single backward slash) is the
                // default. Need it here to access the third argument
                // -- mustWork -- which throws the error to be caught
                // if the path doesn't exist
                csvFileName = Rcpp::as<string>(normalizePath(csvFileName, "\\", true));
            } catch (...) {
                // Get the full path of the INI file with
                // normalizePath. Then, get just the directory using dirname.
                Rcpp::String parentPath = dirname(normalizePath(reader->iniFilePath));
                csvFileName = Rcpp::as<string>(filepath(parentPath, csvFileName));
            }

Summarizing: we use normalizePath to try to find the csv: file, and if that doesn't work, catch the error and, assuming that it's a relative path, add on the path of the source INI file. This is old code; functionality has not changed in years.

The other GAs are seeing this error too, but ignoring it

From the same PR, the R-cmd-check test:

Screenshot 2023-02-04 at 12 00 48 PM

@kdorheim THIS IS THE ISSUE YOU FLAGGED and I said, pfft, let's ignore it for now. 🤦

bpbond commented 1 year ago

So we have two problems:

  1. The 'error message' generated in the try-catch block of ini_to_core_reader.cpp is being printed (I think?) and then 'seen' by R, even though the error was caught in the C++ code. This didn't use to occur
  2. In the pkgdown GA, this causes failure because knitr sees the vignette 'failing' and errors out

Note: the code is still running OK!

Even though an 'error message' is generated, the R code still runs fine:

> ini <- system.file("input/hector_ssp245.ini", package = "hector")
> hc <- newcore(ini)
Error in (function (path, winslash = "\\", mustWork = NA) :
path[1]="tables/ssp245_emiss-constraints_rf.csv": No such file or directory

[many identical messages, one for each `csv:` reference in the INI]

> run(hc)
Hector core:    Unnamed Hector core
Start date: 1745
End date:   2300
Current date:   2300
Input file: /Users/bpbond/Dropbox/Documents/Work/Code/hector/inst/input/hector_ssp245.ini

[fetchvars and plot, everything looks good]
bpbond commented 1 year ago

Previously good and merged PRs fail

I have tried branching from a number of different commits in the v3_dev history and then opening a PR, and the same thing happens (e.g. https://github.com/JGCRI/hector/pull/690/ ).

Not a pkgdown.yaml problem

Sort of a corollary of the point above. I've tried lots of different tweaks to pkgdown.yaml, making sure it has the same setup as R-CMD-check.yaml, trying older (e.g. from 2022) versions, etc.

This implies it's an R/environment/software stack problem?

bpbond commented 1 year ago

Local replication

I have been able to replicate this locally, so successfully that I've borked my local installation!

Currently for me with v3_dev HEAD:

So currently any call to the ini_to_core.cpp code now produces 'error messages' on my system. This occurred after I tried to follow pkgdown::build_site() and ended up running

pkg <- as_pkgdown(".", override = list())
utils::install.packages(pkg$src_path, repos = NULL, type = "source", quiet = FALSE)

(which it does in its code)

I've tried uninstalling the hector package, restarting R... 😕

Strictly an R issue

There's no behavior change when running the C++ code.

bpbond commented 1 year ago

Summary

This is confusing. I don't think it's a YAML file issue, but rather an R issue and/or behavior difference between the devtools toolchain and R CMD ..., something that changed fairly recently. What's suspicious, of course, is that this is right when @kdorheim and I were upgrading the GA YAML files.

@pralitp @crvernon

~I'm not sure what to try next here...any thoughts? Does either of you have bandwidth to look at this next week? It's holding up the entire Hector v3 release.~ edit: never mind!

Possible workarounds:

bpbond commented 1 year ago

Well, this was certainly a pain in the rear, but:

I have merged #693 that is not a "workaround" but rather a fix of the root issue: the janky try/catch approach taken in ini_to_core_reader.cpp. The new code uses file.exists instead and is shorter and simpler. 🎉

Even simpler would be to rip out Rcpp usage entirely in this file, and just depend on std::filesystem! I couldn't quite get this to work — see #694 — but enough already, closing that PR but will open an issue for the future.