SQ-837: Refine the sample metadata TSV handling and write user guides

[brinkdp]

This PR is a major refactoring of the sidecar sample metadata TSV query logic. Focus was on adding functions like numerical operations, and improving UX.

Things covered in this PR:

• supported numerical operators (inequalities, ranges) and negative values in query filters
• supported ! for NOT filtering
• supported multi-value cells in TSV, delimited by semicolon
• added CLI command to generate sidecar tsv template from project's dimensions index
• added CLI command for a client-side sidecar tsv validator (decided against server-side since it would need to send potentially big request payloads). The validation logic is in the libs package so that the same code can be used in the client side validator and in the server side query processing.
• specified requirements for sample metadata files in DivBase in user guide and implemented them as warnings and errors in the validator and the query manager. Most important one is: if a column is not strictly numeric, treat it as string and send warning to user when they try to use numerical operators on that column.
• ensured errors (and warnings) propagate back to the user (auth exception is triggered first, so it should be fine to have details in the subsequent exceptions, I thought.)
• refactored error on VCF dimensions not being up-to-date and raised it for metadata queries too (previously only for VCF queries)
• added separate CRUDs and endpoints for only getting unique samples (and unique scaffolds; not really used in this PR but very similar logic) from dimensions table. divbase-cli dimensions show --unique-samples now gives number and names of samples (and scaffolds for --unique-samples`)
• added unit and e2e tests for many, many use cases
• add a script that loads a TSV and prints the result df to terminal for inspection. Calls the same function that DivBase uses to load TSVs on the client and server side.
• wrote user guide pages. Related pages on dimensions and VCF queries is a TODO that will be covered in its own branch(es).

Sidecar metadata TSV format and filtering syntax

To not repeat what I wrote in the user guide, I would suggest spinning up mkdocs and reading: http://localhost:8008/divbase/user-guides/sidecar-metadata/

Also updated the quick-start guide, but there is really no room there to discuss the metadata format and query syntax there.

Create pre-filled template

The first column in the TSV, Sample_ID is the only column that we stricly enforce since it needs to match exactly with the sample IDs in the VCFs in the project. To help users get this column right, I added a CLI command that reads the dimensions index of the project and generates at TSV with this first column populated with the sample names.

For instance, if local-project-1 has files as per local_dev_setup.py and divbase-cli dimensions update --project local-project-1 has been run:

divbase-cli dimensions create-metadata-template 

will save a single-column TSV with all unique sample names from the dimensions file to sample_metadata_<project_name>.tsv. Output filename can be controlled with --output.

TSV file Validation

The TSV validator runs client-side and is intended to be run on local TSV files before they are uploaded to DivBase. It validates the content in the TSV; checks on the query filters are done elsewhere. The validator does make one request to the server to fetch a list of the samples from the project's dimensions index to be able to validate sample names. Therefore I sorted the CLI command under divbase-cli dimensions just like the TSV template creator.

We talked about having Pandas in the libs to be able to ensure that the same code can be used client-side and server-side for doing the same checks. To my surpise, Pandas was already in the libs package... I could have sworn that it was in the api package when I looked last week! Well, anyway, that made it an easy choice to refactor all shared code to a class in the libs package.

For local-project-1 after its dimensions index has been updated:

# Should give a warning about mixed-type column, but should otherwise be fine:
divbase-cli dimensions validate-metadata-file tests/fixtures/sample_metadata.tsv --project local-project-1

# Intentionally, no sample names match dimensions here, should return a lot of warnings and errors:
divbase-cli dimensions validate-metadata-file tests/fixtures/sample_metadata_incorrect_formatting_to_test_tsv_validator.tsv --project local-project-1

Metadata queries

In addition to the shared TSV content validator that is also run by SidecarQueryManager when a metadata query begins processing, there are also checks for the filter processing. For instance they e.g. check if numerical filters can be applied or not. This part of the code does not raise errors, only warnings. The user will typically get metadata query results but with warnings.

[brinkdp]

Thanks for the feedback on this here and in person, @RMCrean . After ruminating on and trying out your suggestion of using Python list style arrays and ast.literal_eval instead, I think this is a stronger strategy. The query syntax consequently has stricter error handling for multi-value cells, and I think that is for the better. I've updated the user guide to describe the new syntax rules.

After implementing the bracket arrays, I decided to work on simplifying this part of the code so that it will be easier for us to maintain in the future and easier for you to overview in the review.

I did not work on the centralised task exception handling yet. Perhaps that would be better suited for its own branch?

SharedMetadataValidator

After this refactoring, SharedMetadataValidator handles all processing of the TSV, including loading it to dataframe and running validation. The resulting MetadataValidationResult is a nested dataclass of the final dataframe, stats, and aggregated warnings and errors. The design principle here is that SharedMetadataValidator does not raise errors or report warnings: it only collects them. Then the CLI and query engine each act on that information for their own purposes.

Warnings and errors detected by SharedMetadataValidator are now categorized by Enums in addition to storing the messages. For instance FILE_READ for errors reading the TSV file, SAMPLE_ID_VALUE for errors related to the cells in the sample ID column, etc. This way, the server-side SidecarQueryManager can make decisions on the Enums and not by string-matching the messages, whereas the client-side CLI validator can just print the messages to the user. This should be well needed robustness for the pilot user phase where we might get feedback and change message phrasing. This already help me several times during the refactoring.

The warnings and errors are now aggregated per column instead per row to create more compact messages. For instance, an error like - Column 'Population': Found 2 cell(s) ... Affected cells: Row 8 ('[1, 'three', 5]'), Row 14 ('[12, 'one', 68]') is now a single message and not 2 messages (one per cell) like it was before.

Bracket array implementation

SharedMetadataValidator also handles the parsing of bracket array string literals from the TSV to Python list objects in the dataframe. It is done in SharedMetadataValidator._parse_list_cells_in_dataframe after the TSV is loaded by pd.read_csv. It evaluates all cells that starts with "[" with ast.literal_eval.

If the evaluation passes, it applies a reformatting of the string literal to a Python list to the dataframe. If no errors are raised by ast.literal_eval along the way, this updated dataframe becomes the source-of-thruth that will be stored in the MetadataValidationResult and used for all remaining validation, and by SidecarQueryManager when it applies the query filters.

If ast.literal_eval fails, the cell is untouched (not converted to list), an LIST_SYNTAX Enum error is collected for that cell, and the cell is indexed in self._cells_with_hard_errors so that downstream checks can skip it to avoid edge-cases. The TSV errors will need to be fixed by the user anyway.

MetadataValidationResult consumers: CLI validator and Query engine

CLI validator: divbase-cli dimensions validate-metadata-file.
The CLI client-side validator class was removed in this refactoring since it was mostly a thin wrapper around the shared validator. The stats calculation that was in that class before has been moved to SharedMetadataValidator.

Server-side query engine:
It is worth noting that SidecarQueryManager does two things:

1. It runs the shared validator in the method .load_file(). Based on the MetadataValidationResult from SharedMetadataValidator, it raises errors and reports warnings about the TSV. If the validation results had no errors, it stores the dataframe from MetadataValidationResult in self.df so that it can be used in step 2.
2. It parses the query filter strings from the user CLI input (e.g. divbase-cli query tsv "Area: North, South") with .run_query and applies it to the validated dataframe in self.df . This has its own separate logic and does not call SharedMetadataValidator.

Commands

This TSV fixture is designed to be a vertical slice of the metadata TSV error and warning handler:

# Client-side validator
divbase-cli dimensions validate-metadata-file tests/fixtures/sample_metadata_incorrect_formatting_to_test_tsv_validator.tsv

If you run this against the project you uploaded the 5000 sample mock data to with the commands in PR #70, you should hopefully see how the errors and warnings truncates the sample mismatches. Add the --untruncated option to see the full 5000 samples. Works for all validator results that get truncated (not just sample names)

The mock metadata generator was also updated to accommodate the refactoring. If you want, you can regenerate the mock TSV with bracket array multi-values and validate it against the project dimensions where you put the mock 5000 sample VCF.

python scripts/generate_mock_sample_metadata.py --vcf mock_vcf_5000s_100r.vcf.gz --output mock_metadata_mock_vcf_5000s_100r.tsv --columns 25 --add-warning-column

divbase-cli dimensions validate-metadata-file mock_metadata_mock_vcf_5000s_100r.tsv --project <PROJECT_WHERE_THE_MOCK_5000_DATA_IS>

# Can also upload it to the bucket. Not needed for the client side validator, but for queries.
divbase-cli files upload  mock_metadata_mock_vcf_5000s_100r.tsv --project <PROJECT_WHERE_THE_MOCK_5000_DATA_IS>

[Copilot]

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

packages/divbase-api/src/divbase_api/worker/tasks.py:204

• sample_metadata_query_task calls latest_version_of_all_files() and _check_that_dimensions_is_up_to_date_with_VCF_files_in_bucket() twice back-to-back with identical arguments. This duplicates an S3 listing call (potentially expensive) and can double the runtime of every metadata query task. Remove the duplicated second call, or document why the check must run twice (it doesn’t appear to here).

    latest_versions_of_bucket_files = s3_file_manager.latest_version_of_all_files(bucket_name=bucket_name)
    _check_that_dimensions_is_up_to_date_with_VCF_files_in_bucket(
        vcf_dimensions_data=vcf_dimensions_data,
        latest_versions_of_bucket_files=latest_versions_of_bucket_files,
        project_id=project_id,
    )

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[RMCrean]

Awesome job with these changes! 👍 I've tried to stress test the system. I really like the validator aspect of the sample metadata and good to know this approach scales fine timing wise for relatively big sample metadata files. Sounds good to focus on the exception handling another time.

One thing that I realized now when I was playing around (maybe not from this PR per say), but I feel like it is was worth trying to improve:

divbase-cli dimensions update
divbase-cli query bcftools-pipe --tsv-filter "Area:Northern Portugal" --command "view -s SAMPLES" --metadata-tsv-name sample_metadata.tsv
# job works and creates output
divbase-cli query bcftools-pipe --tsv-filter "Area:Northern Portugal" --command "view -s SAMPLES" --metadata-tsv-name sample_metadata.tsv
# job fails as previous job's results files creates error as dimensions update needs to be run again.
# error message: 
The following VCF files or file  versions in the project are not part of the project's VCF dimensions: 'result_of_job_29.vcf.gz' Please run 'divbase-cli dimensions update --project <project_name>' and then submit the query again.  
# running dimensions update now will make will make the next query work again.  

My understanding is dimensions update is adding the results files to the class SkippedVCFDB(BaseDBModel). An alternative solution might be to modify _check_that_dimensions_is_up_to_date_with_VCF_files_in_bucket fn to just auto skip all files that start with the QUERY_RESULTS_FILE_PREFIX. Then dimensions update doesn't need to be run in between jobs. Not sure if there are any other files that are being included in skipped table other than results files?

[RMCrean]

Question: Why return all records for invalid conditions? Would it not be better to reject the request and say it had invalid formatting?

I'm thinking a user half reading the output (especially if large) or using the output in a pipeline would not necessarily see this warning

[brinkdp]

Oh, that is a remnant from the previous iteration that relied on soft warnings instead of errors. It was also never tested at scale with the 5000 sample file. I completely agree that it would be better like you suggested. Updated in eb84afc.

Now a query on an non-existent column returns an SidecarInvalidFilterError. E.g. divbase-cli query tsv "NonExistentColumn:value"

[brinkdp]

Awesome, thanks for all the feedback on this. It has really made this a much better implementation!

Good that you noticed that behaviour with results files and the dimensions index. It was not intended to behave like that, but perhaps I never really tested submitting back-to-back jobs? I implemented your suggestions and added an e2e test for this.

About the result file checks:
I think the QUERY_RESULTS_FILE_PREFIX is good first gatekeeping layer for not having result files end up in queries.
There is also a second layer to catch the case when the results file has been renamed: the results files are "branded" with a custom line in the header that says that it is DivBase-generated. If the dimensions files sees such a file, it will not index it and record it in the SkippedVCFDB table. Files that are in that table will not be considered for bcftools queries. With these two layers, I think that we will stop most cases of results files being considered for queries. A user would have both rename the file and modify the header to get around these checks.

Thanks for spotting this. These are exactly the things we need to identify right now!

Will merge now 🚀