Support source-module wrapped analysis replay#204
Conversation
| .decodeJaspPlotObject <- function(plot) { | ||
| tryCatch( | ||
| decodeplot(plot, returnGrob = FALSE), | ||
| error = function(e) plot | ||
| ) | ||
| } |
There was a problem hiding this comment.
Two issues.
- If a user loads a dataset, then does the analysis, it works. If they load another dataset afterward, and show then print/ save the plot from the previous analysis, this no longer works, no? We need some hook to know which encoding/ decoding object should be used for which plot object.
- If we do save or saveRDS on the output from jasp, then restart the r session and then try to replay the plot, all the information about encoding/ decoding is no longer available because this lives on the C++ side of jaspSyntax. We need a way around this. This could be done by immediately storing the decodedplot and not doing it on demand.
There was a problem hiding this comment.
Agreed; the later architecture now handles both cases without relying on the live dataset. runJaspResults() captures a per-analysis decode context after dataset preload, finishJaspResults() / .saveState() materialize stored state with that context, and state$figures stores decoded plot objects for replay/export. For saved or reloaded results, saveImage(), editImage(), and rewriteImages() use the stored-result-state path instead of the currently active dataset. The actual token replacement now comes from the opaque jaspSyntax/SyntaxInterface encoder context rather than an R-side mapping.
|
Implemented the eager-decoding follow-up from the review/spec discussion. What changed:
Companion jaspTools follow-up:
Verification:
|
|
Follow-up from the final desiderata audit: I found one remaining provenance gap in the live R6 wrapper path. oRObject() called the C++ oRObject() materializer first, and that C++ code can consult the currently active global decoder before the R-side eager decoder sees the object. That meant a live wrapper could still be decoded against the wrong dataset after an analysis/dataset switch, even though saved state was already eager-decoded. Fixed in 17d2533:unJaspResults() stores the per-analysis decode context on the result wrapper and propagates it to child wrappers.
Verification:
|
|
@vandenman this is ready for your review. The final desiderata pass is pushed, including the decode-context provenance follow-up for live result materialization and stored plot replay/export. |
Currently translated at 100.0% (26 of 26 strings) Translation: JASP/jaspBase Translate-URL: https://hosted.weblate.org/projects/jasp/jaspbase/vi/ Co-authored-by: Thành Khôi Lê <lethanhkhoi@gmail.com>
|
Follow-up after the direct I pushed
I also moved the per-analysis decode-context capture until after dataset preload, because that is when Verification after the change:
|
| defaultMapping <- tryCatch( | ||
| getExportedValue("jaspSyntax", "columnMapping")(strict = FALSE), | ||
| error = function(e) character() | ||
| ) |
There was a problem hiding this comment.
This should at least throw a warning, no? Same for the requestedMapping below?
There was a problem hiding this comment.
Yes. This changed in the later fallback-removal pass. The R-side mapping/requestedMapping fallback is gone, and encoded-token decoding now goes through jaspSyntax::decodeColumnText() backed by native SyntaxInterface. If encoded legacy state is encountered without a usable context, jaspBase warns that it cannot safely decode it instead of silently applying a stale or approximate R mapping; native decoder failures for encoded tokens propagate as errors.
| plt <- .decodeJaspPlotObject( | ||
| plt, | ||
| returnGrob = FALSE, | ||
| decodeContext = .jaspDecodeContext(source = "stored-result-state") | ||
| ) |
There was a problem hiding this comment.
In saveImage we should always use the returnGrob path because otherwise we might show encoded column names in plots within jasp.
There was a problem hiding this comment.
Agreed. saveImage() now decodes through .decodeJaspPlotObject(..., returnGrob = TRUE, decodeContext = .jaspDecodeContext(source = stored-result-state)), so the saved/printed object uses the stored result context and avoids showing encoded labels from a live-dataset mismatch.
| plot <- .decodeJaspPlotObject( | ||
| jaspPlotCPP$plotObject, | ||
| returnGrob = FALSE, | ||
| decodeContext = .jaspDecodeContext(source = "stored-result-state") | ||
| ) |
There was a problem hiding this comment.
revert this? when reassigned to $plotobject this should do the de/ encoding then within writeImage?
There was a problem hiding this comment.
This path changed with the stored-state architecture. We no longer rely on a live decoder or raw encoded object here; rewriteImages() decodes the stored plot object with .jaspDecodeContext(source = stored-result-state), edits the decoded editable object, and then assigns that decoded object back to plotObject. The actual rendering path still runs through writeImageJaspResults(), which keeps state/rendered output synchronized.
| plot <- .decodeJaspPlotObject( | ||
| jaspPlotCPP$plotObject, | ||
| returnGrob = FALSE, | ||
| decodeContext = .jaspDecodeContext(source = "stored-result-state") | ||
| ) |
There was a problem hiding this comment.
revert this as well, see prior comments.
There was a problem hiding this comment.
Same resolution as the prior plot-state comments: this is now using the stored-result-state context rather than live analysis state. editImage() decodes the stored editable plot object before resize/edit and assigns the decoded object back, while writeImageJaspResults() remains responsible for rendering and returning the object that should be persisted.
| .runWrappedAnalysisWithVerbosity <- function(expr, verbose = "analysis") { | ||
| verbose <- .normalizeRunWrappedAnalysisVerbose(verbose) | ||
| showAnalysis <- .runWrappedAnalysisShowsAnalysis(verbose) | ||
| showJasp <- .runWrappedAnalysisShowsJasp(verbose) | ||
|
|
||
| if (!showJasp) { | ||
| outputFile <- tempfile("jaspBase-runWrappedAnalysis-") | ||
| outputConnection <- file(outputFile, open = "wt") | ||
| outputSink <- sink.number(type = "output") | ||
| on.exit({ | ||
| while (sink.number(type = "output") > outputSink) | ||
| sink(type = "output") | ||
| close(outputConnection) | ||
| unlink(outputFile) | ||
| }, add = TRUE) | ||
|
|
||
| sink(outputConnection, type = "output") | ||
| } | ||
|
|
||
| if (showAnalysis) | ||
| return(.runWrappedAnalysisWithDecodedConditions(expr)) | ||
|
|
||
| suppressWarnings(suppressMessages(expr)) | ||
| } |
There was a problem hiding this comment.
this works, but why not adjust https://github.com/jasp-stats/jasp-desktop/blob/22c167b122776652968722eaf7c09debb24e3203/Common/log.cpp#L18 so that we can disable or enable this in a static jasp-build from jaspSyntax?
There was a problem hiding this comment.
Agreed, this belongs in Desktop rather than jaspBase. The corresponding Desktop PR now changes Common/log.cpp so logType::null is a real sink instead of std::cout, and the SyntaxInterface bridge can be silenced from jaspSyntax via the verbose toggle. See jasp-desktop#6249, specifically the Common/log.cpp reply explaining why syntaxBridgeSetVerbose(false) needed that change.
There was a problem hiding this comment.
Small correction to the cross-PR note: the final Desktop implementation no longer changes Common/log.cpp or the global logType::null default. That broader approach was reverted after Joris' review.
The current Desktop shape is narrower: SyntaxInterface initializes logging with the same Boost null stream pattern used by Engine/MainWindow, then toggles Log::setWhere()/Log::setDefaultDestination() for the bridge. So this remains owned by the native SyntaxInterface/desktop layer, but does not change global logging behavior in Common.
| plotObject <- .decodeJaspPlotObject(plot, returnGrob = FALSE, decodeContext = decodeContext) | ||
| plot2draw <- plotObject |
There was a problem hiding this comment.
this should be reverted, see prior comments about encoding.
There was a problem hiding this comment.
Addressed by moving the decode ownership rather than keeping the earlier R-side approach. writeImageJaspResults() now receives/passes a decode context, decodes the plot through jaspBase's JASP-owned result boundary, and delegates token replacement to native jaspSyntax/SyntaxInterface. The R-side mapping fallback that motivated this concern has been removed.
| // Keep the state object in sync with the rendered object. R returns an | ||
| // editable decoded object for ggplot-like plots and a materialized object | ||
| // for function/base plots. | ||
| if(writeResult.containsElementNamed("obj")) |
There was a problem hiding this comment.
why did this need to change? This was not broken?
There was a problem hiding this comment.
This became necessary once we eagerly store decoded plot state. Without writing the returned object back into plotInfo[obj], the PNG could be rendered from a decoded/materialized object while the stored plot object remained encoded. The C++ change keeps the persisted state object in sync with the object returned by writeImageJaspResults() for all plot types, not only function/base plots.
| x | ||
| } | ||
|
|
||
| .decodeJaspFactorValues <- function(x, fieldName = NULL, decodeContext) { |
There was a problem hiding this comment.
this reimplements the decoding from jasp. But is this really the best appraoch? Shouldn't we just reinstantiate that object? See https://github.com/jasp-stats/jasp-desktop/blob/development/Common/columnencoder.h, we could make an R6 object in jaspSyntax that provides an API to this and then use that instead? This is reimplementing the exact logic in that file.
There was a problem hiding this comment.
Agreed. The current implementation no longer reimplements ColumnEncoder token replacement in jaspBase. That logic moved to the native bridge in jasp-desktop#6249, and jaspSyntax#8 carries the opaque Desktop encoder context plus calls native decodeColumnText(). jaspBase now only decides which JASP-owned result surfaces to traverse and passes encoded text to jaspSyntax/native decoding; the R mapping/gsub fallback has been removed.
Carry the native column decoder snapshot through jaspBase result materialization so tables, plots, wrappers, and stored state decode against the analysis dataset rather than live global state. Keep fallback decoding shallow when jaspSyntax is unavailable and avoid double-rendering function/base plots when writing images.
|
@vandenman I pushed the follow-up implementation based on the decoder-ownership discussion. The current architecture is now split as follows:
In this PR, jaspBase now carries the captured decoder context through result materialization, decodes tables/footnotes/stored figures/ggplot labels, and adds plot-specific decoding for ggplot data, mappings, layer data/mappings, facet mappings, and plot metadata. It still keeps arbitrary analysis-owned model state opaque, so this should not reintroduce the earlier too-deep object rewriting problem. This also fixes the MixedModelsLMM smoke case that exposed encoded names inside the returned plot object: with patched jaspBase installed into a temporary R library, Validation on Windows/R 4.6: focused jaspBase tests, focused jaspSyntax tests after rebuilding/installing against local SyntaxInterface, SyntaxInterface export check 16/16, and the MixedModelsLMM smoke above. |
|
Addressed the silent fallback concern in 3189fcc.\n\nWhat changed:\n- Removed .decodeJaspColumnTextWithMapping() and the R-side mapping/gsub fallback from result decoding.\n- Removed the �llowLiveFallback path through decodeColNames().\n- Result/state/plot/condition decoding now propagates native jaspSyntax::decodeColumnText() failures instead of returning encoded text.\n- decodeColNames() still passes plain text through outside JASP, but now errors if an encoded bridge token is encountered without an installed decoder.\n- decodeName() no longer swallows decoder errors before passing names into C++ wrappers.\n\nThe intended ownership is now strict: jaspBase owns R result traversal only; token replacement must come from jaspSyntax -> native SyntaxInterface / ColumnEncoder. jaspSyntax PR #8 contains the matching bridge cleanup; jasp-desktop #6249 owns the native decoder API.\n\nFocused checks passed locally against source-loaded jaspSyntax and jaspBase:\n- est-result-object-decoding.R\n- est-runWrappedAnalysis.R\n\nConsumer checks in jaspTools #79 also passed with local source-loaded jaspSyntax/jaspBase:\n- est-rbridge-shim.R\n- est-jaspSyntax-lifecycle.R\n- est-generated-example-tests.R\n\nThe native runtime still emits the known stack-imbalance warnings at process shutdown in the runWrappedAnalysis-focused test, but assertions pass. |
|
@vandenman I pushed the architecture-level follow-up from the decoding review. jaspBase no longer receives or normalizes R-side column mappings. It carries the opaque jaspSyntax/SyntaxInterface encoder context and treats native decoding failures for encoded tokens as bugs, while still limiting eager decoding to JASP-owned result copies rather than downstream module internals. Related PRs: jasp-desktop jasp-stats/jasp-desktop#6249, jaspSyntax jasp-stats/jaspSyntax#8, jaspTools jasp-stats/jaspTools#79. |
There was a problem hiding this comment.
I have tested jaspSyntax with this jaspBase, and it works.
If @vandenman agrees, we could merge this.
| .jaspEncodedColumnTokenPattern <- function() "(JaspColumn_[[:alnum:]_]+_Encoded|JaspExtraOptions_[[:alnum:]_]+_Encoded|jaspColumn[0-9]+)" | ||
|
|
||
| .containsJaspEncodedTokens <- function(x) { | ||
| if (!is.character(x) || length(x) == 0L) | ||
| return(FALSE) | ||
| any(grepl(.jaspEncodedColumnTokenPattern(), x, perl = TRUE), na.rm = TRUE) | ||
| } | ||
|
|
||
| .decodeJaspColumnText <- function(x, columnEncoderContext = NULL) { | ||
| if (!is.character(x) || length(x) == 0L) | ||
| return(x) | ||
| if (!.containsJaspEncodedTokens(x)) | ||
| return(x) | ||
|
|
||
| if (is.list(columnEncoderContext) && "columnEncoderContext" %in% names(columnEncoderContext)) | ||
| columnEncoderContext <- columnEncoderContext[["columnEncoderContext"]] | ||
| if (is.null(columnEncoderContext)) | ||
| return(x) | ||
|
|
||
| decoded <- tryCatch( | ||
| getExportedValue("jaspSyntax", "decodeColumnText")(x, columnEncoderContext), | ||
| error = function(e) { | ||
| stop( | ||
| "jaspBase result decoding requires a working native jaspSyntax column decoder: ", | ||
| conditionMessage(e), | ||
| call. = FALSE | ||
| ) | ||
| } | ||
| ) | ||
| if (is.character(decoded) && length(decoded) == length(x)) { | ||
| names(decoded) <- names(x) | ||
| decoded | ||
| } else { | ||
| stop("Native jaspSyntax column decoder returned an invalid result.", call. = FALSE) | ||
| } |
There was a problem hiding this comment.
Following the "parse don't validate" idea, just call the decoder immediately. Do not check for the presence of an encoded token, the columnencoder can do this much more efficiently. Do not validate the results afterward. All this should and is handled by the columnEncoder itself.
There was a problem hiding this comment.
Implemented in 1b414bf.
.decodeJaspColumnText() now only unwraps the stored decode context and delegates directly to jaspSyntax::decodeColumnText() whenever a context is available. I removed the jaspBase-side encoded-token pre-scan and the post-call result-shape validation, so token/no-token handling and decoder result contracts stay below this layer.
I also added a focused regression test that verifies plain text is still delegated when a decode context is present; that would have failed with the old pre-scan.
Focused checks passed locally with the matching local jaspSyntax/SyntaxInterface installed first on .libPaths():
test-result-object-decoding.Rtest-runWrappedAnalysis.R
|
Changes look good but this needs jaspSyntax to be available/ installable first. |
5 participants
Summary
This PR provides the jaspBase runtime side of the jaspTools -> jaspSyntax bridge:
runWrappedAnalysis()resolve analyses from explicit source module paths and QML files, not only installed module packages$toRObject(), including JASP-owned table names, footnotes, stored result-state figures, and ggplot labels/data/mappings/metadataWhy
Desktop/SyntaxInterface owns token replacement, jaspSyntax exposes that bridge, and jaspBase owns the boundary between backend analysis objects and R-facing result copies. This PR keeps that boundary explicit: JASP-owned display/result surfaces are decoded eagerly from the captured encoder context, while arbitrary downstream module internals stay opaque.
That fixes source-module replay and
toRObject()decoding without adding downstream module dependencies to core packages or duplicating Desktop encoding rules in R.Related PRs
Verification
test-result-object-decoding.R,test-runWrappedAnalysis.Rtest-desktop-jasp-contract.R,test-dataset-helpers.Rtest-jaspSyntax-lifecycle.Rgit diff --check