Fix #36: Make vignette failures deterministic - #44
Merged
Conversation
Both vignette failures on the macOS runner share a shape: model fitting produces no usable model for most or all viable transitions, and that only surfaces much later, as "No fitted model for viable transition(s)" from allocation or as "No viable transitions found in trans_meta_t" after the reconciliation step. Neither message says what actually went wrong, which is why the platform difference is hard to read off the CI log. Two changes, one to remove a source of cross-platform divergence and one to make the remaining failures self-explanatory. create_trans_meta_t() numbered transitions in whatever order the rows arrived in. The `order()` call sat in `i` alongside a `:=` in `j`, which assigns in that order but does not reorder the table, so id_trans was assigned in grouping order instead. Its input is trans_v, an unordered DuckDB join whose row order is not guaranteed across platforms, thread counts or partition layouts, so id_trans -- a key that trans_preds_t, trans_models_t and trans_rates_t all reference -- was not reproducible. Sort explicitly before numbering, and cover it with a test that shuffles the input. The fit workers turn an error into a warning plus a sentinel row so that one unfittable transition does not discard the models that did train. Keep that, but record the error message in the sentinel's learner_params map (no schema change), report all failures of a batch in one warning naming each transition and its reason, and replay those reasons in the allocation error that reports the missing models. Also break ties in the best-partial-model query deterministically by learner_id, resolving the FIXME there: with two learners scoring an identical AUC the choice was left to scan order. Sentinel rows from failed partial fits are skipped, as they carry no learner to retrain. Refs #36 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UZRQbx2oqSzgNmp5zxuw5r
warn_failed_fits() re-reported, at the end of a fitting batch, what the workers had already warned about individually, and predict_trans_pot() then pointed back at those warnings by name. That reference is no help once the models come from another session: the warnings are gone, and only the record in trans_models_t remains. Drop the duplicate batch warning and let the per-worker warnings stand on their own. Where the missing model actually becomes fatal, in predict_trans_pot(), read the recorded error_message back out and re-emit one warning per transition before the error, so the reasons are in front of whoever hits the failure rather than in a log they never saw. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UZRQbx2oqSzgNmp5zxuw5r
knitr collects warnings into the rendered document, and a chunk that fails takes that document down with it, so the render log carries the fatal error and nothing that led up to it. That is why the macOS vignette failures report only "No fitted model for viable transition(s)" with no trace of the fitting warnings that explain it. Route them through evaluate()'s log_warning, via knitr's evaluate hook, and set warn = 1 so they print as they happen. Warnings then appear in the render log inline, attributed to the chunk that raised them, and -- unlike the knit_hooks$set(warning = ...) alternative, which only fires once a chunk completes -- including warnings raised inside the failing chunk itself. Verified that the rendered document is byte-identical. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UZRQbx2oqSzgNmp5zxuw5r
Renumbering id_trans also reorders the transitions the filter iterates over, which shifts the RNG stream GRRF draws from, so the expected importances needed the same re-baselining perf_expected already had. The values are not a relabelling of the old ones. Checked that the run is reproducible (ranger deactivates parallelism under regularization) and that this machine reproduces the perf_expected values committed alongside, so the two baselines agree on their origin. Loosened the tolerance to 1e-6, matching perf_expected. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UZRQbx2oqSzgNmp5zxuw5r
R CMD check picked up a new "no visible binding for global variable" for error_message, from the data.table expression in predict_trans_pot; add it to the globalVariables list. That check reports OK again. Alongside the existing pak/RSPM workflows, add a PR check that takes every dependency as an apt binary from r2u. r-cran-terra there declares Depends: libgdal34t64, libgeos-c1t64, libproj25, so GDAL and PROJ arrive prebuilt rather than being rebuilt, and nothing is compiled from source. All of this package's dependencies are in r2u for noble at the versions already in use, including duckdb 1.5.5 and terra 1.9-34. This deliberately does not layer r2u onto the rocker geospatial image: every r-cran-* deb declares Depends: r-base-core, meaning the apt R under /usr/lib/R, while the rocker images ship an R built from source under /usr/local/lib/R. Adding r2u there installs a second R and puts the debs in a library the running R never looks at. The job also runs tinytest directly with at_home = TRUE before the check. tests/tinytest.R leaves TT_AT_HOME unset, so under R CMD check every inst/tinytest/test_integ_*.R file exits early and the integration tests never run in CI at all. Untested: this workflow cannot be exercised from here, as the sandbox blocks Docker image pulls. The r2u package availability and dependency claims above were verified against the repository index. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UZRQbx2oqSzgNmp5zxuw5r
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are failure modes in the vignettes that make them fail for some as-yet undetermined reason on the macOS full R CMD check. This points to underlying problems with the determinism of
trans_meta_t, reporting on fitting errors, and the way that quarto vignettes log their errors.