rstudio / tensorflow

TensorFlow for R
https://tensorflow.rstudio.com
Apache License 2.0
1.33k stars 321 forks source link

Erroneous use of `getNamespaceVersion()` in a comparison #607

Closed TimTaylor closed 1 month ago

TimTaylor commented 1 month ago

As pointed out in this thread on R-devel, the following line could break at a future (sufficiently high) release of reticulate:

https://github.com/rstudio/tensorflow/blob/2ed2029989ff08c8a714faa82e60f64d84a82e5d/R/package.R#L63

getNamespaceVersion("reticulate")
#>  version 
#> "1.39.0"
getNamespaceVersion("reticulate") >= "1.99.0"
#> version 
#>   FALSE
getNamespaceVersion("reticulate") >= "1.100.0"
#> version 
#>    TRUE

This can be solved with either the suggested compareVersion() or via wrapping with as.package_version():

compareVersion(getNamespaceVersion("reticulate"), "1.38.0") >= 0
#> [1] TRUE
compareVersion(getNamespaceVersion("reticulate"), "1.100.0") >= 0
#> [1] FALSE

as.package_version(getNamespaceVersion("reticulate")) >= "1.38.0"
#> [1] TRUE
as.package_version(getNamespaceVersion("reticulate")) >= "1.100.0"
#> [1] FALSE
eddelbuettel commented 1 month ago

FWIW that is due to a recent-ish change in R itself. The change, IIRC, occurred on the 'other side' i.e. we now need to promote the comparison string:

edd@rob:~$ docker run --rm -ti r-base:4.3.3 Rscript -e 'getNamespaceVersion("base") > as.package_version("4.3.0")'
[1] TRUE
edd@rob:~$ docker run --rm -ti r-base:4.3.3 Rscript -e 'getNamespaceVersion("base") > as.package_version("4.4.0")'
[1] FALSE
edd@rob:~$ 

Ditto for 4.4.1 directly:

edd@rob:~$ Rscript -e 'getNamespaceVersion("base") > as.package_version("4.3.0")'
[1] TRUE
edd@rob:~$ Rscript -e 'getNamespaceVersion("base") < as.package_version("4.5.0")'
[1] TRUE
edd@rob:~$ 

But your overall point stands: reticulate needs such a cast either on the left or right to have the comparison work correctly.

t-kalinowski commented 1 month ago

Thank you both! This is very helpful and much appreciated. The fix will be on main shortly.