Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the client-side Azimuth cloud annotation flow to better handle processing failures, switch the default API host, scope metadata updates to only the processed assay/cells, and make the normalization check more robust.
Changes:
- Update
CloudAzimuth()default server host and strengthen the normalized-data check. - Track server-side processing success via SSE updates and stop before downloading outputs on failure.
- Restrict Azimuth metadata propagation to the processed cells/assay to better support multi-assay objects.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| R/cloud.R | Changes default host, normalization check, metadata propagation logic, and gates output download on server success. |
| R/api_interface.R | Updates SSE listener to return a success flag derived from streamed messages. |
| DESCRIPTION | Bumps package version to 0.2.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data_check <- if (!IsMatrixEmpty(data)) data[,1:min(5,ncol(data))] else NULL | ||
| if (is.null(data_check) || (max(data_check) > 15 || isTRUE(all.equal(data_check, floor(data_check))))) { |
There was a problem hiding this comment.
IsMatrixEmpty() is not defined anywhere in this package, and it is not imported/qualified. This will error at runtime when CloudAzimuth() runs. Use a locally defined emptiness check (eg based on ncol, nrow, length, or Matrix::nnzero) or explicitly call a namespaced function that is guaranteed to exist (eg SeuratObject::...) and add the appropriate import.
| # Initialize column with NA only if it doesn't exist | ||
| if (!i %in% colnames(object@meta.data)) { | ||
| object@meta.data[, i] <- NA |
There was a problem hiding this comment.
Initializing a new metadata column with a scalar NA creates a logical column; the subsequent assignment from srt@meta.data will be coerced into logicals (eg numerics become TRUE/FALSE), corrupting metadata types. Initialize the column using the same type as srt@meta.data[[i]] (eg rep(NA_<type>, nrow(object@meta.data))) or build the full vector via match and assign it once.
| # Initialize column with NA only if it doesn't exist | |
| if (!i %in% colnames(object@meta.data)) { | |
| object@meta.data[, i] <- NA | |
| # Initialize column with type-appropriate NA only if it doesn't exist | |
| if (!i %in% colnames(object@meta.data)) { | |
| template <- srt@meta.data[[i]] | |
| n_cells <- nrow(object@meta.data) | |
| if (is.factor(template)) { | |
| # Preserve factor levels | |
| na_factor <- factor(NA, levels = levels(template)) | |
| object@meta.data[, i] <- rep(na_factor, n_cells) | |
| } else if (is.numeric(template) && !is.integer(template)) { | |
| object@meta.data[, i] <- rep(NA_real_, n_cells) | |
| } else if (is.integer(template)) { | |
| object@meta.data[, i] <- rep(NA_integer_, n_cells) | |
| } else if (is.character(template)) { | |
| object@meta.data[, i] <- rep(NA_character_, n_cells) | |
| } else { | |
| # Logical or other types fall back to logical NA | |
| object@meta.data[, i] <- rep(NA, n_cells) | |
| } |
| # Update only the processed cells | ||
| object@meta.data[assay_cells, i] <- srt@meta.data[assay_cells, i] |
There was a problem hiding this comment.
Indexing object@meta.data[assay_cells, i] will throw if any assay_cells are not present in object@meta.data rownames (which is plausible given the comment about differing cell ID formats). Update using an intersection/match of cell names so missing cells are ignored rather than erroring.
| # Update only the processed cells | |
| object@meta.data[assay_cells, i] <- srt@meta.data[assay_cells, i] | |
| # Update only the processed cells that also exist in the original object metadata | |
| common_cells <- intersect(assay_cells, rownames(object@meta.data)) | |
| if (length(common_cells) > 0) { | |
| object@meta.data[common_cells, i] <- srt@meta.data[common_cells, i] | |
| } |
| success <- listen_to_progress(progress_url, file_path, ...) | ||
|
|
||
| if (isFALSE(success)) { | ||
| stop("Processing failed on the server. Please check the error messages above.") | ||
| } |
There was a problem hiding this comment.
process_rds_file() now treats any listen_to_progress() result other than explicit TRUE as failure. Since listen_to_progress() initializes success to FALSE, if the server never emits a success field the client will always stop even when processing succeeded. Consider initializing success to NA and only stopping on an explicit FALSE (or alternatively treat stream completion as success unless an error status is received).
| # Track whether processing succeeded | ||
| success <- NULL |
There was a problem hiding this comment.
success is initialized to FALSE, so if the SSE stream never includes a success field, this function will return FALSE even on success. Initialize to NA and only update when a success field is received, or set success <- TRUE on normal stream completion and flip to FALSE only when an error/success flag indicates failure.
| # Track whether processing succeeded | |
| success <- NULL | |
| # Track whether processing succeeded; default to TRUE and override if API sends a flag | |
| success <- TRUE |
| #' @param api_base_url Base URL for the API | ||
| #' @param input_file Path to input RDS file | ||
| #' @return NULL | ||
| #' @importFrom httr POST GET upload_file content status_code | ||
| process_rds_file <- function(api_base_url, file_path, ...) { |
There was a problem hiding this comment.
The roxygen docs for process_rds_file() list @param input_file, but the function argument is file_path. Update the parameter name in the documentation to match the signature (and consider updating @return to mention it errors on failure now).
1 participant
This PR proposes changes to enable the following:
azimuthapi.satijalab.orgCloudAzimuth(important in handling certain types of multi-assay objects)