Carry native encoder context through analysis replay by FBartos · Pull Request #79 · jasp-stats/jaspTools

FBartos · 2026-05-14T12:04:31Z

Summary

This PR updates jaspTools as the orchestration layer for source-module analysis replay:

keeps the existing runAnalysis(..., verbose = ...) controls for separating analysis diagnostics from Desktop/QML/native chatter
stores the opaque jaspSyntax/SyntaxInterface column encoder context when a dataset is preloaded from a .jasp file or source dataset
passes that context into decodeAnalysisResults() so result replay uses the dataset/QML provenance that produced the analysis
removes the old preloaded column-mapping plumbing; jaspTools no longer performs or transports R-side decoding maps
preserves jaspTools as a thin bridge: it resolves inputs, runs the analysis, and forwards the captured context to jaspSyntax/jaspBase

Why

Encoding and decoding should be invisible to module authors and test authors. The lower-level PRs make Desktop the source of truth for token replacement; jaspTools only needs to keep the captured context attached to replay so old .jasp files and source-module tests decode the same way JASP would.

Related PRs

jasp-desktop native encoder context: Expose native column encoder context jasp-desktop#6249
jaspSyntax encoder-context R API: Expose Desktop encoder context for result decoding jaspSyntax#8
jaspBase result materialization/runtime replay: Support source-module wrapped analysis replay jaspBase#204

Validation

focused jaspTools lifecycle tests passed: test-jaspSyntax-lifecycle.R (133 pass, 3 gated real-Descriptives skips)
focused jaspBase tests passed: test-result-object-decoding.R, test-runWrappedAnalysis.R
focused jaspSyntax tests passed against the rebuilt local Desktop bridge: test-desktop-jasp-contract.R, test-dataset-helpers.R
R parse check passed
git diff --check

vandenman

Still need to actually run this, but some feedback below. The subprocesses idea looks interesting and has potential, but the current implementation looks a little fragile. Also, subprocesses are nice in theory but can make errors much harder to debug.

vandenman · 2026-05-14T12:12:55Z

 #'
 #' @export analysisOptions
-analysisOptions <- function(source) {
+analysisOptions <- function(source, modulePath = NULL) {


Isn't this always clear from context? E.g., when running jaspdescriptives it's clear that that is also always the module path?

Mostly yes, and the default path still does that inference from module.dirs / the active module context. I kept modulePath as an optional provenance override because .jasp files record module identity/version, not the local source checkout path. That matters for source-branch replay, generated tests, and multi-module archives; normal module workflows should not need to pass it.

Mostly yes for the ordinary module workflow, and the default still infers that path when modulePath = NULL. I kept the argument as an explicit provenance/source-checkout override for cases where the saved .jasp file identifies the module but not the local checkout to replay against. Clarified that in the analysisOptions() docs in e5e9411.

vandenman · 2026-05-14T12:13:39Z

+
+  modulePath <- .modulePathForAnalysisName(analysis, modulePath)
+  options <- jaspSyntax::readDefaultAnalysisOptions(
+    modulePath = modulePath,


Or is this the reason for the above? Still think we could infer the module path automatically?

Yes, this is the reason for the optional argument. The implementation still infers when modulePath = NULL; explicit modulePath only pins the source checkout or disambiguates a named module/analysis path collection. I kept this as orchestration/provenance, not extra QML semantics in jaspTools.

Yes, automatic inference remains the normal path. The explicit modulePath is only for pinning or disambiguating source replay: generated tests, local branch checkouts, or multi-module .jasp archives. I tightened the docs in e5e9411 to say that it should only be passed for those cases.

vandenman · 2026-05-14T12:15:14Z

-  rFuncLocExpr <- paste0("\\{[^\\{\\}]*func:\\s*", name, "[^\\{\\}]*\\}")
-  if (!grepl(rFuncLocExpr, fileContents))
-    stop("Could not locate qml file for R function ", name, " in inst/qml directory and did not find the R function in inst/Description.qml to look for an alternative name for the qml file")
+analysisOptionsFromQMLFileSubprocess <- function(analysis, modulePath = NULL) {


Why do we use subprocesses? That feels a little over engineered? I mean not a bad idea to be able to run tests in parallel in the future, but don't think that is your motivation right now?

Agreed for analysisOptions(). I removed the jaspTools subprocess path there; QML defaults now call jaspSyntax::readDefaultAnalysisOptions() directly. The .jasp extraction isolation remains in jaspSyntax, where the native bridge state is owned.

vandenman · 2026-05-14T12:17:32Z


-      return(list(value = value, types = typesStructure))
-    }
+`%||%` <- function(x, y) {


This exists in base R already since R 4.5.0 or so, so maybe use that one instead (and definitely avoid shadowing that one otherwise).

Agreed. I removed the custom %||% helper so we do not shadow base R. The only remaining use case was a label fallback, now handled by a named internal helper.

vandenman · 2026-05-14T12:21:12Z

+
+.jaspToolsLaunchSubprocess <- function(scriptPath, inputPath, outputPath, logPath) {
+  rscript <- file.path(R.home("bin"), if (.Platform$OS.type == "windows") "Rscript.exe" else "Rscript")
+  system2(


Yeah so if we really want this we should consider mirai or processx rather than doing it "manually".

Agreed. I replaced the manual Rscript/system2 runner with callr, which uses processx underneath. I kept subprocess containment only for runAnalysis(): while checking this against jaspMixedModels, direct in-process replay of the Larks and Owls GLMM still crashes R with access violation 0xC0000005, so the child process is currently protecting the parent/test session from native crashes rather than preparing for parallelism.

vandenman · 2026-05-14T12:21:58Z

+
+.jaspToolsSubprocessScript <- function(resultLines, saveLines,
+                                       beforeResultLines = character(0),
+                                       statusExpression = "inherits(result, 'jaspTools.subprocessError')") {


This much code as a string seems very prone to breakage and hard to debug?

Agreed. The long generated child script is gone. The child process now runs normal internal R functions via callr, so the code is easier to debug and covered like ordinary R code.

vandenman · 2026-05-14T12:23:43Z

+  if (!is.character(x) || length(x) == 0L)
+    return(x)
+
+  tokenPattern <- "(JaspColumn_[[:alnum:]_]+_Encoded|jaspColumn[0-9]+)"


Is this still necessary? Decoding is handled by jasp Syntax?

Agreed. I removed canonicalizeJaspColumnTokens() and the compatibility test around native/legacy encoded tokens. Runtime result decoding is handled by jaspSyntax::decodeAnalysisResults(); expect_equal_tables() now compares strings literally again.

vandenman · 2026-05-14T12:26:56Z

+runAnalysisSubprocessScript <- function() {
+  .jaspToolsSubprocessScript(
+    beforeResultLines = "warnings <- character(0)",
+    resultLines = c(


Same stuff about very long code as text

Agreed. The long code-as-text path in runAnalysis() is gone as part of the callr refactor.

FBartos · 2026-05-14T13:07:28Z

Pushed follow-up commit e464286 addressing the review pass.

Summary:

kept modulePath as an optional source-checkout/provenance override, while preserving automatic inference for the normal module workflow;
removed the analysisOptions() subprocess path entirely;
replaced the manual Rscript/string-script runner with callr for runAnalysis() crash containment only;
removed the custom %||% helper to avoid shadowing base R;
removed canonicalizeJaspColumnTokens() because result decoding belongs to jaspSyntax.

Verification:

focused jaspTools bridge tests pass: test-analysisOptions.R, test-expect-equal-tables.R, test-rbridge-shim.R, test-jaspSyntax-lifecycle.R;
git diff --check passes;
checked the jaspMixedModels Larks and Owls GLMM path: options and dataset extraction succeed, but direct in-process runAnalysis() still hard-crashes R with access violation 0xC0000005; the new callr path contains that crash and reports a parent-side subprocess failure instead of taking down the session.

FBartos · 2026-05-14T14:36:05Z

Follow-up from the jaspMixedModels replay crash reported against jaspTools::runAnalysis():

I reproduced the failure through this PR's public path, but the fix belongs in the lower bridge layer and was pushed to the linked jaspSyntax PR:

jaspSyntax PR: Add native bridge APIs for JASP replay jaspSyntax#7
Fix commit: 79258ee Preserve JASP replay provenance
Detailed root-cause/verification note: Add native bridge APIs for JASP replay jaspSyntax#7 (comment)

No new jaspTools patch was needed. The important behavior is that jaspTools::extractDatasetFromJASPFile() now receives a data frame with .jasp provenance from jaspSyntax; when runAnalysis() preloads it, jaspSyntax::loadAnalysisDataset() reuses the saved .jasp archive as the native dataset source. That preserves Desktop column typing for real replay instead of forcing jaspTools to know or reconstruct QML/data semantics.

Verified locally with jaspMixedModels/examples/Larks and Owls.jasp: jaspTools::runAnalysis('MixedModelsGLMM', dataset, opts) now returns results,status,typeRequest,state instead of the callr/native crash.

FBartos · 2026-05-14T18:09:52Z

Final cross-repo follow-up from the senior pass: no additional jaspTools code changes were needed here.

The remaining failures were below the orchestration layer:

jaspSyntax needed hardening around module-description parsing, .jasp dataset provenance, strict decoder boundaries, and source-checkout subprocess DLL ordering. Those fixes are in Add native bridge APIs for JASP replay jaspSyntax#7.
Desktop/SyntaxInterface needed native fixes for QGuiApplication argument lifetime and archive-version-aware DB loading. Those fixes are in Expose SyntaxInterface bridge APIs for jaspSyntax jasp-desktop#6221.

With those branches installed together, the reported MixedModels path now completes:

analysisOptions("Larks and Owls.jasp") returns saved options.
extractDatasetFromJASPFile("Larks and Owls.jasp") returns the expected 260 x 9 dataset.
runAnalysis("MixedModelsGLMM", dataset, opts) returns a result list with status = "complete".

This keeps jaspTools as orchestration only, which matches the review direction: QML/native semantics stay in jaspSyntax and Desktop rather than being reimplemented here.

FBartos · 2026-05-14T19:42:43Z

I chased the noisy focused-test warnings through the bridge stack. There are no jaspTools code changes for this follow-up; the fixes landed in the owning layers:

Desktop PR: guards the headless QML layout edge case, adds terminal SyntaxInterface shutdown, and avoids eager parse-only jaspBase/friendly-helper startup.
jaspSyntax PR: calls the terminal shutdown on unload/session exit and fixes Windows DLL bundling order so a rebuilt build/R-Interface/libR-InterfaceNoRInside.dll is not overwritten by a stale build-root DLL.

With those PRs pushed, fresh installed jaspSyntax focused tests are clean for the prior GridLayout, qml:, QThreadStorage, and stack imbalance noise. No orchestration change was needed in jaspTools.

FBartos · 2026-06-01T09:08:31Z

Architecture re-check done after the latest jaspBase/jaspSyntax/native-decoder changes. I found and fixed one remaining boundary leak: jaspTools was still attempting to call a public jaspBase state decoder, but that decoder is intentionally internal in jaspBase PR #204. jaspBase already decodes result state before saving it, so jaspTools now only reads the saved callback state and leaves state traversal/decoding in jaspBase. Pushed as aaf02f1.\n\nThe current ownership boundaries are now:\n- jasp-desktop PR #6249 owns native token decoding through SyntaxInterface / ColumnEncoder.\n- jaspSyntax PR #8 exposes the native bridge and handles JSON result payload decoding via that bridge.\n- jaspBase PR #204 owns R-facing result/state traversal and keeps .decodeJaspResultState internal.\n- this jaspTools PR orchestrates options, dataset loading, and runWrappedAnalysis replay without reimplementing token/state decoding.\n\nFocused checks pass locally with source-loaded jaspTools:\n- test-rbridge-shim.R\n- test-jaspSyntax-lifecycle.R (3 real Descriptives integration tests skipped unless explicitly enabled)\n- test-generated-example-tests.R

FBartos · 2026-06-02T15:03:57Z

Follow-up on the silent fallback review: no jaspTools code changes were needed for this part. The fallback removal landed in the owning packages:

jaspSyntax Fix dependency writing of plot tests #8, commit 2b2d73f: encoded-token decoding now requires native SyntaxInterface decoding; no R mapping/gsub fallback.
jaspBase #204, commit 3189fcc: result/state/plot/condition traversal now propagates native decoder failures instead of falling back to R replacement.

I reran the jaspTools bridge checks against source-loaded local jaspSyntax/jaspBase and they pass:

test-rbridge-shim.R
test-jaspSyntax-lifecycle.R
test-generated-example-tests.R

vandenman

Looks good, mostly a few smaller things.

vandenman · 2026-06-22T08:37:00Z

    if (missing(installJaspCorePkgs)) {
      title <- if (jaspBaseIsLegacyVersion()) {
-        "- Would you like jaspTools to install jaspResults, jaspBase and jaspGraphs? If you opt no, you must install them yourself."
+        "- Would you like jaspTools to install jaspResults, jaspBase, jaspSyntax and jaspGraphs? If you opt no, you must install them yourself."


This is not going to work. Remove the jaspSyntax from here and do not install it below.

Agreed. Addressed in e5e9411: setupJaspTools() no longer mentions or installs jaspSyntax, and the startup update check no longer recommends updating it through the jaspTools core-package path. jaspSyntax remains in DESCRIPTION as an ordinary import because the bridge APIs are still required by this PR.

vandenman · 2026-06-22T08:37:55Z

+.jaspToolsRunSubprocess <- function(task, payload, failureMessage,
+                                    isError = function(result) FALSE) {
+  logPath <- tempfile(paste0("jaspTools-", task, "-"), fileext = ".log")
+  result <- tryCatch(
+    callr::r(


Why do we run stuff in subprocesses? This means things like "browser()" will no longer work?

Agreed after re-checking the tradeoff. Addressed in e5e9411 by removing the subprocess path entirely: runAnalysis() now stays in-process, R/subprocess.R is deleted, the subprocess tests/options are gone, and callr was removed from DESCRIPTION. This keeps browser() and normal interactive debugging as the default behavior.

vandenman · 2026-06-22T08:40:53Z

@@ -0,0 +1,39 @@
+import QtQuick


So one conceptual question about this qml file which is used for tests.

Is it really the responsibility of jaspTools to test this? I guess we could, but it feels like something that belongs in jaspSyntax.

An alternative is to just do git submodule on an actual jasp module (and set it to the latest released tag or so).

Agreed. The direct jaspSyntax::readAnalysisOptionsFromQml() fixture test was testing jaspSyntax parser behavior rather than jaspTools orchestration. Removed that synthetic minimal module fixture and test in e5e9411; the remaining jaspTools tests cover delegation/wiring boundaries instead. I did not add a module submodule here because that would turn this narrow bridge coverage into heavier integration setup.

Remove runAnalysis subprocess containment so local replay remains debuggable in-process. Stop setupJaspTools from installing or update-checking jaspSyntax while keeping it as an Imports dependency, and remove the synthetic QML parser fixture that belonged in jaspSyntax coverage rather than jaspTools.

vandenman · 2026-06-22T14:28:15Z

Changes look good but this needs jaspSyntax to be available/ installable first.

Migrate local analysis replay to jaspSyntax

fd212e4

FBartos requested a review from vandenman May 14, 2026 12:09

vandenman requested changes May 14, 2026

View reviewed changes

Simplify jaspTools bridge subprocess handling

e464286

FBartos added 4 commits May 14, 2026 22:40

Prune stale bridge compatibility code

ef194f2

Align bridge setup and extraction tests

4d3e3fe

Remove machine-specific bridge test paths

039104e

Delegate result state decoding to jaspBase

53dec21

FBartos mentioned this pull request May 26, 2026

Update jaspBase for decoded result objects jasp-stats/jasp-desktop#6235

Closed

FBartos added 3 commits May 27, 2026 09:12

Forward quiet flag to wrapped analysis

9dde127

Add runAnalysis verbosity levels

db6ab20

Honor jaspSyntax verbosity defaults

91a1e44

FBartos mentioned this pull request Jun 1, 2026

Expose Desktop encoder context for result decoding jasp-stats/jaspSyntax#8

Merged

Keep result-state decoding in jaspBase

aaf02f1

Carry encoder context through analysis replay

07e8fbe

FBartos changed the title ~~Migrate local analysis replay to jaspSyntax~~ Carry native encoder context through analysis replay Jun 3, 2026

This was referenced Jun 3, 2026

Carry native encoder context through analysis replay #80

Closed

Expose native column encoder context jasp-stats/jasp-desktop#6249

Merged

Require updated jaspSyntax bridge API

3c15649

vandenman requested changes Jun 22, 2026

View reviewed changes

Uh oh!

Conversation

FBartos commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Related PRs

Validation

Uh oh!

vandenman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FBartos commented May 14, 2026

Uh oh!

FBartos commented May 14, 2026

Uh oh!

FBartos commented May 14, 2026

Uh oh!

FBartos commented May 14, 2026

Uh oh!

FBartos commented Jun 1, 2026

Uh oh!

FBartos commented Jun 2, 2026

Uh oh!

vandenman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

FBartos commented May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading

FBartos May 14, 2026 •

edited

Loading