SQ-893: Write user guide and add polish to the VCF queries#87
Merged
Conversation
Towards allowing users to specify a list of samples to query in the VCF queries.
Samples can come from: tsv metadata query, a list of sample names, or if none of the two: all samples.
I.e. user-defined samples and not metadata query-derived sample results.
Small change but communicates more clear what the command does. Should consider renaming the command name too (bcftools-pipe) at some point.
Also, if user added --samples or --samples-list, check these against the dimensions cache and make an early exit if there is a mismatch (e.g. spelling, dimensions not up-to-date)
Previously, it opened quite a few sessions with no real good reason. Now it opens two: once at the start, and once at the end after all the calculation are done. Using a single session could work too but it would be open for quite a long time and risk becoming stale.
As discussed in the PR review. Since other CLI commands, in particular the queries, rely on the integrity and accuracy of the dimensions entries, it is better use hard errors and celery task FAILURE here.
The old SAMPLES placeholder that was needed to insert the TSV metadata query resolved samples was brittle and a hack for the original demo. Now the system will take the resolved samples and run view -s on them as the first step in the bcftools pipe. Users can override this by explicitly stating view -s in the query CLI command if they think that it is more efficient to subset on samples later in the pipe. (Example: it can be more performant to first subset on a genomic region with view -r and then subset on samples with view -s, rather than the other way around, due to the N+1 read/write operation needed for VCF sample columns.)
Run early in task and early-exit if command is invalid for DivBase.
Shlex is now used to parse the input command from the CLI and before starting subprocesses with bcftools.
Now that view -s is not its own, first command per default, the tests show that view -r needs indexed files. Previously, the indexin only was enforce for the temp files, but now indexing also is checked and enforced for the source files.
Otherwise they show up in typer
Full cmd is now divbase-cli query vcf. Clear and imperative.
Move some of that content into the detailed guides instead.
Use numbered heads for accessibility to easier see the nested subheadings
By showing the commands we do no support, we do not need to explain every command that we might support
With a focus on implementing/removing TODOs
E.g. the name bcftools-pipe was created for the first demo of divbase, but now this is referred to as vcf queries elsewhere in the codebase.
For consistency with codebase which uses shlex.split for parsing user input commands like this
Include as part of the validation run in the endpoint layer and just not in the task layer. tasks.py will call it again when fetching the scaffolds it needs to in order to select the VCF files to download, which perhaps is overly defensive.
Clarify the help message for the query vcf subcommand to indicate that one of the mutually exclusive sample selection options is required.
return the int job_id, not the internal celery task uuid to user if sample metadata query not finished in time
Legacy db entries (in my case >200 task history entries since the last four months) can trigger a bug in the task history deserialization since the previous fields were capitalized. I have observerd a similar error on the dev cluster and thus this fix should be applied to the main branch as well. Alternatively, we could purge the dev cluster db and start over since we are not yet in pilot phase.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors and “polishes” DivBase VCF queries with a documentation-driven approach, including a renamed CLI/API surface (query vcf + /v1/query/vcf/), hardened bcftools command validation, improved sample-selection modes, and expanded tests and user guides.
Changes:
- Renames the VCF query CLI/API entrypoints and updates docs/scripts accordingly.
- Adds explicit sample-selection modes (
--tsv-filter,--samples,--samples-file,--all-samples) plus normalization/validation. - Centralizes/hardens bcftools command validation and expands unit/e2e test coverage + new/updated user guides.
Reviewed changes
Copilot reviewed 42 out of 44 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/divbase_lib/test_query_schemas.py | Adds schema validation tests for non-empty command + pipeline segments. |
| tests/unit/divbase_cli/test_query_cli.py | Adds tests for CLI sample normalization from --samples / --samples-file. |
| tests/unit/divbase_api/test_vcf_queries.py | Adds unit tests for sample-selection modes, bcftools validator, and manager behavior. |
| tests/unit/divbase_api/services/test_task_history.py | Ensures legacy task-history payloads still deserialize. |
| tests/e2e_integration/queries/test_bcftools_tasks.py | Updates overlap tests + adapts to new dimensions dataclasses/errors. |
| tests/e2e_integration/queries/test_BcftoolsQueryManager.py | Updates integration tests for new command injection and parsing behavior. |
| tests/e2e_integration/queries/conftest.py | Updates fixtures to use BCFToolsInput/SampleFileMapping. |
| tests/e2e_integration/conftest.py | Updates cleanup logic to new ProjectVCFDimensionsData structure. |
| tests/e2e_integration/cli_commands/test_queue_status.py | Updates CLI command names and expected behavior. |
| tests/e2e_integration/cli_commands/test_query_cli.py | Adds coverage for new query modes and submission-time validation behavior. |
| tests/e2e_integration/cli_commands/test_dimensions_cli.py | Adds --cached-vcf-files CLI coverage and adapts to dataclass dimensions. |
| scripts/benchmarking/run_mouse_vcf_job_in_docker_compose.py | Updates benchmarking script to new query vcf syntax. |
| scripts/benchmarking/factorial_design_submit_jobs.py | Updates benchmarking submission script to new query vcf syntax. |
| packages/divbase-lib/src/divbase_lib/api_schemas/vcf_dimensions.py | Adds schema for “unique VCF files” endpoint response. |
| packages/divbase-lib/src/divbase_lib/api_schemas/queries.py | Adds shared validators, sample-selection model validation, legacy key mapping. |
| packages/divbase-cli/src/divbase_cli/cli_commands/query_cli.py | Renames command to vcf, adds sample-selection UX, normalization, new endpoint path. |
| packages/divbase-cli/src/divbase_cli/cli_commands/dimensions_cli.py | Adds --cached-vcf-files output using a dedicated endpoint + table TSV. |
| packages/divbase-api/src/divbase_api/worker/tasks.py | Implements sample-selection modes + worker-side defensive command validation. |
| packages/divbase-api/src/divbase_api/worker/crud_dimensions.py | Converts worker dimensions payloads from dicts to dataclasses. |
| packages/divbase-api/src/divbase_api/services/queries.py | Centralizes bcftools command validation, improves parsing, injection, and subprocess handling. |
| packages/divbase-api/src/divbase_api/routes/vcf_dimensions.py | Adds endpoint for unique VCF file/version entries. |
| packages/divbase-api/src/divbase_api/routes/queries.py | Renames endpoint to /vcf/..., adds pre-queue validation for samples/command. |
| packages/divbase-api/src/divbase_api/crud/vcf_dimensions.py | Adds CRUD for unique VCF file/version pairs. |
| mkdocs.yml | Updates nav to new/renamed query docs pages. |
| docs/user-guides/vcf-query-syntax.md | Adds a comprehensive VCF query syntax guide (draft). |
| docs/user-guides/vcf-files.md | Adds/expands VCF preparation + compatibility guidance. |
| docs/user-guides/vcf-dimensions.md | Updates dimensions guide and adds --cached-vcf-files documentation. |
| docs/user-guides/tutorial-query-on-public-data.md | Updates tutorial commands/links for renamed query command. |
| docs/user-guides/troubleshooting.md | Starts troubleshooting page draft. |
| docs/user-guides/sidecar-metadata.md | Updates references/anchors to renamed query docs. |
| docs/user-guides/setup_divbase_cli.md | Minor doc capitalization fix. |
| docs/user-guides/running-queries.md | Removes old running-queries overview page. |
| docs/user-guides/running-queries-overview.md | Adds new running-queries overview page. |
| docs/user-guides/quick-start.md | Updates quick-start query references to query vcf and new docs. |
| docs/user-guides/query-syntax.md | Removes old placeholder VCF query syntax doc. |
| docs/user-guides/index.md | Expands user-guides index with the new guide set. |
| docs/user-guides/how-to-create-efficient-divbase-queries.md | Adds initial efficiency tips and links. |
| docs/index.md | Adds prominent tutorial link. |
| docs/development/celery_task_implementation.md | Updates developer docs for renamed endpoints/commands. |
| docs/assets/diagrams/vcf_query_processing_for_user_guide.mermaid | Adds diagram for VCF query processing. |
| docs/assets/diagrams/bcftools_task_logic_flow_full.mermaid | Updates diagram labels/endpoint names. |
| README.md | Updates README examples and explains sample-injection behavior. |
Comments suppressed due to low confidence (1)
packages/divbase-lib/src/divbase_lib/api_schemas/vcf_dimensions.py:1
s3_version_idis modeled as a requiredstr, but the backend can yieldNULL/Nonefor version IDs (and worker dataclasses already treat it as optional). This will cause response-model validation failures for projects with missing version IDs. Makes3_version_idoptional (str | None) or ensure the API always returns a non-null string (e.g. empty string) and document the invariant.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| The processing of the VCF files on the DivBase server is done with [`bcftools`](https://github.com/samtools/bcftools). DivBase will detect the VCF files in the project's data store that are needed for the query; if more than one VCF file is needed, DivBase will ensure that the files are compatible with each other according to the requirements of `bcftools` and ensure that a single results file with the subset data is returned to the user by running `bcftools merge` and `bcftools concat` on the intermediate files as needed. The result is a single VCF file that is uploaded to the projects data store and named after the job ID. | ||
|
|
||
| Users can query the VCF data in their project with or without combining it with a [sample metadata query](docs/user-guides/sidecar-metadata.md). |
There was a problem hiding this comment.
These links look incorrect for MkDocs’ relative linking: the first includes a redundant docs/user-guides/ prefix, and the second points to a filename that doesn’t exist in this PR (sidecar-metadata-queries.md). Update both to the correct relative path(s) (likely sidecar-metadata.md).
Suggested change
| Users can query the VCF data in their project with or without combining it with a [sample metadata query](docs/user-guides/sidecar-metadata.md). | |
| Users can query the VCF data in their project with or without combining it with a [sample metadata query](sidecar-metadata.md). |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 112 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+16
to
+20
| # This will return the job ID of the submitted job. Example: | ||
| # Job submitted successfully with task id: 123 | ||
|
|
||
| - When any new VCF file is uploaded to the project. | ||
| # Job status can be viewed with e.g. | ||
| divbase-cli task-history id 123 |
There was a problem hiding this comment.
The example output here no longer matches the CLI output (the CLI now includes a hint to use divbase-cli task-history id <job_id>). Consider updating this example so users can copy/paste and recognize the new message format.
Contributor
Author
There was a problem hiding this comment.
Full CLI output added ot the example. This comment reminds me that it would be good to harmonize when we use "job ID" and when we use "task ID" for user facing messages.
Comment on lines
+146
to
+149
| # Backward compatibility for historical task kwargs created before all_samples option was implemented. | ||
| # To ensure that task-history deserizliation does not break for existing tasks. | ||
| # Could be handled by a backfilling migration instead. | ||
| self.all_samples = True |
There was a problem hiding this comment.
Typo in comment: “deserizliation” → “deserialization”. Since this comment explains backward-compatibility behavior, it’s worth keeping it clean for future maintainers.
Comment on lines
+111
to
115
| f"The query is still being processed and has Task ID: {job_id}. \n" | ||
| f"Please check back later for the results. \n" | ||
| f"To check the status of the query you can use the following command: \n" | ||
| f"divbase-cli task-history id {results.id}" | ||
| f"divbase-cli task-history id {job_id}" | ||
| ) |
There was a problem hiding this comment.
In the timeout error message, this value is the DivBase job id returned from create_task_history_entry, not the Celery task UUID. Referring to it as “Task ID” is confusing for users; consider renaming to “Job ID” in the message (and keep the CLI hint using divbase-cli task-history id <job_id>).
Contributor
Author
There was a problem hiding this comment.
Intentional choice agreed upon in the team. User should see job ID "ints" and not the UUID
Small refactor to make it hopefully more readable, maintainable and DRY.
As spotted by copilot PR review sorting the samples in the dimensions retrieval logic will break the downstream validation logic in query manager that checks for bcftools concat compatibility. Add logic, comments, and regression test for this
As copilot review pointed out and was verified by reading the bcftools manual, there are bcftools view commands that CAN take ;, often within its own quotes. This adds support for such commands, with the caveat that the inner quotes need to be escaped on the CLI side. example CLI command that can be used with tests/fixtures/vcf_specification_v45_example11.vcf.gz divbase-cli query vcf --samples "NA00001,NA00002,NA00003" \ --command "view -i 'FILTER~\"q10\"'"
Contributor
Author
|
Merging now after personal demo/discussion yesterday and fixing of two important cases identified by the copilot review. 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is about documentation-driven polish of the VCF queries in DivBase, including bug fixes, hardening, and UX improvements.
Some major changes include:
Renaming things for improved UX and maintenance. CLI command from
divbase-cli query bcftools-pipetodivbase-cli query vcf. Semantically more satisfying, shorter to type, and comparable to the recent renaming of the metadata command (divbase-cli query tsv). The API endpoint is renamed fromv1/query/bcftools-pipe/tov1/query/vcf/to go along with the CLI command renaming. I was tempted to rename the task and the task function too, but decided against it for now since there is logic that acts on the task nameImproving how samples are entered in the VCF queries. Many design decisions, so covered in separate section below
Centralized bcftools command validation. Users enter the
bcftools viewcommands on the CLI with the--command. The validation has been refactored and hardened and is now centralized invalidate_user_submitted_bcftools_command()inservices/queries.py. Main point of call is in the API layer in order to exit early beforetask.apply_asyncif validation fails. The task itself calls the validation again for direct task invocation (tests/manual Celery enqueue); this is perhaps overly defensive and might be something we want to consider dropping.Related to the validator: I have gone through all
bcftools viewoptions in the bcftools manual and distinguished which should be supported in VCF queries (implemented in_check_if_view_option_is_supportedand documented in Section 4.2. of the VCF query syntax user guide. The blacklisted options typically refer to I/O and should not be controllable by the user.The user guides are now at a first complete draft. I.e. there is text written for all pages, but the editorial work remains.
Substantially increased the test coverage of the VCF query subsystem. This was an undertested part of the codebase, I feel.
dataclasses galore! (dicts be gone, mostly anyway)
Entering samples for VCF queries
The previous implementation of the task did in fact not allow a VCF query to be run without a sample metadata query. Sample metadata based queries should probably be the selling point of the VCF queries since this a novel feature that is not supported by bcftools itself. Still, we need to accommodate for VCF queries that do not use sample metadata queries.
This refactoring was designed on the idea that users should not need to know which VCF file their desired data is in when doing VCF queries, and that they instead should specify which samples they want to query on.
It is now mandatory for the user to input the samples they wish to query on, using one of the following:
--tsv-filter(sample metadata),--samplesand--samples-file(two different ways of direct sample name input to the CLI), or--all-samples. These are mutually exclusive and one of these is required to submit the task. This is validated against in the CLI layer. It seems to me thattyperdoes not support "one of these options is required" out of the box, but I have tried to organise the help message fordivbase-cli query vcfto explain this.I put some guardrails around
--all-samplesto avoid accepting queries that do not subset on more parameters that just the samples: otherwise this would mean that all data in all VCFs in a DivBase project would be merged to a single file. This would be costly and in a way contrary to the idea that DivBase VCF queries are subsets of the full dataset. My intuition is that if users want a single monolithic file, they should download the source VCFs from the DivBase project and perform the merge themselves. We can always remove these guardrails later or implement a--forceoption.The bcftools orchestration logic has also changed to dynamically handle sample ID inputs when the commands are built. The earlier version had users type the magic placeholder
view -s SAMPLESfor the system to know where in the bcftools pipe the sample subsetting should occur. This has now been replaced with an automatic addition ofview -s <samples>to the first segment of the user submitted bcftools command string. In practice, this means that users never need to explicitly specifyview -sin their commands. (They can still do that if they want to control when in the workflow sequence the sample subsetting should occur, e.g. for optimisation purposes)The VCF query command now looks like this: