jtextor / dagitty

Graphical analysis of structural causal models / graphical causal models.
GNU General Public License v2.0
255 stars 47 forks source link

`dagitty("pdag{ ...}")` no longer working as previously? #88

Closed mmaechler closed 5 months ago

mmaechler commented 5 months ago

In earlier versions of dagitty, the following R code, (tried with R 4.2.1 (oldish on purpose, because I have an older dagitty version for that):

 dagitty::dagitty("pdag{ 1 -- 2 1 -> 3 2 -> 3 2 -- 4 2 -- 5 3 <- 4 3 -> 6 4 -- 5 4 -> 6 5 -> 6}")

nicely gave a result, printed as

pdag {
1
2
3
4
5
6
1 -- 2
1 -> 3
2 -- 4
2 -- 5
2 -> 3
3 -> 6
4 -- 5
4 -> 3
4 -> 6
5 -> 6
}
> 

where sessionInfo() ends in

loaded via a namespace (and not attached):
 [1] MASS_7.3-58.3   compiler_4.2.3  dagitty_0.3-1   graphics_4.2.3  tools_4.2.3     curl_5.0.0     
 [7] grDevices_4.2.3 Rcpp_1.0.10     V8_4.3.0        jsonlite_1.8.4  boot_1.3-28.1  

but in current R (4.3.2) and dagitty, I get

> dagitty::dagitty("pdag{ 1 -- 2 1 -> 3 2 -> 3 2 -- 4 2 -- 5 3 <- 4 3 -> 6 4 -- 5 4 -> 6 5 -> 6}")
Error: SyntaxError: Unexpected token )
Error: SyntaxError: Unexpected token )

(yes, the error is given twice; and is not at all helpful here, as we have no ) anywhere in the string we passed to {dagitty}. Enquiring about the "context", we see

> traceback()
9: stop(structure(list(message = "SyntaxError: Unexpected token )", 
       call = NULL, cppstack = NULL), class = c("std::invalid_argument", 
   "C++Error", "error", "condition")))
8: context_eval(join(src), private$context, serialize, await)
7: get_str_output(context_eval(join(src), private$context, serialize, 
       await))
6: evaluate_js(readLines(file, encoding = "UTF-8", warn = FALSE))
5: ct$source(system.file("js/dagitty-alg.js", package = "dagitty"))
4: .getJSContext()
3: .deleteJSVar(xv)
2: tryCatch({
       .jsassign(xv, as.character(x))
       .jsassign(xv, .jsp("DAGitty.GraphParser.parseGuess(global.", 
           xv, ").toString()"))
       r <- structure(.jsget(xv), class = "dagitty")
   }, error = function(e) {
       stop(e)
   }, finally = {
       .deleteJSVar(xv)
   })
1: dagitty::dagitty("pdag{ 1 -- 2 1 -> 3 2 -> 3 2 -- 4 2 -- 5 3 <- 4 3 -> 6 4 -- 5 4 -> 6 5 -> 6}")
> 

which may help you fixing the bug but does not help us much ...

Note this is breaking code that used to work in CRAN package pcalg, notably when calling adjustment() which then calls its own pcalg2dagitty().
We see this in our testing, but for some reasons, this is not currently visible in the pcalg CRAN checks, possibly because {dagitty} is not required, but just suggested by our pcalg package.

jtextor commented 5 months ago

Dear Martin,

Sorry for this. I did check all your pcalg tests, including those not run on CRAN (specifically all the tests in your file test_adjustment.R, which make extensive use of dagitty) before uploading the latest version and I ran all the pcalg checks myself on R 4.3.2 on 3 different platforms. So I am a bit puzzled by this error.

The error message probably indicates that already the dagitty library can't be loaded, which I've seen happen when the underlying V8 package was too old.

Can you please provide a sessionInfo() for the platform that it breaks on and also let me know which platform that is? Especially it would be important to know which version of the package V8 you have.

Best regards Johannes

mmaechler commented 5 months ago

Sure, here it is; V8 does look very new: packageDate("V8") giving "2023-12-04"


dagitty::dagitty("pdag{ 1 -- 2 1 -> 3 2 -> 3 2 -- 4 2 -- 5 3 <- 4 3 -> 6 4 -- 5 4 -> 6 5 -> 6}")
Error: SyntaxError: Unexpected token )
Error: SyntaxError: Unexpected token )
sessionInfo()
R version 4.3.2 (2023-10-31)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Fedora Linux 38 (Thirty Eight)

Matrix products: default BLAS: /usr/local.nfs/app/R/R-4.3.2-inst/lib/libRblas.so LAPACK: /usr/lib64/liblapack.so.3.11.0

locale: [1] LC_CTYPE=de_CH.UTF-8 LC_NUMERIC=C LC_TIME=en_US.UTF-8
[4] LC_COLLATE=de_CH.UTF-8 LC_MONETARY=en_US.UTF-8 LC_MESSAGES=de_CH.UTF-8
[7] LC_PAPER=de_CH.UTF-8 LC_NAME=C LC_ADDRESS=C
[10] LC_TELEPHONE=C LC_MEASUREMENT=de_CH.UTF-8 LC_IDENTIFICATION=C

time zone: Europe/Zurich tzcode source: system (glibc)

attached base packages: [1] stats utils methods base

loaded via a namespace (and not attached): [1] MASS_7.3-60.0.1 compiler_4.3.2 dagitty_0.3-4 graphics_4.3.2 tools_4.3.2
[6] curl_5.2.0 grDevices_4.3.2 Rcpp_1.0.12 V8_4.4.1 jsonlite_1.8.7 [11] boot_1.3-28.1

jtextor commented 5 months ago

This will be difficult to track down. There have been issues before with the package V8 specifically on Fedora Linux. I have just for that reason bought a small Fedora cloud server and tested dagitty on it. Sure enough it works on my Fedora system (Fedora 39), see below.

It would greatly help if you could give me some kind of remote access to your system so I could track down this bug, but of course I understand if that's not an option. Another thing that could be helpful is the output of the V8::engine_info() command that gives the version of the underlying V8 library:

> V8::engine_info()
$version
[1] "11.3.244.8-node.16"

$numeric_version
[1] ‘11.3.244.8’

Also looping in @jeroen to this discussion, the author of the V8 package who was always interested in this kind of Fedora issues :)

> dagitty::dagitty("pdag{ 1 -- 2 1 -> 3 2 -> 3 2 -- 4 2 -- 5 3 <- 4 3 -> 6 4 -- 5 4 -> 6 5 -> 6}")
pdag {
1
2
3
4
5
6
1 -- 2
1 -> 3
2 -- 4
2 -- 5
2 -> 3
3 -> 6
4 -- 5
4 -> 3
4 -> 6
5 -> 6
}
> sessionInfo()
R version 4.3.2 (2023-10-31)
Platform: x86_64-redhat-linux-gnu (64-bit)
Running under: Fedora Linux 39 (Thirty Nine)

Matrix products: default
BLAS/LAPACK: FlexiBLAS OPENBLAS-OPENMP;  LAPACK version 3.11.0

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

time zone: Etc/UTC
tzcode source: system (glibc)

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] dagitty_0.3-4

loaded via a namespace (and not attached):
[1] MASS_7.3-60    compiler_4.3.2 tools_4.3.2    curl_5.1.0     Rcpp_1.0.11   
[6] V8_4.4.1       jsonlite_1.8.8 boot_1.3-28.1 
> 
jeroen commented 5 months ago

I cannot reproduce this either. Indeed, what is the V8::engine_info() on this machine?

mmaechler commented 5 months ago
> V8::engine_info()
$version
[1] "3.14.5.10"

$numeric_version
[1] ‘3.14.5.10’

> sessionInfo()
R version 4.3.2 Patched (2024-02-06 r85870)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Fedora Linux 38 (Thirty Eight)

Matrix products: default
BLAS:   /sfs/u/maechler/R/D/r-patched/F38-64-inst/lib/libRblas.so 
LAPACK: /usr/lib64/liblapack.so.3.11.0

locale:
 [1] LC_CTYPE=de_CH.UTF-8       LC_NUMERIC=C               LC_TIME=en_US.UTF-8       
 [4] LC_COLLATE=de_CH.UTF-8     LC_MONETARY=en_US.UTF-8    LC_MESSAGES=de_CH.UTF-8   
 [7] LC_PAPER=de_CH.UTF-8       LC_NAME=C                  LC_ADDRESS=C              
[10] LC_TELEPHONE=C             LC_MEASUREMENT=de_CH.UTF-8 LC_IDENTIFICATION=C       

time zone: Europe/Zurich
tzcode source: system (glibc)

attached base packages:
[1] graphics  grDevices datasets  stats     utils     methods   base     

other attached packages:
[1] pcalg_2.7-10   fortunes_1.5-4 sfsmisc_1.1-17

loaded via a namespace (and not attached):
 [1] corpcor_1.6.10      cli_3.6.2           rlang_1.1.3         ggm_2.5.1          
 [5] RBGL_1.78.0         DEoptimR_1.1-3      jsonlite_1.8.8      clue_0.3-65        
 [9] V8_4.4.1            graph_1.78.0        stats4_4.3.2        dagitty_0.3-4      
[13] fastICA_1.2-4       abind_1.4-5         MASS_7.3-60.0.1     lifecycle_1.0.4    
[17] BiocManager_1.30.22 cluster_2.1.5       compiler_4.3.2      igraph_2.0.1.1     
[21] robustbase_0.99-0   Rcpp_1.0.12         pkgconfig_2.0.3     curl_5.2.0         
[25] magrittr_2.0.3      tools_4.3.2         boot_1.3-28.1       bdsmatrix_1.3-6    
[29] BiocGenerics_0.46.0
> 
jeroen commented 5 months ago

OK so that is the problem. You have accidentally installed the wrong (very old) legacy version of libv8 on the machine. Can you try the following:

yum remove v8-314
yum install v8-devel

And then reinstall the R package V8. And then if you do V8::engine_info() it should give you a much more recent version.

I am actually not sure why Fedora keeps providing this old legacy version, while they have a current one as well.

jtextor commented 5 months ago

Thanks @jeroen, yes that is probably it. I wonder if you'd recommend that I put a version check into dagitty to give a more informative error message if the V8 backend is too old? Do these version numbers follow a predictable format, and what would be a reasonable minimal version for it?

jeroen commented 5 months ago

You could add a version check for libv8 version 6 or newer, which is when they supported "modern" javascript.

But actually it may not be needed, because I plan to stop supporting the legacy (3.x) versions of libv8 very soon in the R package. Even the oldest Linux distributions now have a more current libv8, and it is often by accident that the user selects the legacy libv8, as above.

mmaechler commented 5 months ago

Well, I think you did notice that indeed, Fedora 's v8 is this old version; the 'devel' means providing the #include files, in addition, not using a newer v8; so your proposition cannot work (but I assume other ways would work; Fedora does provide newer version of v8 BTW, one usesdnfnotyumfor about 10 years nowadays (yumstill exists but rather back compatibility and is not recommended. If I diddnf remove v8it would remove many other things, including e.g.,rstudio`.. So you cannot expect maintainers of fedora networks to do this. Here this about maintenance of hundreds of desktops multicore compute-nodes and also laptops all runnning fedora 38 (till the summer break; then updating Fedora).

An alternative, seems to be to install the R-V8 fedora package. This install the V8 R package (into /usr/lib64/R/library). ... and even though its libs/V8.so show (via ldd to be linked dynamically with /usr/lib64/R/lib/libR.so , the standard Fedora "shared" version of R --- which I rarely use myself, it seems I can use V8, and more importantly dagitty in this case, finally without a problem also from my own self-compiled versions of R (e.g. 4.3.2 patched).

I think you should add this R-V8 requirement for current Fedora somewhere in your docs, or even SystemRequirements.

I'm closing the issue that it was indeed our platform {hundreds of computers though, even locally!} and using the R-V8 does work and is well maintainable.

jeroen commented 5 months ago

You should be able to remove the headers from the legacy libv8 and insttead install the headers of the current libv8 without breaking other software. This will make the R package compile against the correct libv8:

dnf remove v8-314-devel
dnf install v8-devel

This should not affect other software depending on v8, because the runtime libraries are in a separate packages v8-314 and v8 respectively, and these can both remain installed (concurrently) without problems.

The fedora rpm package R-V8 is just a compiled version of the R package; I don't think it make sense to declare a system-requirement of the R package on itself. But indeed it is a nice shortcut on Fedora that many R packages are available in binary form via the corresponding R-xyz rpm package.

mmaechler commented 5 months ago

You should be able to remove the headers from the legacy libv8 and insttead install the headers of the current libv8 without breaking other software. This will make the R package compile against the correct libv8:

dnf remove v8-314-devel
dnf install v8-devel

No, really no. There is no v8 package with a new libv8... v8 in Fedora is what you call v8-314 and v8-devel is v8+headers .. always the old version... v8-devel is already installed here (we do want the headers, as we usually do want to install everything from source!) The only "fully clean" thing is to wait for fedora to change the "libv8" version that belongs to the v8 (and hence(!) v8-devel) fedora package.

jeroen commented 5 months ago

OK well this is strange, it looks different on my machine. On Fedora 39 if I search for v8*devel there are three versions: v8-10.2-devel, v8-11.3-devel and v8-314-devel:

dnf search 'v8*devel'
# v8-10.2-devel.x86_64 : v8 - development headers
# v8-11.3-devel.x86_64 : v8 - development headers
# v8-314-devel.x86_64 : Development headers and libraries for v8

However, only versions 10.2 and 11.3 should be aliased as v8-devel. See also this comment from the Fedora maintainer himself. Indeed on F39 when I test this, we see v8-314-devel does not get listed:

dnf repoquery --whatprovides v8-devel | grep x86_64
# Last metadata expiration check: 0:12:27 ago on Thu Feb  8 17:56:28 2024.
# v8-10.2-devel-3:10.2.154.26-1.18.18.0.1.fc39.x86_64
# v8-10.2-devel-3:10.2.154.26-1.18.19.0.1.fc39.x86_64
# v8-11.3-devel-3:11.3.244.8-1.20.10.0.3.fc39.x86_64
# v8-11.3-devel-3:11.3.244.8-1.20.7.0.1.fc39.x86_64

So that means, a least on my machine it works as intended: v8-314-devel does not provide any v8-devel and if I do dnf install v8-devel I get v8-11.3-devel.

Screenshot 2024-02-08 at 19 36 08

You are right that there is no v8 package; the runtime library package is called nodejs-libs nowadays:

dnf repoquery --requires v8-11.3-devel
# /usr/bin/pkg-config
# libnode.so.115()(64bit)
# nodejs-devel(x86-64) = 1:20.10.0-3.fc39
# nodejs-libs(x86-64) = 1:20.10.0-3.fc39

However again v8 is provided as an alias for nodejs-libs

dnf repoquery --whatprovides v8
# nodejs-libs-1:20.10.0-3.fc39.x86_64
# nodejs18-libs-1:18.19.0-1.fc39.x86_64
mmaechler commented 5 months ago

Well, thanks to your persistence, I searched and tried a few things until I finally found the crucial way to get rid of the wrong /usr/lib64/libv8.so :

dnf remove v8-314
dnf install  v8-devel

The -314 part was really the missing part ! .... and you did have that already in your first suggestion; just using yum instead of dnf ... Don't know why I did not find the solution more quickly...