Skip to content

print.data.table: check for knitr at sys.calls()-6#6563

Merged
tdhock merged 2 commits intopatch-1.16.2from
bandaid-6509
Oct 9, 2024
Merged

print.data.table: check for knitr at sys.calls()-6#6563
tdhock merged 2 commits intopatch-1.16.2from
bandaid-6509

Conversation

@aitap
Copy link
Copy Markdown
Member

@aitap aitap commented Oct 9, 2024

This is a band-aid for #6509. Previous check for mimicsAutoPrint (i.e. knit_print.default) at sys.calls()[[length(sys.calls())-3L]] no longer works after a knitr upgrade. Currently, the call to the autoprint-like function resides at position -6.

Edit: unfortunately there doesn't seem to be a right way to solve this problem. Returning invisible(anything) from [.data.table sets it back to visible because PRIMPRINT(.Primitive("[")) says so, and knitr respects visibility of the evaluation results.

Previous check for mimicsAutoPrint (i.e. knit_print.default) at
sys.calls()[length(.)-3L] no longer works after a knitr upgrade.
Currently, the call to the autoprint-like function resides at
position -6.
@aitap aitap requested a review from MichaelChirico as a code owner October 9, 2024 09:50
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 9, 2024

Comparison Plot

Generated via commit ffeb415

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 11 minutes and 25 seconds

Time taken to run atime::atime_pkg on the tests: 1 minutes and 50 seconds

Comment thread R/print.data.table.R Outdated
( length(SYS) >= 3L && is.symbol(thisSYS <- SYS[[length(SYS)-2L]][[1L]]) &&
as.character(thisSYS) == 'source') || # suppress printing from source(echo = TRUE) calls, #2369
( length(SYS) > 3L && is.symbol(thisSYS <- SYS[[length(SYS)-3L]][[1L]]) &&
as.character(thisSYS) %chin% mimicsAutoPrint ) ||
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the issue/fix, so comments would be super helpful here.
Could you please add somethign like # suppress printing in knitr, #6509
and maybe some explanation about where the magic number 6 comes from?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ffeb415 help?

The original problem is that [.data.table would like to return normally for DT[<any read-only operation>] but invisibly for DT[,<var> := <assignment by reference>].

Unfortunately, when evaluating calls to primitives, R checks a special flag for whether to restore the visibility, and the primitive `[` has this flag set to 0 (force R_Visible = TRUE), so there doesn't seem to be a way to return invisible(anything) from [.data.table.

To achieve something like that, data.table remembers the address of the last data.table on which a by-reference operation has been computed. print.data.table checks if the address of the argument matches the remembered one and, if it suspects it's being called for auto-printing (call stack very shallow or called by a specific knitr function), it refuses to print anything. After something in the knitr stack got upgraded, the check for "am I being called for auto-printing by knitr" no longer works. We could also change the check to "is knit_print.default anywhere in the call stack, not just at positions -3 or -6" and risk false positives.

We could try asking R core to change the flag on .Primitive("["), but almost always forcing visibility on for results of `[` is the right thing and it'll take years before we can enjoy one less workaround in the codebase.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the really good summary, really this should be in the codebase somewhere.

Copy link
Copy Markdown
Member

@tdhock tdhock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great comments thanks!

for a medium-term solution I wonder if rendering vignettes using litedown would fix this, or cause other problems? https://yihui.org/litedown/#sec-package-vignettes

haha

@tdhock tdhock merged commit 9de2567 into patch-1.16.2 Oct 9, 2024
@tdhock tdhock mentioned this pull request Oct 9, 2024
@MichaelChirico MichaelChirico deleted the bandaid-6509 branch October 9, 2024 19:15
@MichaelChirico
Copy link
Copy Markdown
Member

for a medium-term solution I wonder if rendering vignettes using litedown would fix this, or cause other problems?

Not really. The test is here to ensure that anyone using {knitr}+{data.table} gets the expected behavior when knitting their vignettes, regardless of what engine we use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants