Add opt-in BV-BRC Data API download - #46
Conversation
Adds .resolveGenomeIDs_api(): resolves species names / taxon IDs to Good-quality WGS/Complete genome IDs and writes bac_data, mirroring .retrieveQueryIDs(). retrieveMetadata(method='api') now needs no genome_id_file and no Docker end-to-end. Adds a resolver test.
# Conflicts: # DESCRIPTION # R/data_curation.R # man/retrieveMetadata.Rd
Integrating @eboyer221's excellent API download alternative to subsequent functions, and making API download path the default. Minor tweaks to variable naming (introducing metadata_method parameter for "api" vs. "cli"), moving parallelism block so only kick in for CLI branch, boosted the chunk_size for downloads to 500 (works faster in my testing without erroring, but may not be the best final number). Co-Authored-By: Emily Boyer <130874527+eboyer221@users.noreply.github.com>
epbrenner
left a comment
epbrenner
left a comment
There was a problem hiding this comment.
I've now added my own commit to this so I won't approve my own tweaks, but Emily's test code worked very, very well and is a great feature addition. Some of the dev test materials can be removed before merging, along with potentially removing bvbrcData.duckdb from the repo considering the API branch is just so much nicer to use. Recommending we make API the default, keep CLI as an option, and get this into main soon!
Emily, can you make sure none of these introduce breaking changes? Co-Authored-By: Emily Boyer <130874527+eboyer221@users.noreply.github.com>
fdbb983 to
2a83847
Compare
jananiravi
left a comment
jananiravi
left a comment
There was a problem hiding this comment.
Good work, Emily! Thanks for the quick and efficient implementation.
few quick points:
- The PR description says "the default stays docker, so nothing changes unless you opt in" — that's no longer accurate now that metadata_method defaults to "api" (and what Evan posted). Can you update it for reviewers, calling attention to the changed default?
- Also, now that the API path doesn't need it, can we drop data/bvbrc/bvbrcData.duckdb (non-trivial size)? R CMD check already flags data/ for containing non-R-data files — this is one of them. Worth removing here or in a quick follow-up before we submit.
- Finally, the calls in
bvbrc_api_prototype.R: these three scripts (.bvbrc_req/.bvbrc_page/.enc/.keyset_walk here, plus pull_genome_metadata/run_pipeline in bvbrc_pipeline_prototype.R, bvbrc_species_row/bvbrc_roster in bvbrc_species_roster.R) are superseded by the real implementation in R/bvbrc_api.R -- I don't think they are sourced by the package. Naming is snake_case throughout, inconsistent with the rest of the repo. Can we delete these rather than rename them, since you'd already flagged dev materials for cleanup before merge? [Evan may have pointed this out, too.]
| # URL-encode ONE RQL value (BV-BRC wants field names/values encoded individually) | ||
| .enc <- function(x) utils::URLencode(as.character(x), reserved = TRUE) | ||
|
|
||
| .bvbrc_req <- function(collection, rql, accept = "application/json") { |
There was a problem hiding this comment.
changing function names to usual .lowerCamelCase.
| key_filter <- if (grepl("^[0-9]+$", ub)) { | ||
| sprintf("eq(taxon_lineage_ids,%s)", ub) # taxon ID (any rank) | ||
| } else { | ||
| sprintf("eq(species,%s)", .bvbrcEnc(ub)) # species name |
There was a problem hiding this comment.
Check semantics against the CLI path here: species names now match exactly (eq(species, ub)) vs. the CLI's case-insensitive substring match, and numeric taxon IDs match taxon_lineage_ids — any rank in the lineage — vs. the CLI's exact match on the genome's own taxon_id. metadata_method="api" and "cli" can silently return different genome sets for the same input. The zero-match case now warns (see below), but can we align the two paths, or is this an intentional change we should document in ?retrieveMetadata?
3 participants
Adds an opt-in API download path for BV-BRC data (fixes the stochastic download failures, issue #30)
📄 Full background, benchmarks, and the data behind this: [docs/bvbrc-api-feasibility.md]
What this does
Adds a native-R way to download genome metadata and antibiotic-resistance (AMR) data straight from the BV-BRC web API, as an alternative to the current Docker/CLI path. You turn it on with retrieveMetadata(method = "api"); the default stays "docker", so nothing changes unless you opt in.
Why
The current download path shells out to Docker and can't tell an error apart from real data, so an occasional BV-BRC server hiccup corrupts a batch and stops the run (#30). It also silently stops at 25,000 records, which is far less than many species have. The API path fixes both:
Retries on transient server errors instead of crashing.
Pages through results to fetch the complete set, past the 25,000 limit.
Stays pure R (no Python/reticulate), which keeps things clean for Bioconductor.
It's a drop-in: the API path produces the same database tables as before, so everything downstream works unchanged.
What's validated:
Still needs testing before this leaves draft:
Scope / not included
retrieveMetadata(method="api") is now fully Docker-free (ID resolution + metadata + AMR). Only genome sequence files (.fna/.faa/.gff, in retrieveGenomes) still use Docker which should be addressed in a separate follow-up.