SQ-853, SQ-838, SQ-799: Dimensions docs and minor updates#74
Conversation
Fixes the case where a VCF has been uploaded to divbase, dimensions update run, and then the VCF is deleted from the bucket and the indexed dimensions would be stale and lock in since the NoVCFFilesFoundError would be raise before the stale dimensions check
Before it used dicts. The crud logic here still needs dicts, but it is clearer to pass the data between functions and methods with dataclasses
Since the dimensions need to read every line in the VCF files, it is no surprise that bcftools (written in C) is fater than Python. Anecdotally, extracting the dimensions from the 66M variant file in ERZ022025 (mgp.v3.snps.rsIDdbSNPv137.vcf.gz) took ~240 s with Python and ~120 with bcftools.
Turns out that bcftools requires bgzipped VCFs for indexing. Currently, DivBase enforces .vcf.gz but if this were to change, the dimensions update logic would suddenly break for .vcf files. This adds bgzipping steps to the VCFDimensionCalculator.
Flush+commit (1 transaction) instead of two commits (2 transactions) to make this atomic. If two identical dimensions update jobs are picked up by two different workers, there could a potential concurrency issue. The first upsert has postgres row locks with on_conflict_do_update and should be fine, but the FK table update added in #70 was a second transaction without row lock. In the previous CRUD, there was a window between the first and second transactions where another concurrent identical job could see the first transaction (parent row with updated counts) but see that the child tables would be empty and re-calculate the scaffolds and samples. This was not a breaking concurrency issue (no data corruption), but an inefficient one that could cause redundant work.
For user convience for the case if a VCF files has been removed from the bucket during the dimensions update job
Instead of looping over one-by-one
There was a problem hiding this comment.
Pull request overview
This PR adds user-facing documentation for divbase-cli dimensions and updates the worker-side VCF dimensions indexing pipeline to improve performance (via bcftools) and robustness (stale-index cleanup + more atomic CRUD).
Changes:
- Replace Python line-by-line VCF dimension extraction with
bcftools-based header parsing + CSI indexing/stats. - Update
update_vcf_dimensions_taskto clean stale dimensions DB entries earlier (and again at end for a concurrency edge case), and introduce batch-delete CRUD helpers. - Add unit/e2e tests for the new indexing logic and cleanup behaviors; expand user guides around dimensions + query prerequisites.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/divbase-api/src/divbase_api/worker/vcf_dimension_indexing.py |
New bcftools-based dimensions extraction + CSI indexing pipeline. |
packages/divbase-api/src/divbase_api/worker/tasks.py |
Moves stale-entry cleanup earlier, adds end-of-task cleanup, deletes CSI artifacts, switches CRUD payloads to dataclasses. |
packages/divbase-api/src/divbase_api/worker/crud_dimensions.py |
Refactors worker CRUD to use dataclass payloads, uses flush for atomic parent/child updates, adds batch delete helpers. |
tests/unit/divbase_api/test_vcf_dimension_indexing.py |
New unit tests covering header parsing, CSI path handling, stats parsing, and skip behavior. |
tests/e2e_integration/cli_commands/test_dimensions_cli.py |
New e2e tests for stale-index cleanup, batch delete helpers, worker-file cleanup, and plain .vcf indexing. |
docs/user-guides/vcf-dimensions.md |
New/expanded end-user guide for dimensions update/show and related commands. |
docs/user-guides/running-queries.md |
Clarifies query overview and adds prerequisite guidance around dimensions. |
docs/user-guides/quick-start.md |
Minor wording update related to dimensions update. |
docs/user-guides/query-syntax.md |
Adds a note about dimensions/VCF state at query submission time. |
Comments suppressed due to low confidence (1)
packages/divbase-api/src/divbase_api/worker/tasks.py:560
VCF_files_deletedis now always returned as a list (possibly empty). Previously this field was normalized toNonewhen there were no deletions; keeping that behavior would better match theDimensionUpdateTaskResultschema (Optional[list[str]]) and avoid a subtle response-shape change for clients.
if not files_indexed_by_this_job:
files_indexed_by_this_job = None
if not divbase_results_files_skipped_by_this_job:
divbase_results_files_skipped_by_this_job = None
result = DimensionUpdateTaskResult(
status="completed",
VCF_files_added=files_indexed_by_this_job,
VCF_files_skipped=divbase_results_files_skipped_by_this_job,
VCF_files_deleted=vcfs_deleted_from_bucket_since_last_indexing,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| The `divbase-cli dimensions update` pre-reads the VCF files in the project's data store so that DivBase does not need to fetch and read each VCF every time the user submits commands that require knowledge about the VCF dimensions, such as queries. | ||
|
|
||
| The `divbase-cli dimensions update` will look for `.vcf.gz` files in the project's data store. If there is no VCF dimensions cache for the project, it will create it. If not, it will compare the existing record in the cache with the current status of the object store. If any new VCF files have been added or if any VCF file version have been updated, it will update the VCF dimensions cache with that information. DivBase uses `bcftools` to extract the dimensions information from the VCF files. | ||
|
|
There was a problem hiding this comment.
This section says dimensions update “will look for .vcf.gz files”, but the worker code indexes both .vcf and .vcf.gz (and even bgzips plain .vcf internally). Please update the docs to reflect that plain .vcf inputs are supported.
There was a problem hiding this comment.
This is a delibrate mismatch and defensive programming. At the moment, only .vcf.gz files are allowed to be uploaded to DivBase buckets. If .vcf files would be allowed in the future, the bgzip step ensures that the dimensions update does not break.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This is functionally the same, but a more formal interpretation of the bcftools manual.
|
This is ready for review @RMCrean. I'd say that this is mostly docs and some nice-to-haves for the dimensions update command. Have a nice weekend! |
RMCrean
left a comment
RMCrean
left a comment
There was a problem hiding this comment.
Thanks for these improvements and changes. Nice catch with the double db commit, it is something I will try to think about going forward. And cool idea to make use of bcftools for the dimensions command! 🚀
Disclaimer that I haven't yet had a chance to manually checkout and test the changes or look at the docs (I was hoping to find time to check it out before the end of the day, but ran out of time I'm afraid). But I still have some questions and ideas from looking at it via GH.
I will hopefully have time tomorrow to test it out.
| def _delete_job_files_from_worker( | ||
| vcf_paths: list[Path] = None, metadata_path: Path = None, output_file: Path = None | ||
| ) -> None: | ||
| """ | ||
| After uploading results to bucket, delete job files from the worker. | ||
| """ | ||
| for vcf_path in vcf_paths: | ||
| vcf_path = Path(vcf_path) | ||
| try: | ||
| os.remove(vcf_path) | ||
| logger.info(f"Deleted {vcf_path} from worker.") | ||
| except Exception as e: | ||
| logger.warning(f"Could not delete input VCF file from worker {vcf_path}: {e}") | ||
| csi_path = vcf_path.with_suffix(vcf_path.suffix + ".csi") | ||
| if csi_path.exists(): | ||
| try: | ||
| os.remove(csi_path) | ||
| logger.info(f"Deleted CSI index {csi_path} from worker.") | ||
| except Exception as e: | ||
| logger.warning(f"Could not delete CSI index file {csi_path}: {e}") | ||
| if metadata_path is not None: | ||
| try: | ||
| os.remove(metadata_path) | ||
| logger.info(f"Deleted {metadata_path} from worker.") | ||
| except Exception as e: | ||
| logger.warning(f"Could not delete metadata file from worker {metadata_path}: {e}") | ||
| if output_file is not None: | ||
| try: | ||
| os.remove(output_file) | ||
| logger.info(f"Deleted {output_file} from worker.") | ||
| except Exception as e: | ||
| logger.warning(f"Could not delete output file from worker {output_file}: {e}") |
There was a problem hiding this comment.
Question/thought: Don't feel like you need to implement this on this PR or at all.
I see you're working in Path.cwd for each job for the worker. Perhaps a better approach would be to actually create a temp dir for each job.
E.g.
task_dir = Path.cwd / {task-id}
And do the file download and run all commands inside that dir for each task.
That way your guarantee that the file in the working directory are only generated from this task (aka isolation between runs). Otherwise in a similar vein to another comment, a failed set of deletions here would result in some logger messages (which might not get seen) and lingering files which will be in the same dir as the next worker job run. And with this approach, even if there were lingering files, they wouldn't affect the next job run as it will be performed in a diff dir.
This would also make clean up a lot simpler as you could just drop the entire dir once the job is complete.
There was a problem hiding this comment.
Great idea, and something I've had in the back of my mind for some time too. I think it would be nice to make a separate PR for this.
There was a problem hiding this comment.
That sounds, good I'd be happy to brainstorm if useful 👍
There was a problem hiding this comment.
Great, sounds fun to brainstorm about that!
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.
RMCrean
left a comment
RMCrean
left a comment
There was a problem hiding this comment.
Thanks for implementing the changes and nice job, I think this LGTM 👍
This PR is about writing user guides for
divbase-cli dimensionsand fixing some things that I noticed while doing that.1. Move a check for stale dimensions entry upwards in the task definition.
This covers a soft-lock that previously would happen if a user: uploads a VCF file to the bucket, runs the dimensions update, delete all VCF files in the bucket, and runs dimensions update again against an empty bucket. Prior to this PR, that would raise NoVCFFilesFoundError before the stale dimensions entry cleanup would run.
Then I was thinking about concurrency and added a second run of the check back in at the end of the task too to cover another stale entry edge case. If a VCF file has been removed from the bucket while the dimensions update job was running, the dimensions update would need to be run again before queries could be run. Db operations are cheap in comparison to dimensions calculations, so before finishing the task, the logic now checks if any VCFs have been deleted from the bucket and drops stale entries from the db. Not stricltly neccessary, but improves UX a little bit.
2. Use bcftools to extract the dimensions instead of Python.
To get all the dimensions data we need, we need to read the VCF file line-by-line. Since bcftools is written in C, it is much faster (and specialised) at this than Python. For large VCF files, this makes a noticable difference: the 5.5Gb, 66M variant file mgp.v3.snps.rsIDdbSNPv137.vcf.gz (ERZ022025) that we have previously used for benchmarking took ~240 s with Python and ~120 with bcftools. About 40 s of that (in both cases) is the download of the VCF file to the worker.
3. Concurrency fix in CRUD
I was looking over the CRUD functions used by the workers and spotted a small concurrency issue with the FK child tables for scaffolds and samples added in #70. Previously, this used two transactions (
db.commit): one after the parent table upsert and one after the child table update. This created a window where an identical dimensions update task could be run in parallel in another worker, see the first transaction but see that the child tables would be empty and re-calculate the scaffolds and samples (i.e. before second transaction has been sent by the first job).This is not a big problem since it mostly wastes some resources and bandwidth to recalculate stuff. But as a principle, this CRUD was not atomic. Now I refactored it to only use a single transaction using
db.flush.