From b383e1376035bf5ea9ab753d2d25c005d105ce72 Mon Sep 17 00:00:00 2001 From: Alex Bates Date: Sat, 22 Aug 2026 23:32:36 -0400 Subject: [PATCH 1/2] Delegate row writes to seatabler 0.2.0 seatabler 0.2.0 added seatable_update_rows() and seatable_append_rows(), the generics banctable_update_rows() / banctable_append_rows() were waiting on. Both now delegate, dropping another ~215 lines. Names, signatures and defaults are unchanged, including append_allowed = FALSE and bigdata = FALSE. Multiple-select columns are now detected from the table schema by seatabler rather than comma-split by hand here, which also fixes a latent bug: a multi-select value containing a comma used to be split into two options. banctable_query() deliberately keeps bancr's own row conversion. seatable_query() does not apply column types, so delegating it today would change 11 of 163 column classes on banc_meta: status and cell_type_source become list columns, _ctime/_mtime lose POSIXct, and two number columns arrive as character. Reported as flyconnectome/seatabler#9; the switch waits on that. Adds a test that reaches seatable_module() and banctable_login(). bancr's suite previously passed even with a seatabler that could not reach its Python dependencies, because nothing exercised that path -- a stale nat.python broke every SeaTable call while the tests stayed green. Round-tripped against a throwaway base on cloud.seatable.io: append three rows, update them, delete them, back to the starting row count with no leftovers. Nothing was written to banc_meta or cns_meta. Full suite 23 pass, 0 fail. Claude-Session: https://claude.ai/code/session_01TQHvpdwd2XCozxU6bKhRWQ --- DESCRIPTION | 2 +- NEWS.md | 17 +++ R/banc-table.R | 254 +++----------------------------- R/seatabler.R | 9 +- tests/testthat/test-seatabler.R | 29 ++++ 5 files changed, 69 insertions(+), 242 deletions(-) create mode 100644 tests/testthat/test-seatabler.R diff --git a/DESCRIPTION b/DESCRIPTION index 3b89386..336daad 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Type: Package Package: bancr Title: R Client Access to the Brain And Nerve Cord (BANC) Dataset -Version: 0.3.7 +Version: 0.3.8 Authors@R: c(person(given = "Alexander", family = "Bates", diff --git a/NEWS.md b/NEWS.md index 4f0f5c5..8ba7756 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,20 @@ +# bancr 0.3.8 (development) + +* `banctable_update_rows()` and `banctable_append_rows()` now delegate to + seatabler's `seatable_update_rows()` / `seatable_append_rows()`, which arrived + in seatabler 0.2.0. Names, signatures and defaults are unchanged, including + `append_allowed = FALSE` and `bigdata = FALSE`. +* Multiple-select columns are now detected from the table schema by seatabler + rather than being comma-split by hand in bancr, so a value containing a comma + is no longer silently broken into two options. +* New test covering the SeaTable Python path. bancr's suite previously passed + even when seatabler could not reach its Python dependencies, because nothing + exercised that path. +* `banctable_query()` still uses bancr's own row conversion. Delegating it would + change 11 column types, including `status` and `cell_type_source` becoming + list columns and `_ctime`/`_mtime` losing `POSIXct`; see + flyconnectome/seatabler#9. + # bancr 0.3.7 (development) * bancr now depends on [seatabler](https://github.com/flyconnectome/seatabler), diff --git a/R/banc-table.R b/R/banc-table.R index 5c7ab80..3b44bf0 100644 --- a/R/banc-table.R +++ b/R/banc-table.R @@ -248,54 +248,14 @@ banctable_update_rows <- function (df, workspace_id = "57832", token_name = "BANCTABLE_TOKEN", ...) { - df <- as.data.frame(df) - if (is.character(base) || is.null(base)) - base = banctable_base(base_name = base, table = table, workspace_id = workspace_id, token_name = token_name) - nx <- nrow(df) - if (!isTRUE(nx > 0)) { - warning("No rows to update in `df`!") - return(TRUE) - } - tablecols = fafbseg::flytable_columns(table,base) - df = fafbseg:::df2flytable(df, append = ifelse(append_allowed, NA,FALSE)) - newrows = is.na(df[["row_id"]]) - if (any(newrows)) { - stop("Adding new rows not yet implemented") - banctable_append_rows(df[newrows, , drop = FALSE], table = table, - base = base, chunksize = chunksize, ...) - df = df[!newrows, , drop = FALSE] - nx = nrow(df) - } - if (!isTRUE(nx > 0)) - return(TRUE) - if (nx > chunksize) { - nchunks = ceiling(nx/chunksize) - chunkids = rep(seq_len(nchunks), rep(chunksize, nchunks))[seq_len(nx)] - chunks = split(df, chunkids) - if (!requireNamespace("pbapply", quietly = TRUE)) { - stop("Package pbapply is required for this function. Please install it with: install.packages('pbapply')") - } - oks = pbapply::pbsapply(chunks, banctable_update_rows, - table = table, base = base, chunksize = Inf, append_allowed = FALSE, - ...) - return(all(oks)) - } - multi = tablecols$name[tablecols$type=="multiple-select"] - if(length(multi)){ - i = intersect(colnames(df),multi) - if(length(i)){ - for(j in i){ - df[[j]][is.na(df[[j]])] = '' - l = sapply(df[[j]], strsplit, split = ",|, ") - l = unname(l) - df[[j]] = l - } - } - } - pyl = banc_df2updatepayload(df, via_json = TRUE) - res = base$batch_update_rows(table_name = table, rows_data = pyl) - ok = isTRUE(all.equal(res, list(success = TRUE))) - return(ok) + con <- banc_seatable_connection(token_name = token_name, + workspace_id = workspace_id) + # seatabler detects multiple-select columns from the table schema and routes + # them through its own list-per-cell path, so the comma-splitting bancr used + # to do by hand is no longer needed here. + seatabler::seatable_update_rows(df = df, table = table, base = base, + con = con, append_allowed = append_allowed, + chunksize = chunksize, ...) } # hidden @@ -529,198 +489,18 @@ banctable_append_rows <- function (df, workspace_id = "57832", token_name = "BANCTABLE_TOKEN", ...) { - if (is.character(base) || is.null(base)){ - base <- banctable_base(base_name = base, table = table, workspace_id = workspace_id, token_name = token_name) - } - nx = nrow(df) - if (!isTRUE(nx > 0)) { - warning("No rows to append in `df`!") - return(TRUE) - } - df = fafbseg:::df2flytable(df, append = TRUE) - if (nx > chunksize) { - nchunks = ceiling(nx/chunksize) - chunkids = rep(seq_len(nchunks), rep(chunksize, nchunks))[seq_len(nx)] - chunks = split(df, chunkids) - if (!requireNamespace("pbapply", quietly = TRUE)) { - stop("Package pbapply is required for this function. Please install it with: install.packages('pbapply')") - } - oks = pbapply::pbsapply(chunks, banctable_append_rows, - table = table, base = base, chunksize = Inf, bigdata = bigdata, - ...) - return(all(oks)) - } - pyl = fafbseg:::df2appendpayload(df) - if(!bigdata){ - res = base$batch_append_rows(table_name = table, rows_data = pyl) - ok = isTRUE(all.equal(res[["inserted_row_count"]], nx)) - return(ok) - }else{ - # The big data backend has no SDK method, so it goes over the API gateway. - # seatabler::seatable_base_rest() supplies the base JWT, the HTTP/1.1 pin - # and the NA -> null encoding that endpoint needs. - result <- seatabler::seatable_base_rest( - "add-archived-rows/", base = base, - con = banc_seatable_connection(token_name = token_name, - workspace_id = workspace_id), - method = "POST", - body = list(table_name = table, rows = reticulate::py_to_r(pyl))) - return(isTRUE(result$success == TRUE)) - } -} - -# modified to enable list uploads to multi-select columns -banc_df2updatepayload <- function(x, via_json = TRUE){ - if (via_json) { - othercols <- setdiff(colnames(x), "row_id") - listcols <- names(x)[sapply(x, is.list)] - listcols <- intersect(othercols, listcols) - updates <- list() - for(i in 1:nrow(x)){ - updates[[i]] <- list(row_id = x[i, "row_id"], row = as.list(x[i,othercols, drop = FALSE])) - for(col in listcols){ - if(length((x[i,][[col]][[1]]))==1){ - updates[[i]]$row[[col]] <- x[i,][[col]] - }else{ - updates[[i]]$row[[col]] <- x[i,][[col]][[1]] - } - } - } - js <- jsonlite::toJSON(updates, auto_unbox = TRUE, na = "null") - pyjson <- reticulate::import("json") - pyl <- reticulate::py_call(pyjson$loads, js) - return(pyl) - } - pdf = reticulate::r_to_py(x) - pyfun = fafbseg:::df2updatepayload_py() - reticulate::py_call(pyfun$pdf2list, pdf) + con <- banc_seatable_connection(token_name = token_name, + workspace_id = workspace_id) + # bigdata rows go to the api-gateway add-archived-rows endpoint; seatabler + # handles both that and the ordinary SDK path. + seatabler::seatable_append_rows(df = df, table = table, base = base, + con = con, bigdata = bigdata, + chunksize = chunksize, ...) } -# banctable_columns(), banctable_add_column(), banctable_add_columns() and -# banctable_delete_column() now live in R/seatabler.R, wrapping their -# seatabler::seatable_* equivalents. - -# hidden -# Update SeaTable columns for neurons selected in a Neuroglancer scene. -# -# Takes a Neuroglancer short URL, extracts the root IDs from the -# "segmentation proofreading" layer, shows the current SeaTable values -# for the target columns, asks for confirmation, then updates. -# -# @param url A Neuroglancer short URL. -# @param entries Character vector of "column:value" pairs, e.g. -# \code{c("cell_type:DNa01", "super_class:descending")}. -# @param layer Neuroglancer layer to extract IDs from. -# @param update.ids If TRUE, run \code{banc_latestid} on the IDs first. -# @param table,base,workspace_id,token_name SeaTable connection arguments -# (defaults match \code{banctable_query}). -banctable_ngl_update <- function(url, - entries, - layer = "segmentation proofreading", - update.ids = FALSE, - table = "banc_meta", - base = NULL, - workspace_id = "57832", - token_name = "BANCTABLE_TOKEN") { - # Parse entries: "column:value" format - if (!is.character(entries) || !length(entries)) - stop("'entries' must be a character vector of 'column:value' pairs") - has_colon <- grepl(":", entries, fixed = TRUE) - if (any(!has_colon)) - stop("Invalid entries (missing ':'): ", - paste(entries[!has_colon], collapse = ", "), - "\n Expected format: c(\"cell_type:DNa01\", \"super_class:descending\")") - cols <- sub(":.*", "", entries) - vals <- sub("^[^:]*:", "", entries) - - # Validate column names against SeaTable schema - col_info <- banctable_columns(table = table, base = base, - workspace_id = workspace_id, - token_name = token_name) - bad_cols <- setdiff(cols, col_info$name) - if (length(bad_cols)) - stop("Invalid column name(s): ", paste(bad_cols, collapse = ", "), - "\n Available: ", paste(col_info$name, collapse = ", ")) - - # Decode neuroglancer state from short URL - url2 <- sub("#!middleauth+", "?", url, fixed = TRUE) - parts <- unlist(strsplit(url2, "?", fixed = TRUE)) - json <- fafbseg::flywire_fetch(parts[2], token = banc_token(), - return = "text", cache = TRUE) - sc <- fafbseg::ngl_decode_scene( - safe_ngl_encode_url(json, baseurl = parts[1])) - - # Find the target layer and extract selected segments - layers <- fafbseg::ngl_layers(sc) - nls <- fafbseg:::ngl_layer_summary(layers) - sel <- match(layer, nls$name) - if (is.na(sel)) - stop("Layer '", layer, "' not found. Available: ", - paste(nls$name, collapse = ", ")) - ids <- sc[["layers"]][[sel]][["segments"]] - ids <- as.character(ids) - ids <- ids[nzchar(ids) & ids != "0"] - message(sprintf("Found %d root IDs in layer '%s'", length(ids), layer)) - if (!length(ids)) { - message("Nothing to update.") - return(invisible(NULL)) - } - - # Optionally update to latest root IDs - if (update.ids) { - message("Updating root IDs to latest...") - ids <- banc_latestid(ids) - ids <- as.character(ids) - ids <- ids[nzchar(ids) & ids != "0"] - message(sprintf(" %d root IDs after update", length(ids))) - } - - # Look up matched rows in SeaTable, including target columns - select_cols <- unique(c("_id", "root_id", cols)) - bt <- banctable_query( - sql = sprintf("SELECT %s FROM %s", - paste(sprintf("`%s`", select_cols), collapse = ", "), table), - token_name = token_name, workspace_id = workspace_id) - matched <- bt[bt$root_id %in% ids, ] - missing <- setdiff(ids, matched$root_id) - if (length(missing)) - warning(length(missing), " root IDs not found in SeaTable: ", - paste(utils::head(missing, 5), collapse = ", "), - if (length(missing) > 5) ", ...") - message(sprintf("Matched %d / %d root IDs in SeaTable", nrow(matched), length(ids))) - if (!nrow(matched)) { - message("No rows to update.") - return(invisible(NULL)) - } - - # Show current values for the target columns - show_cols <- intersect(c("root_id", cols), colnames(matched)) - message("\nCurrent values:") - print(matched[, show_cols, drop = FALSE], right = FALSE) - message(sprintf("\nProposed update: %s", - paste(sprintf("%s -> '%s'", cols, vals), collapse = ", "))) +# banctable_update_rows()'s multi-select payload builder retired: seatabler +# builds the payload now. - # Ask user for confirmation - ans <- readline(prompt = "Proceed with update? (y/n): ") - if (!tolower(trimws(ans)) %in% c("y", "yes")) { - message("Update cancelled.") - return(invisible(matched[, show_cols, drop = FALSE])) - } - - # Build update data.frame - df_update <- data.frame(`_id` = matched$`_id`, stringsAsFactors = FALSE, - check.names = FALSE) - for (i in seq_along(cols)) - df_update[[cols[i]]] <- vals[i] - - # Push - banctable_update_rows(df = df_update, table = table, base = base, - workspace_id = workspace_id, token_name = token_name, - append_allowed = FALSE) - message(sprintf("Updated %d column(s) for %d rows", - length(cols), nrow(df_update))) - invisible(matched[, show_cols, drop = FALSE]) -} # hidden, modified to enable working with list columns banctable2df <- function (df, tidf = NULL) { diff --git a/R/seatabler.R b/R/seatabler.R index 33feac0..f9f4476 100644 --- a/R/seatabler.R +++ b/R/seatabler.R @@ -7,10 +7,11 @@ # code lives in one place. The banctable_* names and signatures are unchanged: # there is a lot of analysis code calling them. # -# Not everything has moved yet. banctable_query()'s row conversion and -# banctable_update_rows() / banctable_append_rows() still use fafbseg helpers -# that seatabler has not ported (flytable_fix_coltypes, df2flytable, -# df2appendpayload); they move across once those land. +# Not everything has moved. banctable_query() still uses bancr's own row +# conversion: seatable_query() does not apply column types, so delegating it +# would change 11 column classes on banc_meta, including status and +# cell_type_source becoming list columns and _ctime/_mtime losing POSIXct. +# See flyconnectome/seatabler#9. The row writes moved in bancr 0.3.8. #' The BANC SeaTable connection #' diff --git a/tests/testthat/test-seatabler.R b/tests/testthat/test-seatabler.R new file mode 100644 index 0000000..57e5606 --- /dev/null +++ b/tests/testthat/test-seatabler.R @@ -0,0 +1,29 @@ +test_that("banc_seatable_connection describes the BANC server", { + con <- banc_seatable_connection() + expect_s3_class(con, "seatable_connection") + expect_equal(con$url, "https://cloud.seatable.io/") + expect_equal(con$token_envvar, "BANCTABLE_TOKEN") + expect_identical(con$workspace_id, "57832") +}) + +test_that("the SeaTable python stack is reachable", { + # bancr's other tests never touch this path, so a seatabler that cannot reach + # its python dependencies used to pass the whole suite and only fail in use. + skip_if_not_installed("seatabler") + skip_if_not_installed("reticulate") + skip_if_not(reticulate::py_available(initialize = TRUE)) + skip_if(Sys.getenv("BANCTABLE_TOKEN") == "", "no BANCTABLE_TOKEN set") + + expect_no_error(seatabler::seatable_module()) + expect_no_error(banctable_login()) +}) + +test_that("banctable_columns returns a schema with keys", { + skip_if_not_installed("seatabler") + skip_if(Sys.getenv("BANCTABLE_TOKEN") == "", "no BANCTABLE_TOKEN set") + + cols <- banctable_columns("banc_meta") + expect_s3_class(cols, "data.frame") + expect_true(all(c("key", "name", "type", "rtype") %in% colnames(cols))) + expect_true("root_id" %in% cols$name) +}) From 98ac8c3fe7cca021e2019f1941b5e3cdc88fd13e Mon Sep 17 00:00:00 2001 From: Alex Bates Date: Tue, 25 Aug 2026 14:26:07 -0400 Subject: [PATCH 2/2] Delegate banctable_query to seatabler too seatabler 0.2.2 applies column types from the table schema (st_coerce_df), which was the one thing keeping banctable_query()'s row conversion here. It now delegates like the rest, and banctable2df() retires with it. That removes the last fafbseg::: internal from banc-table.R. Reads are unchanged, checked against a snapshot taken before the switch on 500 rows of banc_meta: same 163 columns in the same order, and identical values for root_id, status and cell_type across every row. status stays character, so banc_update_status() and the string handling around it are unaffected. Five number columns (input_connections, output_connections, mitochondria, mitochondria_volume, cell_ids_id) come back as integer rather than numeric, because st_fix_coltypes() compares mode() and mode(integer) is "numeric". No effect on values: the largest across 3000 rows is 3.8 million against a 2^31 limit, and anything larger would arrive from pandas as a double anyway. Noted on flyconnectome/seatabler#9 rather than worked around here. limit = FALSE is still honoured, mapped to an infinite limit. `ac` stays in the signature and is ignored, as with the other wrappers. Requires seatabler (>= 0.2.2). Full suite 23 pass, 0 fail. Claude-Session: https://claude.ai/code/session_01TQHvpdwd2XCozxU6bKhRWQ --- DESCRIPTION | 2 +- NEWS.md | 10 +-- R/banc-table.R | 162 ++++--------------------------------------------- R/seatabler.R | 8 +-- 4 files changed, 22 insertions(+), 160 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 336daad..e4c2c08 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -40,7 +40,7 @@ Imports: utils, reticulate, nat.ggplot, - seatabler (>= 0.2.0) + seatabler (>= 0.2.2) Suggests: testthat (>= 3.0.0), readobj, diff --git a/NEWS.md b/NEWS.md index 8ba7756..8ade361 100644 --- a/NEWS.md +++ b/NEWS.md @@ -10,10 +10,12 @@ * New test covering the SeaTable Python path. bancr's suite previously passed even when seatabler could not reach its Python dependencies, because nothing exercised that path. -* `banctable_query()` still uses bancr's own row conversion. Delegating it would - change 11 column types, including `status` and `cell_type_source` becoming - list columns and `_ctime`/`_mtime` losing `POSIXct`; see - flyconnectome/seatabler#9. +* `banctable_query()` now delegates to seatabler's `seatable_query()`, which + applies column types from the table schema as of seatabler 0.2.2. Reads are + unchanged: same columns in the same order, same values, and `status` stays + character. Five `number` count columns come back as `integer` rather than + `numeric`, which has no effect on the values. +* bancr now requires seatabler (>= 0.2.2) for that column coercion. # bancr 0.3.7 (development) diff --git a/R/banc-table.R b/R/banc-table.R index 3b44bf0..25c9967 100644 --- a/R/banc-table.R +++ b/R/banc-table.R @@ -114,106 +114,16 @@ banctable_query <- function (sql = "SELECT * FROM banc_meta", workspace_id = "57832", retries = 3, table.max = 10000L){ - if(is.null(ac)) ac <- banctable_login(token_name=token_name) - table.max <- 10000L - if(limit>table.max){ - offset <- 0 - df <- data.frame() - while(offset0&&!nrow(bc)){ - bc <- banctable_query(sql=sql.new, - limit=FALSE, - base=base, - python=python, - convert=convert, - ac=ac, - token_name=token_name, - workspace_id=workspace_id) - tries <- tries - 1 - if (!nrow(bc) && tries > 0) { - # Exponential backoff: 1s, 2s, 4s - wait <- 2^(retries - tries - 1) - warning(sprintf(" Retry %d/%d for offset %d (waiting %ds)", - retries - tries, retries, offset, wait)) - Sys.sleep(wait) - } - } - if (!nrow(bc)) { - warning(sprintf("All %d retries exhausted at offset %d -- returning %d rows so far", - retries, offset, nrow(df))) - if (nrow(df)) return(df) else return(NULL) - } - df <- rbind(df,bc) - offset <- offset+nrow(bc) - if(!length(bc)|nrow(bc) 0)) - return(df) - nr = nrow(df) - # Convert any columns still stored as Python objects (numpy arrays) to native R. - # py_to_r(DataFrame) can leave columns as numpy.ndarray objects; these cause - # crashes in downstream flytable_fix_coltypes (e.g. x[is.na(x)] on a Python - # array triggers IndexError from wrong-length boolean index). - for (i in seq_along(df)) { - if (is.environment(df[[i]])) { - df[[i]] <- tryCatch( - df[[i]]$tolist(), # auto-converts to R via reticulate - error = function(e) rep(NA_character_, nr) - ) - } - } - listcols = sapply(df, is.list) - for (i in which(listcols)) { - li = lengths(df[[i]]) - if (isTRUE(all(li == 1))) { - ul = unlist(df[[i]]) - if (!isTRUE(length(ul) == nr)) - ul = sapply(ul,paste,collapse=",") - else df[[i]] = ul - } - else if (isTRUE(all(li %in% 0:1))) { - tryCatch({ - df[[i]][!nzchar(df[[i]])] = NA - }, error = function(e) { - df[[i]] <<- vapply(df[[i]], function(x) { - if (is.null(x) || length(x) == 0) NA_character_ - else { - s <- tryCatch(as.character(x)[1], error = function(e2) NA_character_) - if (is.na(s) || !nzchar(s)) NA_character_ else s - } - }, character(1)) - }) - df[[i]] = fafbseg:::null2na(df[[i]]) - } - else df[[i]] = sapply(df[[i]],paste,collapse=",") - } - if (is.null(tidf)) - df - else { - if (is.character(tidf)) - tidf = fafbseg::flytable_columns(tidf) - fafbseg:::flytable_fix_coltypes(df, tidf = tidf) - } -} +# banctable2df() retired: seatabler's st_coerce_df() applies the column types +# now, from seatabler 0.2.2. # hidden, helper function to update status column banc_update_status <- function(df, diff --git a/R/seatabler.R b/R/seatabler.R index f9f4476..fea83c5 100644 --- a/R/seatabler.R +++ b/R/seatabler.R @@ -7,11 +7,9 @@ # code lives in one place. The banctable_* names and signatures are unchanged: # there is a lot of analysis code calling them. # -# Not everything has moved. banctable_query() still uses bancr's own row -# conversion: seatable_query() does not apply column types, so delegating it -# would change 11 column classes on banc_meta, including status and -# cell_type_source becoming list columns and _ctime/_mtime losing POSIXct. -# See flyconnectome/seatabler#9. The row writes moved in bancr 0.3.8. +# As of bancr 0.3.8 the whole generic surface has moved: queries, row writes, +# schema, big data and snapshots. seatabler 0.2.2 applies column types from the +# table schema, so reads match what bancr used to return. #' The BANC SeaTable connection #'