Skip to content

Commit

Permalink
Add check for newer ABIs
Browse files Browse the repository at this point in the history 
Add check on package attachment to see if dependencies have udpated.
These updates may cause runtime binary breaks in SeuratObject, but still allow
SeuratObject to be loaded, so this check happens at attachment
  • Loading branch information
@mojaveazure
mojaveazure committed Sep 15, 2023
1 parent 200b59b commit d567ec8
Showing 1 changed file with 34 additions and 1 deletion.
35 changes: 34 additions & 1 deletion R/zzz.R
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#' @importFrom sp bbox over
#' @importFrom Rcpp evalCpp
#' @importFrom utils head tail
#' @importFrom utils head tail packageVersion
#' @importFrom methods new setClass setClassUnion setMethod setOldClass
#' setValidity slot slot<- validObject
#' @importClassesFrom Matrix dgCMatrix
Expand All @@ -24,6 +24,16 @@ Seurat.options <- list(
progressr.clear = FALSE
)


#%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
# Built With
#%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

.BuiltWith <- c(
R = format(x = getRversion()),
Matrix = format(x = packageVersion(pkg = "Matrix"))
)

#%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
# Reexports
#%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
Expand Down Expand Up @@ -465,5 +475,28 @@ NameIndex <- function(x, names, MARGIN) {
if (length(x = toset)) {
options(Seurat.options[toset])
}
for (i in names(x = .BuiltWith)) {
current <- switch(EXPR = i, R = getRversion(), packageVersion(pkg = i))
if (current > .BuiltWith[i]) {
msg <- paste(
sQuote(x = pkgname),
"was built",
switch(
EXPR = i,
R = "under R",
paste("with package", sQuote(x = i))
),
.BuiltWith[i],
"but the current version is",
paste0(current, ';'),
"it is recomended that you reinstall ",
sQuote(x = pkgname),
" as the ABI for",
switch(EXPR = i, R = i, sQuote(x = i)),
"may have changed"
)
packageStartupMessage(paste(strwrap(x = msg), collapse = '\n'))
}
}
return(invisible(x = NULL))
}

6 comments on commit d567ec8

@jaganmn
Copy link

@jaganmn jaganmn commented on d567ec8 Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit seems "wrong", from my perspective as a Matrix author.

User library trees are already by default partitioned by R's minor version. If I am running R x.y.z then I will never load packages installed under R a.b.c if x.y != a.b (unless I have somehow shot myself in the foot). Given that R does not introduce ABI changes in patch updates (x.y == a.b && z != c), is there a real reason to be testing for and warning about x.y.z > a.b.c?

I similarly do not see how SeuratObject can be affected by the Matrix ABI when it does not use LinkingTo: Matrix. Are you referring to Matrix class definitions cached in the SeuratObject namespace, which can become stale with Matrix updates?

intersect(grep("^.__C__", names(asNamespace("SeuratObject")), value = TRUE),
          getNamespaceExports("Matrix"))
## [1] ".__C__dsparseMatrix" ".__C__dMatrix"       ".__C__sparseMatrix" 
## [4] ".__C__generalMatrix" ".__C__compMatrix"

But that indicates that your NAMESPACE is missing importClassesFrom directives:

importClassesFrom(Matrix, dsparseMatrix, dMatrix, sparseMatrix, generalMatrix, compMatrix)

AFAIK, the only version mismatch that does affect SeuratObject is Matrix < 1.6-2 and Matrix >= 1.6-2, because Matrix 1.6-2 removes two unexported superclasses of dgCMatrix: mMatrix and xMatrix. Details about their definitions are unfortunately cached in, e.g., getClassDef("Graph")@contains[c("mMatrix", "xMatrix")], and errors arise when the actual definitions are not found in the Matrix namespace. But instead of testing for that discrepancy in .onAttach (why not .onLoad??), you should change to using Imports: Matrix (>= 1.6-2) once 1.6-2 is published by CRAN (today or tomorrow, probably).

@mojaveazure
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Mikael,

It very well could be wrong, but it was something simple to flag potential binary/cache incompatibilities

For the R test, we probably could get rid of it. However, R itself issues these warnings when loading a binary that was built using a newer version of R (eg. binary built under R 4.3.2 loaded under R 4.3.1), so I figured I'd do the reverse. I'm happy to remove it though

For the Matrix test, this one's important. I'm actually running into a case where the removal of the dCsparseMatrix class union from Matrix 1.6-2 is causing issues in SeuratObject binaries built with Matrix 1.6-1. Now, when trying to cast a Graph (inherits from dgCMatrix) to another matrix representation (eg. TsparseMatrix), we get errors. For example

> packageVersion("Matrix")
[1] ‘1.6.2’
> library(SeuratObject)
Loading required package: sp
‘SeuratObject’ was built with package ‘Matrix’
1.6.1 but the current version is 1.6.2; it is
recomended that you reinstall ‘SeuratObject’ as
the ABI for ‘Matrix’ may have changed

Attaching package: ‘SeuratObject’

The following object is masked from ‘package:base’:

    intersect

> data("pbmc_small")
> graph <- pbmc_small[["RNA_snn"]]
> class(graph)
[1] "Graph"
attr(,"package")
[1] "SeuratObject"
> as(graph, "TsparseMatrix")
Error in isVirtualExt(exti) : 
  trying to get slot "virtual" from an object of a basic class ("NULL") with no slots

Specifically, this is happening in methods::.selectSuperClasses() where it's trying to figure out if dCsparseMatrix is a virtual class (cached from Matrix 1.6-1.1, NULL under Matrix 1.6-2) or not

I haven't put the extra importClassesFrom in SeuratObject because the set of classes from Matrix seems to change frequently and it's an extra burden for me to maintain. I do bump the minimum required version of Matrix when SeuratObject gets an update, but don't regularly issue updates to SeuratObject that just bump the minimum required version of Matrix

Lastly, I put this in .onAttach() instead of .onLoad() because I'm issuing the note through packageStartupMessage() and I'm of the opinion that package startup messages should only be issued in .onAttach(); I'm debating on whether or not this note should be moved to a warning and/or moved to .onLoad()

If you have any suggestions for this test (which I do want to keep in some capacity), I'd be happy to incorporate them

@jaganmn
Copy link

@jaganmn jaganmn commented on d567ec8 Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between the classes that I listed in the importClassesFrom directive and the classes mMatrix, xMatrix, and (indeed, I'd forgotten to mention) dCsparseMatrix is that the former are exported by Matrix and the latter were not. Exported classes (just as exported functions) cannot be removed haphazardly. So the importClassesFrom directive that I proposed would not cause problems down the line. Actually, it would prevent problems, because class definitions that you import are never cached: they are retrieved from Matrix at load time, not at install time.

It is unfortunate that classes that we never exported (in this case dCsparseMatrix) were exposed to and inadvertently used by packages like SeuratObject. There were two ways for Matrix to overcome this limitation of S4:

  1. Export the classes and tell packages that had cached the definitions to import them.
  2. Remove the classes and tell packages that had cached the definitions to depend on the version of Matrix that removed them.

Option 2 was chosen because it required less maintenance for everyone involved in the long term; even Matrix itself was not using mMatrix, xMatrix, and dCsparseMatrix.

Anyway ...

Of course, the choice of whether/how to keep the test is yours. Ideally, you would make the test more specific, to save your users and the people who maintain binaries of your package (CRAN, Debian, r-universe, ...) considerable headache (see, e.g., the discussion here about TMB's similarly unspecific test). One way to do that is to warn only for mismatch involving versions of Matrix known to cause problems (see, e.g., this recent commit by the TMB maintainer). But if you already plan to change to Imports: Matrix (>= 1.6-2) in your next release, then the resulting more specific test would not test anything at all, as 1.6-2 is the only version since 1.6-1 (the version on which you currently depend) known to cause problems.

@mojaveazure
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So importing the classes you mentioned above doesn't fully solve the problem, at least as far as the end-user is concerned. I have a branch of SeuratObject where I import those classes. I then built a Docker image where I

  1. download the branch of SeuratObject with the updated class imports (import-classes.tar.gz)
  2. install Matrix 1.6-1
  3. install the branch of SeuratObject that has the updated imports
  4. update Matrix to 1.6-2
FROM rocker/r-ver:latest

RUN apt-get update && apt-get install -y wget && \
    wget https://github.com/mojaveazure/seurat-object/archive/refs/heads/fix/import-classes.tar.gz && \
    Rscript -e "install.packages('remotes')" \
        -e "remotes::install_version('Matrix', '1.6.1')" \
        -e "remotes::install_deps('import-classes.tar.gz', upgrade = FALSE)" && \
    R CMD INSTALL import-classes.tar.gz && \
    Rscript -e "install.packages('Matrix')"

Using this image (called objmat), I then ran the your intersect() command to ensure that no classes were being cached and my Graph -> TsparseMatrix conversion to see if I could trigger the error

$ docker run -it --rm objmat Rscript -e "intersect(grep('^.__C__', names(asNamespace('SeuratObject')), value = TRUE), getNamespaceExports('Matrix'))"
character(0)
$ docker run -it --rm objmat Rscript -e "library(SeuratObject); data('pbmc_small'); graph <- pbmc_small[['RNA_snn']]; as(graph, 'TsparseMatrix')"
Loading required package: sp
‘SeuratObject’ was built with package ‘Matrix’ 1.6.1 but the current
version is 1.6.2; it is recomended that you reinstall ‘SeuratObject’ as
the ABI for ‘Matrix’ may have changed

Attaching package: ‘SeuratObject’

The following object is masked from ‘package:base’:

    intersect

Error in getClassDef(x@superClass, package = packageSlot(x))@virtual :
  no applicable method for `@` applied to an object of class "NULL"
Calls: as ... .selectSuperClasses -> vapply -> FUN -> isVirtualExt
Execution halted

The internal class definition of dCsparseMatrix is still cached somewhere in SeuratObject. If I install Matrix 1.6-2, then SeuratObject (with or without the extra class imports), this error doesn't occur. Hopefully, if a user comes across an error like this, my note would have provided some guidance to the end-user about how to resolve the issue (reinstall SeuratObject from source)

I'm happy to keep the updated imports, but I'm also going to keep the non-specific test as this is not the first time something like this has happened to SeuratObject (Matrix 1.5-3 removed Csparse_validate(), which caused similar S4 caching issues). I haven't decided if I'll adopt TMB's approach of switching to warnings during .onLoad() or leaving it as a message in .onAttach() (I feel my approach is less intrusive, albeit easier to miss)

@jaganmn
Copy link

@jaganmn jaganmn commented on d567ec8 Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So importing the classes you mentioned above doesn't fully solve the problem, at least as far as the end-user is concerned.

Indeed. dCsparseMatrix was not exported by Matrix, and you cannot do importClassesFrom(Matrix, <unexported class>).

The internal class definition of dCsparseMatrix is still cached somewhere in SeuratObject. If I install Matrix 1.6-2, then SeuratObject (with or without the extra class imports), this error doesn't occur. Hopefully, if a user comes across an error like this, my note would have provided some guidance to the end-user about how to resolve the issue (reinstall SeuratObject from source)

Right. The test is useful for as long as you continue to use Imports: Matrix (>= 1.6-1) instead of Imports: Matrix (>= 1.6-2).

I'm happy to keep the updated imports, but I'm also going to keep the non-specific test as this is not the first time something like this has happened to SeuratObject (Matrix 1.5-3 removed Csparse_validate(), which caused similar S4 caching issues).

Yes, I remember vaguely. The underlying problem was that SeuratObject (at the time version 4.1.3) was missing importClassesFrom(Matrix, CsparseMatrix) in its NAMESPACE, so a stale CsparseMatrix definition was cached and used.

In general, I agree that Matrix needs to do a better job of detecting such quasi-breakage (quasi-, in the sense of requiring a re-installation rather than a patch) and reporting ahead of time to the respective maintainers. I am looking at ways to do so programmatically, so we can contact all affected maintainers ahead of our releases. In the mean time, it would be nice if the text in WRE about importClassesFrom would be adjusted to prevent situations like these. (I asked on R-devel a few months ago ...)

@mojaveazure
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next update to SeuratObject will have require Matrix (>= 1.6-2) (or newer if you release before I do) as my policy is to always require the newest version of Matrix whenever SeuratObject is updated, but I don't have an ETA on that right now. I'm also going to leave the test as-is: as it only occurs during .onAttach() and is signaled through packageStartupMessage(), it should be easy to suppress if it causes any trouble. I've added the additional class imports in the develop branch, so that will be part of the next release. If there's anything I can do to make SeuratObject a more reliable reverse dependency or report breakages due to S4 caching, please let me know

Please # to comment.