Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve Matrix-ABI documentation #768

Open
bbolker opened this issue Feb 12, 2024 · 15 comments
Open

improve Matrix-ABI documentation #768

bbolker opened this issue Feb 12, 2024 · 15 comments

Comments

@bbolker
Copy link
Member

bbolker commented Feb 12, 2024

From @jaganmn:

We are preparing Matrix version 1.7-0 for CRAN submission on March 11, ahead
of the spring release of R version 4.4.0.

We should be ready/get out in front of this.

@jaganmn
Copy link
Contributor

jaganmn commented Feb 13, 2024

FWIW, the glmmTMB machinery seems a bit overcomplicated. Why not put

.build.versions <- sapply(c("Matrix", "TMB"), utils::packageVersion, simplify = FALSE)

in zzz.R (outside of .onLoad) and be done with it? Involving Makevars and files under inst/ doesn't seem necessary, but maybe there are nuances that I'm not seeing ...

@bbolker
Copy link
Member Author

bbolker commented Feb 28, 2024

Just to be clear: we should plan to send an updated lme4 to CRAN as soon as possible after Matrix is posted, right? (So that the binaries get rebuilt with the new version of Matrix)?

The reason for all of the Makevars/inst stuff is that we typically need to know the version of Matrix etc. that the package was built with, in order to compare it to the versions that are currently being loaded; your .buildversions() will only tell us the latter ... am I missing something?

@jaganmn
Copy link
Contributor

jaganmn commented Feb 28, 2024

Just to be clear: we should plan to send an updated lme4 to CRAN as soon as possible after Matrix is posted, right? (So that the binaries get rebuilt with the new version of Matrix)?

Yes, though we will ask CRAN to rebuild lme4 anyway. The version bump will primarily serve as a nudge for repositories other than CRAN.

The reason for all of the Makevars/inst stuff is that we typically need to know the version of Matrix etc. that the package was built with, in order to compare it to the versions that are currently being loaded; your .buildversions() will only tell us the latter ... am I missing something?

At install time, files R/*.R are parsed, parsed expressions are evaluated in an empty namespace, and namespace objects are serialized in glmmTMB.rdb. So if you put

.build.versions <- sapply(c("Matrix", "TMB"), utils::packageVersion, simplify = FALSE)

in any R/*.R and install glmmTMB, then glmmTMB:::.build.versions will give the result of the sapply call at install time. It is no different from assignments of the form <name> <- function(...) in R/*.R.

@jaganmn
Copy link
Contributor

jaganmn commented Feb 28, 2024

Then your .onLoad would look something like:

.onLoad <- function(libname, pkgname) {
    ## do stuff
    for (p in names(.build.versions))
        if (utils::packageVersion(p) != .build.versions[[p]])
            ## complain
    ## do stuff
}

where utils::packageVersion(p) and .build.versions[[p]] are the load and install time versions of package p.

(Well, for Matrix you want to be storing and comparing ABI versions, not package versions. Anyway ...)

@bbolker
Copy link
Member Author

bbolker commented Mar 4, 2024

preliminary attempt: 8be641b (eventually I will try to back-port this to glmmTMB, where we'll have to check both Matrix and TMB -- that's why the checking function is a little more general than it needs to be)

@jaganmn
Copy link
Contributor

jaganmn commented Mar 4, 2024

Looks OK modulo my comment on the commit. Probably worth testing locally (if you haven't already) by installing lme4 under Matrix < 1.6-2, installing Matrix >= 1.6-2, and loading lme4.

@jaganmn
Copy link
Contributor

jaganmn commented Mar 17, 2024

BTW: It was decided that Matrix 1.7-0 should be released with Depends: R (>= 4.4.0) and so not until April. That means that users who do not manually set R_LIBS_USER will be more protected from having binary incompatible installations of Matrix and lme4 (because the default value of R_LIBS_USER contains R's x.y version number). But people will still find ways to defy my expectations, I'm sure ...

@bbolker
Copy link
Member Author

bbolker commented Mar 18, 2024

As you can see from this thread on r-package-devel, this is already causing minor headaches - 1.7.0 has been installed on win-builder without bumping the binary-dependent packages ... sigh ...

@jaganmn
Copy link
Contributor

jaganmn commented Mar 21, 2024

It was wrong of me to say "not until April" because binary repositories have already published contrib directories for R 4.4. That means that Matrix 1.7-0 binaries would become available to users of R-devel even before April 24.

Martin submitted 1.7-0 to CRAN today, and the incoming checks just finished, so people out there could start seeing 1.7-0 binaries soon-ish. I can ping here when 1.7-0 is actually published. At any rate, an lme4 submission in the next week or so would make sense ...

@bbolker
Copy link
Member Author

bbolker commented Mar 23, 2024

What are the risks that the releases won't be properly synchronized, i.e. that lme4 will hit CRAN before the Matrix 1.7.0 ... ?

@jaganmn
Copy link
Contributor

jaganmn commented Mar 23, 2024

Nonzero unless you change to Depends: R (>= 4.4.0), LinkingTo: Matrix (>= 1.7-0). You can ask CRAN to make sure that the sequence is correct, but other repositories won't know to do the same. I guess you could monitor the contrib/4.4 directories of non-CRAN repositories and submit only after those finally publish 1.7-0 ...

@bbolker
Copy link
Member Author

bbolker commented Mar 27, 2024

Ugh. I think this is upon us now. I asked CRAN maintainers for advice about staging releases and got the following:

The only way you can ensure that lme4 is built against Matrix 1.7.0 is for it to depend on Matrix (>= 1.7.0). And as that is only available for R-devel, you also need to specify R (>= 4.4) and CRAN will need to put the new lme4 version in 4.4.0/Other until 4.4.0 is released.

I have a pretty strong preference not to make lme4 dependent on R >= 4.4 ... although I see that Matrix 1.7.0 depends on R >= 4.4 (why?)

(It is not just binary packages -- end users installing from source are also affected by the segfaults from lme4 built against 1.6.5 but used with 1.7.0. Including CRAN members ....)

I don't really understand this point — I guess the point is that lme4 has to be re-installed from source (but, I'm much more worried about supporting users who get binaries from other repositories/aren't up to installing compilation tools and re-installing from source themselves ..)

FWIW we seem to have builds of Matrix 1.7.0 as part of R-devel builds for both Windows and macOS. Windows has re-built lme4: macOS has not and offers binary packages of Matrix 1.6.5.

I'm really not sure what to do next. I guess sending a new version of lme4 now will make sure that the Bioconductor/etc. builds get updated (since Matrix 1.7.0 is already there), and Matrix 1.7.0 will never be released for R 4.3.0 ...

Any thoughts or clarifications are welcome.

@jaganmn
Copy link
Contributor

jaganmn commented Mar 28, 2024

I have a pretty strong preference not to make lme4 dependent on R >= 4.4 ... although I see that Matrix 1.7.0 depends on R >= 4.4 (why?)

Partly to mitigate the fallout of the ABI breakage for most users who will not see R 4.4 until after April 24. Partly because we plan to use BLAS and LAPACK routines not exposed in the R API until R 4.4.

I'm really not sure what to do next. I guess sending a new version of lme4 now will make sure that the Bioconductor/etc. builds get updated (since Matrix 1.7.0 is already there), and Matrix 1.7.0 will never be released for R 4.3.0 ...

You can cross your fingers and submit lme4 now without the stricter dependencies, with the reasonable hope that any repository building binaries for R 4.4 before April 24 will do so using the latest version of R-devel, including the latest versions of all recommended packages. Or you can withhold the submission until after April 24 and deal will the fallout in the mean time (e.g., #775). Or you can submit now with the stricter dependencies, which is maybe CRAN's preference ...

I understand not wanting to bar for "no reason" users of recent/old versions of R from new/future versions of lme4. There actually is a reason for Matrix, which is that new versions of Matrix will use R 4.4 API.

Martin may have more/other thoughts but he is on vacation.

@bbolker
Copy link
Member Author

bbolker commented Mar 28, 2024

I think I'm going to cross my fingers and submit. Thanks for the input.

@bbolker
Copy link
Member Author

bbolker commented Mar 28, 2024

New version on CRAN now, should propagate.

Never did fix these things:

And, I think I'm going to leave this open for now because I think we could still do better a documenting - i.e., collect the information about when/why these updates are needed, give the clearest possible description of the solutions (install from r-universe? CRAN?), and put this on a man page that the warning message points to ...

@bbolker bbolker changed the title upcoming Matrix ABI change improve Matrix-ABI documentation Mar 28, 2024
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

No branches or pull requests

2 participants