[r] Use lower and upper pin bounds on tiledb-r for release-1.9 branch

[johnkerl]

Issue and/or context: Per #2373 (comment) following advice https://stackoverflow.com/questions/63834412/enforce-upper-bound-for-r-package-version-dependency which is at variance with rstudio/renv#764 and, in fact, everything I've ever been told. I'm currently uncertain whether the bidirectional-pin syntax is genuinely new.

Changes: Try it out!

Notes for Reviewer: This PR is not ready for review. I will take this PR out of draft status as such time as it becomes ready for review.

[eddelbuettel]

To be plain, and at least to me, renv does not matter. It's an add-on with its own behavior. (The author is a close friend though.)

We would need the suite of package building and checking command, plus install.packages(), available.packages() and update.packages() to support it.

I also learned only this week (on list, from CRAN maintainer Uwe Ligges) that these packages functions support a filter argument that can be function (!!) so apparently one could express one's own preferences or constraints. I would recommend playing with small ad-hoc package on the side rather than burning cycles in our (constrained) CI. But that's just me.

Edit: Typo corrected. Picked the wrong noun in haste.

[codecov]

Codecov Report

Merging #2374 (3cb2e01) into release-1.9 (826957c) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@             Coverage Diff              @@
##           release-1.9    #2374   +/-   ##
============================================
  Coverage        90.43%   90.43%           
============================================
  Files               37       37           
  Lines             3940     3940           
============================================
  Hits              3563     3563           
  Misses             377      377           

Flag	Coverage Δ
python	90.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
python_api	90.43% <ø> (ø)
libtiledbsoma	∅ <ø> (∅)

[mojaveazure]

To be plain, and at least to me, renv does not matter. It's an add-on with its own behavior. (The author is a close friend though.)

+100; renv does weird things and while there's good intentions behind it, it's very easy to get out of whack. I would very much be in favor of not caring about whether or not we work with renv

everything I've ever been told

This behavior is new to me as well and AFAICT not well documented. I've also never seen it used anywhere. There is a lot of experimenting with bidirectional pins, repo combos, and install chains that I'd like to play around with before adopting this in any way

[johnkerl]

There is a lot of experimenting with bidirectional pins, repo combos, and install chains that I'd like to play around with before adopting this in any way

@mojaveazure +1.

#2373 is a known-good pattern which, factually, does solve the problem for CI. And I'm unaware of solutions I'm able to implement which are available to solve the problem outside of CI.

Those facts, combined with your assertion above which I agree with, mean:

• I will merge [r/ci] Controlled downgrade for TileDB-R 0.25 #2373 to keep our CI green, as it's a proved fact (four releases now) that it does keep our CI green
• Experiment here
• Anyone else who puts up a PR with a solution for non-CI users, I will be excited to read (and, who knows, this may end up being that PR ...)

[johnkerl]

CI failing as at
https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8711996536/job/23897314307?pr=2374
and
https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8711996548/job/23897314280?pr=2374
with

...
Get:67 https://r2u.stat.illinois.edu/ubuntu jammy/main amd64 r-cran-triebeard amd64 0.4.1-1.ca2204.1 [163 kB]

Get:68 https://r2u.stat.illinois.edu/ubuntu jammy/main amd64 r-cran-urltools amd64 1.7.3-1.ca2204.1 [274 kB]

Fetched 108 MB in 0s (0 B/s)                                                    
Error: pbmc3k.tiledb
pbmc3k.sce
Execution halted

which I do not understand 🤔 👀

[eddelbuettel]

That looks like an unrelated error with the Additional_repositories: entry we use for the two data packages. We made no changes to that aspect, and I think I had a similar 'intra-GH' fluke once before. (That Additional_repositories: is hosted at GH, but mortals like us do not know if in one or more data centers etc).

(And it just worked for me in a Docker container here.)

requested change was made

[mojaveazure]

I've done some experimentation with double-pinning and I don't think it actually works. I've got a branch off of release/1.8 with a double-pin of tiledb-r to >= 0.24.0, <= 0.24.99 that's built on R-universe (source). Using this branch, I've tried testing out double-pins and their effect on the installation and loading/attachment process, and so far I see no effect

R sees two versions of tiledb-r: 0.24.0 from my R-universe and 0.26.0 from CRAN/P3M

#8 [5/9] RUN Rscript -e "db <- utils::available.packages(filters = c('R_version', 'OS_type'))"     -e "db[db[, 'Package'] == 'tiledb', c('Version', 'Repository')]"
#8 2.478        Version  Repository
#8 2.478 tiledb "0.24.0" "https://mojaveazure.r-universe.dev/src/contrib"
#8 2.478 tiledb "0.26.0" "https://p3m.dev/cran/__linux__/jammy/latest/src/contrib"
#8 DONE 2.5s

R also sees my branch of tiledbsoma with the double-pin

#10 [7/9] RUN Rscript -e "uri <- paste(contrib.url(getOption('repos')[1L], 'source'), 'tiledbsoma_1.8.1.tar.gz', sep = '/')"     -e "dest <- tempfile()"     -e "dir.create(dest, showWarnings = FALSE, recursive = TRUE)"     -e "download.file(uri, destfile = file.path(dest, basename(uri)))"     -e "untar(file.path(dest, basename(uri)), files = 'tiledbsoma/DESCRIPTION', exdir = dest)"     -e "desc <- read.dcf(file.path(dest, 'tiledbsoma/DESCRIPTION'), fields = 'Imports')"     -e "unlink(dest, recursive = TRUE, force = TRUE)"     -e "print(desc)"
#10 0.740 trying URL 'https://mojaveazure.r-universe.dev/src/contrib/tiledbsoma_1.8.1.tar.gz'
#10 1.270 Content type 'application/x-gzip' length 720118 bytes (703 KB)
#10 1.275 ==================================================
#10 1.388 downloaded 703 KB
#10 1.388
#10 1.417      Imports
#10 1.417 [1,] "R6, methods, Matrix, stats, bit64, tiledb (>= 0.24.0), tiledb\n(<= 0.24.99), arrow, utils, fs, glue, urltools, Rcpp,\ndata.table, spdl, rlang, tools, tibble"
#10 DONE 1.5s

However, when installing this branch of tiledbsoma, R opts for 0.26.0 from CRAN; the double-pin also allows full loading/attachment of tiledbsoma despite the higher version of tiledb-r

#12 [9/9] RUN Rscript -e "library(tiledbsoma)"     -e "tiledbsoma::show_package_versions()"
#12 2.375 tiledbsoma:    1.8.1
#12 2.375 tiledb-r:      0.26.0
#12 2.375 tiledb core:   2.22.0
#12 2.375 libtiledbsoma: 2.20.1
#12 2.375 R:             R version 4.3.2 (2023-10-31)
#12 2.375 OS:            Ubuntu 22.04.3 LTS
#12 DONE 2.4s

Dockerfile with all steps is available as a gist; the build used was

docker build -t doublepin -f ./doublepin.Dockerfile --progress=plain .

[eddelbuettel]

@mojaveazure There is a difference in behavior between installing and loading / attaching. The effect of the dual inequality is more on the latter as it seems to have not effect on the former (which would be a 'nice to have' but it not implemented by R). It remains hard / difficult to 'hide' a higher version available at CRAN. But see eg my PR #2450 which uncorks the issue here (via explicit installation steps).

PS It may be possible to point install.packages() via the available= argument at a filtered list of available.packages() but I guess we would need to work out suitable filter (which, say, prefers a 'universe' source to a CRAN source?). Uncharted territory so far.

[johnkerl]

@eddelbuettel @mojaveazure I always appreciate your R-expert dialog!

As I, an R non-expert, am functioning in a clerical role on this PR -- @eddelbuettel and/or @mojaveazure please accept this PR as-is, or accept with proposed changes for a follow-up PR, or request changes on this PR with specific proposed changes -- thank you!

[eddelbuettel]

(Had this typed earlier an apparently never sent it / saved it. Sorry.)

@mojaveazure At the risk of drifting further 'inside baseball' you can look at R's own src/packages.R in base packages utils. I took the existing 'CRAN' filter (that makes a CRAN repo version win) and (brute force) added a 'but not for tiledb'. That works.

## This one a 'CRAN' filter in file src/packages.R in 'utils'
## So let's build not-CRAN one for tiledb
available_packages_filters_db_not_CRAN_tiledb <- function(db) {
    packages <- db[, "Package"]
    dups <- packages[duplicated(packages)]
    drop <- integer()
    CRAN <- getOption("repos")["CRAN"]
    ## do nothing if there is no CRAN repos on the list
    if(is.na(CRAN)) return(db)
    for(d in dups) {
        pos <- which(packages == d)
        pkg <- db[pos[1], "Package"]
        if (pkg == "tiledb") {
            ind <- startsWith(db[pos, "Repository"], CRAN)
            if(!all(ind)) drop <- c(drop, pos[ind])
        } else {
            ind <- !startsWith(db[pos, "Repository"], CRAN)
            if(!all(ind)) drop <- c(drop, pos[ind])
        }
    }
    if(length(drop)) db[-drop, , drop = FALSE] else db
}

and use it (note that it picks up the CRAN repo via the named entry in getOptions("repos") which is not terribly robust against an unnamed list of repo):

> options("repos" = c(univ="https://tiledb-inc.r-universe.dev", getOption("repos")))
> AP1 <- data.frame(available.packages(filters=list(available_packages_filters_db_not_CRAN)))
> AP1[with(AP1, Package=="tiledb"), c(1:2,17)]
       Package Version                                    Repository
tiledb  tiledb  0.25.0 https://tiledb-inc.r-universe.dev/src/contrib
>

[eddelbuettel]

@johnkerl @mojaveazure I suggest two more changes I can make if we're all on board with them.

[johnkerl]

@eddelbuettel @mojaveazure all requested changes have been made

[mojaveazure]

@eddelbuettel I showed in the Docker that the double-pin had no effect on loading/attaching. R was more than happy to load tiledbsoma with a version of tiledb-r that exceeded the pin. At this point, I'm not convinced that this PR will do anything to handle mismatches between tiledb-r and tiledbsoma

That being said, we could rig up something in .onLoad() to read DESCRIPTION and use the double-pin to do its own checking of tiledb-r (I would do it today, but I'm OOO)

I won't object to this PR being merged, I just wanted to flag that R doesn't seem to respect it 🤷

[eddelbuettel]

@mojaveazure We can take this off-line. Happy to discuss and dive further. install.packages() can use available.packages() via a filter as shown above, there is no claim that the dual inequality helps there per se as it appears to it other parts.

That being said, we could rig up something in .onLoad() to read DESCRIPTION and use the double-pin to do its own checking of tiledb-r (I would do it today, but I'm OOO)

That behavior appears to be part of R, it actually happened to me once during R CMD build or R CMD INSTALL and I will attempt to reproduce and crystalize.

I won't object to this PR being merged, I just wanted to flag that R doesn't seem to respect it 🤷

I believe there is a mismatch between what you expect to happen and what this is about. No more no less.

[eddelbuettel]

@johnkerl Before merging maybe renaming 'with bidirectional-pin' ? It's not actually bi-directional as I understand it as no information flows back,is there?

[johnkerl]

@mojaveazure I appreciate your concerns about correctness.

We do have:

• Green CI on this PR
• On the release-1.9 branch which needs TileDB-R 0.25 and core 2.21
• While TileDB-R 0.26.0 does exist and is available at CRAN
• Without the controlled-downgrade procedure used on the previous five releases: [r] Update DESCRIPTION and NEWS.md for 1.5.2 #1972 [python] Allow specification of X names on ingest [release-1.6] #1996 [r/ci] Controlled downgrade for TileDB-R 0.23 #2157 [r/ci] Controlled downgrade for TileDB-R 0.24 #2295 [r/ci] Controlled downgrade for TileDB-R 0.25 #2373

This means we do have correctness signal on this PR -- I believe this means we at least have break-even with #1972 #1996 #2157 #2295 #2373 -- not to say that more can't be done (and I note #2406 will eliminate this problem entirely)

I'll of course be watching everything very closely (as usual) during the 1.10 release process happening this week