Repository navigation
Inconsistent semantics after setDT #4783
Description
Activity
One way out of this would be to disable the modify-in-place optimization. To us the little extra memory consumption is certainly worth the gain in consistency.
I think it's in everyone's best interest to disable it always, but if not - at least add an option to do it.Reacted by HughParsonageas.data.tableis guaranteed to copy, and of course there'scopy. If you are trying to get around modify-in-place, either of these are fine:d2 = copy(d1) d2 = as.data.table(d1)@MichaelChirico Thank you. I'm well aware of the alternatives - but I'm not doing exploratory interactive work. I work in a team of R developers with a large R codebase riddled with data.tables and setDT, and still hope this fundamental bug can be solved and not worked around at the user side.
@OfekShilon
I think it is a bit harsh to describe this behaviour as a "fundamental bug". It is absolutely clearly stated in the manual ofsetDT()thatsetDT()modifies its input by reference. So what do you want to achieve in your example? If you want to modifyd1by reference, why do you needd2? If you want to modifyd2without modifyingd1, you have to follow the advice above and usecopyoras.data.table. It seems you want to rely on some consequences of (undocumented and unexported) implementation details. Not a good idea. If your codebase contains suchsetDTcalls, you have to fix them instead of offending the authors of data.table.Note also that
data.table:::shallow()is not (yet) exported (see also #2323), probably not by accident.Note 2: Maybe you shall reformulate your issue as a feature request instead of a "bug report".
Reacted by Dirk Eddelbuettel and Cole Miller@tdeenes I know how to work around this behaviour, with the advice above and in other ways. This does not make this reported behaviour not a bug. Is the expectation for consistency really a 'feature request'?
I'm afraid the usage of 'by reference' in the documentation and discussions on data.table is often ambiguous, and careful distinction is needed. It might mean two different things:
(1) A data table object (pointing to multiple column data) is not copy-on-write. Therefore, when multiple names are bound to the same data.table - every modification to the data.table via one name manifests in all the names.
(2) The column data is sometimes modified in place (cases 3&4 in the original bug), perhaps you mean this too when you write 'by reference'. When no data.frames are involved - this is not a problem.Both these behaviours seem reasonable design choices - and usually are. Specifically, when the data.table was created either by
data.table(***)oras.data.table. Where things go awry is when a data.table is generated bysetDT.
The documentation forsetDTindeed absolutely clearly states that it operates by reference, but this report is about the following operations on the resulting dt , and whether these are by reference or not.dt<-df; setDT(dt)performs a shallow copy, so nowdtanddfare distinct objects pointing to the same columns. What does 'by reference' even mean in this context? Would you expect modifications todtor its columns (these are different things!) to manifest indf?
Whatever you choose 'yes' or 'no', sometimes the current implementation would adhere to your expectation and sometimes not. This cannot be accepted as 'by design'.Not sure what made you say that I want to rely on undocumented and unexported implementation details. I don't. My examples were entirely public and very basic (even fundamental) data.table interfaces, and the results are inconsistent.
dt<-df; setDT(dt) performs a shallow copy, so now dt and df are distinct objects pointing to the same columns. What does 'by reference' even mean in this context? Would you expect modifications to dt or its columns (these are different things!) to manifest in df?
This is the crucial point I guess; my interpretation of the documentation is that you shall not ask these questions when using
setDTon data.frames (or lists). Just accept that you can not know which modifications ofdtor its columns will manifest indf. I really can not imagine a legitimate use case where one wants to use a data.frame for breaking the standard copy-on-write semantics of R. So you have only two options:- You want to keep
dfas it is -> you have to protect it before callingsetDTon it or on any objects which keeps a reference to it. - You do not bother what happens to
df, you need a data.table -> you callsetDTon it and continue with the return value ofsetDT.
setDT,:=, and otherset*functions give you great power which comes with great responsibility. So when you use them, as you did in the following operations on the resulting dt, you have to be sure that you had not left behind any list or data.frame objects which you want to re-use later and whose elements are shared withdt.If you think your use case does not fit into 1) or 2), please provide us with a minimal reproducible example. Note that your current example belongs to 2) unless you wanted to keep
dfas it is; in the latter case your example is an example for the misuse ofsetDTand:=.- You want to keep
Sorry, I failed to follow the link which points to the original issue (#4589) in which @mattdowle and others gave you a pretty exhaustive explanation for the proper use of
setDT. Upon a quick check of your comments there and in this issue, it seems you want to get data.table-features but keep your object as a data.frame. I still do not get the idea behind this, but I definitely find it inappropriate to refer to the current behaviour ofsetDTas a "fundamental bug" after the first author of the package gave you a clear description of why this is not a bug and a major contributor of the package improved the documentation based on your original issue.The code example is of course simplified, but lots of very real use cases exist. A prominent one is using setDT inside a function - in that discussion a data.table maintainer (Arun) expressed the will to have such cases resolved.
I think I just hit a very related point, with a similar use case here? #4816 (comment)
We keep getting bit by this. Perhaps the original example (
d1<-data.frame(a=1); d2 <- d1; setDT(d1)) seemed contrived? It is just an extra simplification of real life, where the issue often surfaces when trying to operate on function arguments:> df <- data.frame(a=1:2) > f <- function(x) { + setDT(x) + x[ , b := 5:6] # doesn't leak to df + x[!is.na(a), a:=3:4] # leaks to df :( + setDF(x) + } > > f(df) > df a 1: 3 2: 4I think the solution to this might just be to warn users (via ?setDT) with something along the lines of: "use of setDT inside of function definitions, especially on objects that were passed as arguments, may cause unpredictable modification of objects outside the scope of the function. When writing a function, we recommend either A) using as.data.table (not setDT) inside functions. This guarantees a side-effect free ("pure") function or B) Write functions that expect (and are documented as expecting) data.tables as inputs. This allows creating "functions" which are pass-by-reference (ie, they avoid copying) but behave like procedures (in that there may be side-effects). "
As a bit of an aside, I'll add that I have some experience with a third approach, in the package intervalaverage, where data.tables are explicitly required as inputs (and thus are passed by reference) but the function is carefully written to restore any changes on.exit(). This results in a pure function that benefits from pass-by-reference speed without any side-effects (which are unpredictable to the typical R user who expects functions to behave like pure functions). The approach used in intervalaverage (pure functions using pass-by-reference under the hood), only makes sense if you want to return an entirely new table. If you want to modify the original table, approach B is "best" (although potentially unfamiliar to R users).
related post I made nearly a decade ago on SO: https://stackoverflow.com/questions/13756178/writings-functions-procedures-for-data-table-objects
@myoung3 this is pretty much what we try to do now. However in an enterprise-size codebase like ours (~600K R LOC, in ~12 large in-house packages) if you can't transition to data.table gradually or use it eliminate specific bottlenecks in a pipeline - it's very hard to use it at all. We tried various techniques and conventions, but this data.table inconsistent behavior is a major, major pain for us (and I suspect for others).
Also, warnings as you suggest don't solve other similar situations.d1 <- data.frame(a=c(1,2,3,4,5), b=c(2,3,4,5,6)) d2 <- d1 setDT(d2) d2[, b:=3:7] # impacts only d2 d2[!is.na(a), b:=5:9] # leaks to d1One technical solution might be for
setDTto mark the input columns as 'setDT generated', and then use this marking to bypass thememrecyclecall in the C functionassign.Reacted by Michael Young@OfekShilon with an R footprint that large, it would certainly make sense for some engineering effort to be "donated" to support us in fixing
setDT. We accept PRs.@MichaelChirico I can try - but do you guys now agree that this is a problem to be solved? Seems most of this thread doesn't.
- I mean, will you merge such a PR?
3 remaining items
Hey @OfekShilon really do appreciate your efforts here. We are simply bottlenecked on reviewer time. Appreciate your patience 🙌
I see Jan added this to our 1.14.3 milestone -- I believe we are prioritizing a release in the near future, so that should mean this gets eyes soon.
@jangorecki @MichaelChirico 1.14.3 flew by and again this PR+bugs are ignored. What can be done to get this to be discussed?
Hi Ofek, 1.14.4 wound up being a patch release to stay on CRAN, almost no recent work (including merged PRs) was included
Reacted by Ofek@MichaelChirico can this be added maybe to 1.14.5?
What I think @OfekShilon wants to happen is that after
setDT(x), neitherxnor its columns share storage with any other variables (This means «Do modifications to d2 impact d1?» — «no».)The opposite, i.e., given
x <- y, performingsetDT(x)to make bothxandyinto references to the samedata.table, is very hard or impossible to achieve within R's interface. We can find out thatxis shared with some other variable, but since the type and length of an R vector are fixed during its creation, makingxover-allocated involves creating a new object referencing its former contents (a "shallow copy") and then changing the variable binding. Theythat was formerly a reference toxnow references a different object. There is no way to find and replace it with the newxwithout sifting through all objects in the heap. (What ifyinstead lived in a list, not an environment? Should it remain shared then? This way lies madness.)In R ≥ 4.0, we can prevent the sharing, sort of. Variables now have reference counts! When the
setDT(...)argument is shared as a whole, we can see it:x <- data.frame(a = c(1,2), b = c(2,3)) y <- x # at this point: # REFCNT(x$a) == REFCNT(x$b) == 1 (both columns live in one list that we can call either `x` or `y`); this is fine # address(y) == address(x), so # REFCNT(y) == REFCNT(x) == 2 (they are the same object with two pointers aimed at it); this is not fine setDT(x) x[1, a:=10] y$a[1] # also 10
Solution: make a deep copy before continuing. If we took a shallow copy instead, the columns would become shared, causing us a problem (see below).
When columns inside the
setDT(...)argument are shared, we can also see it:x <- mtcars[names(mtcars)] # at this point: # address(x) != address(mtcars), this is fine # REFCNT(x) == 1 (x is a new list not shared with anything); this is also fine # REFCNT(x[[...]]) == REFCNT(mtcars[[...]]) == 2: every column now lives in two lists at the same time; this is not fine setDT(x) x[1, disp := 9999] datasets::mtcars$disp[1] # also 9999
Solution: duplicate every column that has a reference count above 1.
Cc: @yihui Would this help your friend?
Are there problems with this approach? Of course there are.
- We're not allowed to look at the reference counts themselves, only whether they are 0, 1, or ≥ 2. Also, giving a value as an argument to a function increments the reference count for the duration of the function call:
x <- 1 # REFCNT(x) == 1 (function(y) { y # REFCNT(x) == REFCNT(y) == 2 })(x)
We want to check that
REFCNT(x)andREFCNT(x[[i]])was 1 before the call tosetDT(x). This means substituting the name of the variable and performing the check in the C code before the promise is forced, incrementingREFCNT(x). OtherwiseREFCNT(x)is either 2 or more than 2 and we cannot distinguish between the two. Checks for other supported forms ofsetDT(...), such assetDT(x[[i]])orsetDT(get("foo"))are possible but not trivial. - This goes contrary to the documented ("the input object is modified in place with no copy" ... "If you require a copy, take a copy first") and tested behaviour of
setDT(setDT() fails in case of nested calls #6735):# More regressions noted in #6735 baz = function(x) setDT(x) x = data.frame(a=1) baz(x) test(2295.7, is.data.table(x))
(It's recognised that this is not good behaviour, but we already have reverse dependencies that break without it.)
- When a reference count of an object is incremented due to it being stored inside a parent object, R doesn't decrement it when the parent goes out of scope:
a <- 1 invisible(replicate(9999, list(a))) # reference `a` many times gc(full = TRUE) # and get rid of those references # REFCNT(a) = 20000
Decrementing the reference counts properly will involve patching the garbage collector and slowing it down approximately as much as
--with-valgrind-instrumentation=1(but without actually running under Valgrind). Without that fixed, we're going to produce unneeded copies. And on R < 4.0, we have to choose between false negatives (unintended data sharing as it happens now) and false positives (unintended copying).
If anyone is interested, I can submit the
check-refcntbranch as a pull request, but it's far from complete right now.- We're not allowed to look at the reference counts themselves, only whether they are 0, 1, or ≥ 2. Also, giving a value as an argument to a function increments the reference count for the duration of the function call:
(This is a cleanup and improvement of some of the #4589 discussion.)
Take this code:
Do modifications to
d2impactd1? We could live with both 'yes' or 'no', but the answer is sometimes:In cases 1&2
d2'plunks' the full columns into itself andd1isn't affected. In cases 3 & 4 it seems that operation-in-place optimization kicks in (address(d2$b)is unchanged), so there is no copy-on-write and data still pointed to byd1is overwritten.These semantic discrepancies make (the otherwise great) setDT unusable to us except in the most trivial scripts.
#Output of sessionInfo()R version 3.6.1 (2019-07-05)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19041)
Matrix products: default