Skip to content

Commit ecab18f

Browse files
committed
Fix CI failures: zero-column assay coercion, hdf5r handle and reference bookkeeping
- SeuratObject's Assay <-> Assay5 coercions fail with "subscript out of bounds" under R >= 4.5 when the feature-level meta data has rows but no columns (a fresh assay without variable features, e.g. the RNA assay next to an SCT assay). upgradeSeurat()/downgradeSeurat() now pad such a table for the duration of the coercion and re-apply variable features explicitly. Regression test added; reproduced and verified on R 4.6.1. - The AnnData compat readers and the h5ad rewriter close every HDF5 handle as soon as it is no longer needed instead of leaving them for close_all() and GC finalizers. On Windows the CRAN hdf5r binary bundles HDF5 1.12.1, where closing a file walked element by element crashed the test runner; the on-disk rewrite tests carry the same guard test-cli-integration.R already uses for that library combination. - Legacy __categories object references are assembled from the object address on the file handle already held instead of hdf5r's create_reference(), whose temporary file handle corrupts hdf5r's reference bookkeeping after enough references (GC-timing dependent "r_count can never be more than 1 larger than h5_count"). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBoywjd4uCB2ADnBNzBpp9
1 parent 8195bef commit ecab18f

6 files changed

Lines changed: 180 additions & 30 deletions

File tree

‎NEWS.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ matching `Read10X_Image()`.
3838
`row`/`col`/`tissue` columns are kept in the image `misc` so a later
3939
downgrade restores them).
4040
- Both accept a file path (`.rds` / `.h5seurat`) plus `dest`.
41+
- Assays whose feature-level meta data has no columns (a fresh assay without
42+
variable features) convert in both directions: SeuratObject's own
43+
`Assay5 <-> Assay` coercions fail on such a table under R >= 4.5, so the
44+
conversion pads it for the duration of the coercion.
4145

4246
## Lossless h5ad layout upgrade / downgrade (`upgradeH5AD`, `downgradeH5AD`, `h5adLayout`)
4347

@@ -59,6 +63,13 @@ matching `Read10X_Image()`.
5963
directions. `strings = "categorical"` produces a file every anndata >= 0.8
6064
can read.
6165

66+
- The rewriter and the AnnData readers close every HDF5 handle as soon as it
67+
is no longer needed, and legacy `__categories` object references are built
68+
from the object address instead of hdf5r's `create_reference()`, whose
69+
temporary file handle corrupts hdf5r's reference bookkeeping after enough
70+
references (a GC-timing dependent "r_count can never be more than 1 larger
71+
than h5_count" error).
72+
6273
## AnnData cross-version compatibility (anndata 0.7 through 0.13)
6374

6475
Fixture files written by anndata 0.7.8, 0.8.0, 0.10.9, 0.11.4, 0.12.19 and

‎R/AnnDataCompat.R‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,32 @@ NULL
6565
#'
6666
#' @keywords internal
6767
#' @noRd
68+
#' Close an hdf5r object handle without failing
69+
#'
70+
#' Handles are closed as soon as they are no longer needed: hdf5r's
71+
#' \code{close_all()} and its GC finalizers misbehave on HDF5 1.12 (the
72+
#' library bundled by the CRAN Windows binary) when a file is closed while
73+
#' many child handles are still open.
74+
#'
75+
#' @keywords internal
76+
#' @noRd
77+
.h5_close_quietly <- function(obj) {
78+
if (!is.null(obj) && inherits(obj, "H5RefClass") && !inherits(obj, "H5File")) {
79+
tryCatch(obj$close(), error = function(e) NULL)
80+
}
81+
invisible(NULL)
82+
}
83+
84+
#' Open a child of an HDF5 group, apply a function and close the handle
85+
#'
86+
#' @keywords internal
87+
#' @noRd
88+
.h5_with_child <- function(parent, name, fn) {
89+
child <- parent[[name]]
90+
on.exit(.h5_close_quietly(child), add = TRUE)
91+
fn(child)
92+
}
93+
6894
.h5ad_as_logical <- function(x) {
6995
if (is.logical(x)) return(x)
7096
if (is.factor(x)) return(as.logical(toupper(as.character(x))))
@@ -89,8 +115,8 @@ NULL
89115
.h5ad_read_nullable <- function(grp) {
90116
if (!inherits(grp, "H5Group")) return(NULL)
91117
if (!(grp$exists("values") && grp$exists("mask"))) return(NULL)
92-
values <- grp[["values"]]$read()
93-
mask <- .h5ad_as_logical(grp[["mask"]]$read())
118+
values <- .h5_with_child(grp, "values", function(d) d$read())
119+
mask <- .h5ad_as_logical(.h5_with_child(grp, "mask", function(d) d$read()))
94120
enc <- .h5ad_encoding(grp)
95121
if (identical(enc, "nullable-boolean")) {
96122
values <- .h5ad_as_logical(values)
@@ -123,9 +149,11 @@ NULL
123149
.h5ad_read_categorical_group <- function(grp) {
124150
if (!inherits(grp, "H5Group")) return(NULL)
125151
if (!(grp$exists("codes") && grp$exists("categories"))) return(NULL)
126-
codes <- grp[["codes"]]$read()
152+
codes <- .h5_with_child(grp, "codes", function(d) d$read())
127153
categories <- .h5ad_read_string_like(grp, "categories")
128-
if (is.null(categories)) categories <- as.character(grp[["categories"]]$read())
154+
if (is.null(categories)) {
155+
categories <- as.character(.h5_with_child(grp, "categories", function(d) d$read()))
156+
}
129157
is_ordered <- isTRUE(.h5ad_as_logical(.h5ad_attr(grp, "ordered", FALSE))[1])
130158
.h5ad_decode_codes(as.integer(codes), as.character(categories), ordered = is_ordered)
131159
}
@@ -155,6 +183,7 @@ NULL
155183
.h5ad_read_string_like <- function(parent, name) {
156184
if (is.null(parent) || !parent$exists(name)) return(NULL)
157185
child <- parent[[name]]
186+
on.exit(.h5_close_quietly(child), add = TRUE)
158187
if (inherits(child, "H5D")) {
159188
return(as.character(child$read()))
160189
}
@@ -237,6 +266,7 @@ NULL
237266
.h5ad_read_column <- function(grp, col) {
238267
if (!grp$exists(col)) return(NULL)
239268
obj <- grp[[col]]
269+
on.exit(.h5_close_quietly(obj), add = TRUE)
240270
if (inherits(obj, "H5Group")) {
241271
vals <- .h5ad_read_nullable(obj)
242272
if (!is.null(vals)) return(vals)
@@ -247,9 +277,11 @@ NULL
247277
# Legacy (< 0.8) categorical: integer codes + __categories/<col>
248278
if (grp$exists("__categories")) {
249279
cats_grp <- grp[["__categories"]]
280+
on.exit(.h5_close_quietly(cats_grp), add = TRUE)
250281
if (cats_grp$exists(col)) {
251282
codes <- obj$read()
252283
cats_dset <- cats_grp[[col]]
284+
on.exit(.h5_close_quietly(cats_dset), add = TRUE)
253285
categories <- as.character(cats_dset$read())
254286
# anndata 0.7 stored booleans as categoricals with "False"/"True" categories
255287
if (length(categories) <= 2L && all(categories %in% c("False", "True"))) {
@@ -309,7 +341,11 @@ NULL
309341
if (inherits(obj, "H5D")) {
310342
enc <- .h5ad_encoding(obj)
311343
if (identical(enc, "null")) return(NULL)
312-
simple <- tryCatch(obj$get_space()$is_simple(), error = function(e) TRUE)
344+
simple <- tryCatch({
345+
sp <- obj$get_space()
346+
on.exit(.h5_close_quietly(sp), add = TRUE)
347+
sp$is_simple()
348+
}, error = function(e) TRUE)
313349
if (!isTRUE(simple)) return(NULL)
314350
vals <- obj$read()
315351
if (is.factor(vals) && all(levels(vals) %in% c("FALSE", "TRUE"))) {
@@ -328,7 +364,7 @@ NULL
328364
if (!is.null(cg)) return(cg)
329365
out <- list()
330366
for (nm in names(obj)) {
331-
out[nm] <- list(tryCatch(.h5ad_read_element(obj[[nm]]), error = function(e) NULL))
367+
out[nm] <- list(tryCatch(.h5_with_child(obj, nm, .h5ad_read_element), error = function(e) NULL))
332368
}
333369
out
334370
}
@@ -369,7 +405,7 @@ NULL
369405
# under __categories (this also restores the `ordered` flag).
370406
redo <- character(0)
371407
if (grp$exists("__categories")) {
372-
redo <- intersect(names(grp[["__categories"]]), wanted)
408+
redo <- intersect(.h5_with_child(grp, "__categories", names), wanted)
373409
cols[redo] <- NULL
374410
}
375411
for (col in setdiff(wanted, names(cols))) {

‎R/H5ADVersion.R‎

Lines changed: 61 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,26 @@ NULL
149149
dst_parent[[dst_name]]
150150
}
151151

152+
# Object reference to `obj` (an H5D / H5Group of `file`) for a
153+
# H5T_STD_REF_OBJ attribute. hdf5r's own `create_reference()` opens a
154+
# temporary file handle whose garbage-collection finalizer corrupts hdf5r's
155+
# reference bookkeeping ("r_count can never be more than 1 larger than
156+
# h5_count" after enough references, at GC-dependent moments). A classic
157+
# object reference is the object's header address, so it is assembled here
158+
# from `obj_info()` and attached to the file handle we already hold; the
159+
# caller closes the returned object right after writing the attribute.
160+
.h5w_object_reference <- function(file, obj) {
161+
addr <- as.numeric(obj$obj_info()$addr)
162+
bytes <- integer(8)
163+
for (i in seq_len(8)) {
164+
bytes[i] <- addr %% 256
165+
addr <- addr %/% 256
166+
}
167+
ref <- hdf5r::H5R_OBJECT$new(1, file)
168+
ref$ref <- as.raw(bytes)
169+
ref
170+
}
171+
152172
.h5_is_scalar <- function(dset) {
153173
isTRUE(tryCatch(dset$get_space()$get_simple_extent_type() == "H5S_SCALAR",
154174
error = function(e) FALSE)) ||
@@ -208,12 +228,15 @@ NULL
208228
!enc %in% .H5AD_MATRIX_ENCODINGS) {
209229
walk(obj, depth + 1L)
210230
}
231+
.h5_close_quietly(obj)
211232
}
212233
}
213234
walk(h5, 0L)
214235
root_enc <- .h5ad_encoding(h5)
215-
obs_is_group <- h5$exists("obs") && inherits(h5[["obs"]], "H5Group")
216-
obs_df_version <- if (obs_is_group) as.character(.h5ad_attr(h5[["obs"]], "encoding-version", "")) else ""
236+
obs_is_group <- h5$exists("obs") && .h5_with_child(h5, "obs", function(o) inherits(o, "H5Group"))
237+
obs_df_version <- if (obs_is_group) {
238+
as.character(.h5_with_child(h5, "obs", function(o) .h5ad_attr(o, "encoding-version", "")))
239+
} else ""
217240
layout <- if (compound) {
218241
"compound"
219242
} else if (legacy_cats || identical(obs_df_version, "0.1.0") ||
@@ -222,7 +245,7 @@ NULL
222245
} else {
223246
"encoded"
224247
}
225-
has_manifest <- h5$exists("uns") && h5[["uns"]]$exists(.H5AD_MANIFEST)
248+
has_manifest <- h5$exists("uns") && .h5_with_child(h5, "uns", function(u) u$exists(.H5AD_MANIFEST))
226249
has_ns <- "nullable-string-array" %in% encs
227250
has_nb <- any(c("nullable-integer", "nullable-boolean") %in% encs)
228251
has_null <- "null" %in% encs
@@ -448,18 +471,22 @@ print.h5ad_layout <- function(x, ...) {
448471
# recorded in the manifest so an upgrade can restore it.
449472
.h5ad_write_column_legacy <- function(g, col, x, src_enc, path, ctx, na_note = "") {
450473
write_cat <- function(f, note = "") {
451-
if (!g$exists("__categories")) g$create_group("__categories")
474+
if (!g$exists("__categories")) .h5_close_quietly(g$create_group("__categories"))
452475
cg <- g[["__categories"]]
476+
on.exit(.h5_close_quietly(cg), add = TRUE)
453477
lv <- levels(f)
454478
cd <- .h5w_dataset(cg, col, lv)
479+
on.exit(.h5_close_quietly(cd), add = TRUE)
455480
.h5w_attr(cd, "ordered", is.ordered(f))
456481
codes <- as.integer(f) - 1L
457482
codes[is.na(codes)] <- -1L
458483
d <- .h5w_dataset(g, col, codes, dtype = .h5w_codes_dtype(length(lv)))
459-
ref <- cg$create_reference(col)
484+
on.exit(.h5_close_quietly(d), add = TRUE)
485+
ref <- .h5w_object_reference(ctx$dst_file, cd)
486+
on.exit(.h5_close_quietly(ref), add = TRUE)
460487
d$create_attr(attr_name = "categories", robj = ref,
461488
dtype = hdf5r::h5types$H5T_STD_REF_OBJ, space = .h5w_scalar_space())
462-
invisible(d)
489+
invisible(NULL)
463490
}
464491
if (is.factor(x)) {
465492
if (src_enc %in% c("nullable-string-array", "string-array")) {
@@ -501,6 +528,10 @@ print.h5ad_layout <- function(x, ...) {
501528
#' @noRd
502529
.h5ad_write_dataframe <- function(src, name, dst_parent, path, ctx) {
503530
obj <- src[[name]]
531+
on.exit(.h5_close_quietly(obj), add = TRUE)
532+
child_attr <- function(cn, attr, default) {
533+
.h5_with_child(obj, cn, function(o) as.character(.h5ad_attr(o, attr, default)))
534+
}
504535
# ---- decode
505536
if (inherits(obj, "H5D")) {
506537
# anndata < 0.7: single compound dataset. Categories (if any) live in
@@ -571,12 +602,12 @@ print.h5ad_layout <- function(x, ...) {
571602
}
572603
na_value <- rec$note %||% ""
573604
if (!nzchar(na_value) && inherits(obj, "H5Group") && obj$exists(cn)) {
574-
na_value <- as.character(.h5ad_attr(obj[[cn]], "na-value", "NaN"))
605+
na_value <- child_attr(cn, "na-value", "NaN")
575606
}
576607
.h5ad_write_column_encoded(g, cn, v, want, ctx, na_value = na_value)
577608
} else {
578609
na_note <- if (inherits(obj, "H5Group") && obj$exists(cn)) {
579-
as.character(.h5ad_attr(obj[[cn]], "na-value", ""))
610+
child_attr(cn, "na-value", "")
580611
} else ""
581612
.h5ad_write_column_legacy(g, cn, v, src_encs[[cn]], cpath, ctx, na_note = na_note)
582613
}
@@ -590,7 +621,7 @@ print.h5ad_layout <- function(x, ...) {
590621
if (want == "nullable-string-array" && !identical(ctx$strings, "categorical")) {
591622
na_value <- irec$note %||% ""
592623
if (!nzchar(na_value) && inherits(obj, "H5Group") && obj$exists(index_name)) {
593-
na_value <- as.character(.h5ad_attr(obj[[index_name]], "na-value", "NaN"))
624+
na_value <- child_attr(index_name, "na-value", "NaN")
594625
}
595626
.h5ad_write_nullable_encoded(g, index_name, idx, "nullable-string-array",
596627
na_value = na_value)
@@ -602,21 +633,23 @@ print.h5ad_layout <- function(x, ...) {
602633
} else {
603634
if (idx_enc == "nullable-string-array") {
604635
.ctx_record(ctx, ipath, idx_enc, "0.1.0",
605-
note = as.character(.h5ad_attr(obj[[index_name]], "na-value", "")))
636+
note = child_attr(index_name, "na-value", ""))
606637
}
607638
if (anyNA(idx)) idx[is.na(idx)] <- ""
608639
.h5w_dataset(g, index_name, idx)
609640
.h5w_encoding(g, "dataframe", "0.1.0")
610641
}
611642
.h5w_attr(g, "_index", index_name)
612643
.h5w_attr(g, "column-order", keep, force_array = TRUE)
613-
invisible(g)
644+
.h5_close_quietly(g)
645+
invisible(NULL)
614646
}
615647

616648
# ---- generic element rewriter -----------------------------------------------
617649

618650
.h5ad_rewrite_dataset <- function(src, name, dst, path, ctx) {
619651
obj <- src[[name]]
652+
on.exit(.h5_close_quietly(obj), add = TRUE)
620653
enc <- .h5ad_encoding(obj)
621654
if (identical(enc, "null") || .h5_is_null_space(obj)) {
622655
# anndata < 0.9 has no `null` encoding: drop it for the legacy layout and
@@ -627,9 +660,11 @@ print.h5ad_layout <- function(x, ...) {
627660
}
628661
d <- .h5w_copy(src, name, dst)
629662
.h5w_encoding(d, "null", "0.1.0")
630-
return(invisible(d))
663+
.h5_close_quietly(d)
664+
return(invisible(NULL))
631665
}
632666
d <- .h5w_copy(src, name, dst)
667+
on.exit(.h5_close_quietly(d), add = TRUE)
633668
if (ctx$target == "encoded") {
634669
if (!nzchar(enc) || enc == "array" || enc == "string-array" ||
635670
enc == "numeric-scalar" || enc == "string" || enc == "bytes") {
@@ -644,11 +679,12 @@ print.h5ad_layout <- function(x, ...) {
644679
} else {
645680
.h5w_strip_encoding(d)
646681
}
647-
invisible(d)
682+
invisible(NULL)
648683
}
649684

650685
.h5ad_rewrite_group <- function(src, name, dst, path, ctx, role = "generic") {
651686
obj <- src[[name]]
687+
on.exit(.h5_close_quietly(obj), add = TRUE)
652688
enc <- .h5ad_encoding(obj)
653689
if (identical(name, .H5AD_MANIFEST) && role == "uns") {
654690
return(invisible(NULL)) # never carry a stale manifest forward
@@ -657,20 +693,22 @@ print.h5ad_layout <- function(x, ...) {
657693
if (enc %in% .H5AD_MATRIX_ENCODINGS ||
658694
(obj$exists("data") && obj$exists("indices") && obj$exists("indptr"))) {
659695
g <- .h5w_copy(src, name, dst)
696+
on.exit(.h5_close_quietly(g), add = TRUE)
660697
if (!nzchar(enc)) enc <- "csr_matrix"
661698
.h5w_encoding(g, enc, "0.1.0")
662699
if (!isTRUE(g$attr_exists("shape")) && ctx$target == "encoded") {
663700
# shape is required by anndata >= 0.8; derive it from indptr/indices
664-
indptr <- obj[["indptr"]]$read()
665-
indices <- obj[["indices"]]$read()
701+
indptr <- .h5_with_child(obj, "indptr", function(d) d$read())
702+
indices <- .h5_with_child(obj, "indices", function(d) d$read())
666703
major <- length(indptr) - 1L
667704
minor <- if (length(indices)) max(indices) + 1L else 0L
668705
.h5w_attr(g, "shape", if (enc == "csc_matrix") c(minor, major) else c(major, minor))
669706
}
670-
return(invisible(g))
707+
return(invisible(NULL))
671708
}
672709
if (enc %in% .H5AD_OPAQUE_ENCODINGS) {
673-
return(invisible(.h5w_copy(src, name, dst)))
710+
.h5_close_quietly(.h5w_copy(src, name, dst))
711+
return(invisible(NULL))
674712
}
675713
# Dataframes
676714
is_df <- identical(enc, "dataframe") || role %in% c("obs", "var") ||
@@ -707,13 +745,14 @@ print.h5ad_layout <- function(x, ...) {
707745
}
708746
# Plain (dict) group: recurse
709747
g <- dst$create_group(name)
748+
on.exit(.h5_close_quietly(g), add = TRUE)
710749
child_role <- if (role == "raw") "raw-child" else if (role == "uns") "uns" else "generic"
711750
for (child in names(obj)) {
712751
cpath <- paste0(path, "/", child)
713-
cobj <- obj[[child]]
752+
child_is_dataset <- .h5_with_child(obj, child, function(o) inherits(o, "H5D"))
714753
r <- child_role
715754
if (role == "raw" && child == "var") r <- "var"
716-
if (inherits(cobj, "H5D")) {
755+
if (child_is_dataset) {
717756
.h5ad_rewrite_dataset(obj, child, g, cpath, ctx)
718757
} else {
719758
.h5ad_rewrite_group(obj, child, g, cpath, ctx, role = r)
@@ -724,7 +763,7 @@ print.h5ad_layout <- function(x, ...) {
724763
} else {
725764
.h5w_strip_encoding(g)
726765
}
727-
invisible(g)
766+
invisible(NULL)
728767
}
729768

730769
#' Rewrite an h5ad file into another on-disk layout
@@ -775,6 +814,7 @@ print.h5ad_layout <- function(x, ...) {
775814
ctx$source_layout <- info$layout
776815
ctx$writer <- pkg
777816
ctx$writer_version <- tryCatch(as.character(utils::packageVersion(pkg)), error = function(e) "")
817+
ctx$dst_file <- dst
778818
ctx$src_uns_exists <- function(key) src$exists("uns") && src[["uns"]]$exists(key)
779819
ctx$src_uns_read <- function(key) src[["uns"]][[key]]$read()
780820

@@ -787,7 +827,7 @@ print.h5ad_layout <- function(x, ...) {
787827
role <- roles[nm] %||% "generic"
788828
if (is.na(role)) role <- "generic"
789829
if (verbose) message(" ", nm)
790-
if (inherits(src[[nm]], "H5D")) {
830+
if (.h5_with_child(src, nm, function(o) inherits(o, "H5D"))) {
791831
if (nm %in% c("obs", "var")) {
792832
.h5ad_write_dataframe(src, nm, dst, nm, ctx)
793833
} else {

0 commit comments

Comments
 (0)