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
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
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.]
| 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?
There was a problem hiding this comment.
I think the divergence here is deliberate (the api lineage match is the more complete of the two; the cli exact-taxon_id match under-catches strain-level genomes), so I've documented it rather than changed behavior. retrieveMetadata's roxygen now has a @details section laying out both differences (species: exact vs case-insensitive substring; taxon ID: lineage vs exact) and how to make the backends agree.
|
Changes in the most recent commit:
|
Adds an 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]
Changed default
retrieveMetadata()/prepareGenomes()now takemetadata_method, which defaults to"api". The previous Docker/CLI path is still available viametadata_method = "cli". An earlier version of this description said the default stayed on Docker — that is no longer true as of the "make API the default" commits.What this does
Adds a native-R way to download genome metadata and antibiotic-resistance (AMR) data straight from the BV-BRC web API, replacing the Docker/CLI path as the default.
metadata_method = "cli"restores the old behavior.Why
The old 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, far less than many species have. The API path fixes both:
It's a drop-in: the API path produces the same database tables as before, so everything downstream works unchanged.
What's validated
metadata_method = "api"(Docker off) produces all the expected tables, correctly populated.dev/parity_check.R,dev/parity_diff.R).Review cleanup (done)
data/bvbrc/bvbrcData.duckdbfrom the repo — the API path doesn't need it, andR CMD checkflagsdata/for non-R-data files.dev/bvbrc_api_prototype.R,dev/bvbrc_pipeline_prototype.R,dev/bvbrc_species_roster.R+ roster CSV); the real implementation lives inR/bvbrc_api.R.Scope / not included
retrieveMetadata(metadata_method = "api")is fully Docker-free (ID resolution + metadata + AMR). Only genome sequence files (.fna/.faa/.gff, inretrieveGenomes) still use Docker — to be addressed in a separate follow-up.Closes #38