[query] Teach hail to export a VDS to an SVCR-VCF #14743

chrisvittal · 2024-10-23T18:37:48Z

Usage and notes in the documentation for the new methods.

Part of #14655.

Security Assessment

This change has no security impact

Impact Description

Does not increase attack surface. New methods only call into old ones.

ehigham · 2024-10-24T15:29:16Z

hail/python/hail/vds/impex.py

+    dataset : :class:`.MatrixTable`
+        Dataset.


Isn't it a VariantDataset?

ehigham · 2024-10-24T15:40:58Z

hail/python/hail/vds/impex.py

+        ref = ref.drop('END')
+
+    if 'gvcf_info' in var.entry and isinstance(var.gvcf_info.dtype, tstruct):
+        warning(


Do you need to warn here? You've documented this above and given an example for how to preserve this information.

I view warnings as a way of telling users about changing behaviour or when they've been a bit naughty (but not terrible) and this doesn't really fall into that. What's your view?

ehigham · 2024-10-24T15:42:54Z

hail/python/hail/vds/impex.py

+    if 'LGT' in var.entry:
+        if 'GT' not in var.entry:
+            var = var.annotate_entries(GT=lgt_to_gt(var.LGT, var.LA))
+        var = var.drop('LGT')


Can you comment on dropping these fields if GT/PT are present? Is that safe to do?

Yes. It's a data error if lgt_to_gt(var.LGT, var.LA) != var.GT.

I'm thinking about writing the reverse (gt_to_lgt). Not entirely sure why one would use it, which is what's stopping me.

ehigham · 2024-10-24T15:59:28Z

hail/python/hail/vds/impex.py

+    _create_row_uids=bool,
+    _create_col_uids=bool,
+)
+def import_vcf(


would it make sense to call this import_svcr to make it distinct from the extant import_vcf?

Yes. But I like the symmetry.

I think import_vcf is actually more correct. We use "svcr" to refer to the logical representation, of which "vds" is our native implementation. The new vcf standard now supports an svcr-like representation, which is what this imports from.

ehigham · 2024-10-24T16:02:55Z

hail/python/hail/vds/impex.py

+    ----
+    The following validations and transformations take place:
+
+    #. The ``LEN`` FORMAT field must exist and be of type :py:data:`.tint32`


Should the types listed in FORMAT and INFO sections be vcf types rather than hail types?

ehigham · 2024-10-24T16:05:04Z

hail/python/hail/vds/impex.py

+    The following validations and transformations take place:
+
+    #. The ``LEN`` FORMAT field must exist and be of type :py:data:`.tint32`
+    #. One of ``GT`` or ``LGT`` must be a FORMAT field of type :py:data:`.tcall`


Should LGT exist given you rename/drop it in export?

I don't want to preclude it from existing, it's well formed to have it.

ehigham · 2024-10-24T16:06:33Z

hail/python/hail/vds/impex.py

+    Parameters
+    ----------
+    path : :class:`str` or :obj:`list` of :obj:`str`
+        One or more paths to VCF files to read. Each path may or may not include glob expressions


Which is it lol?

Good point, will fix both this and import_vcf

ehigham

Mostly a review on the documentation for new public-facing functionality. Your implementation looks good. It's nice how much existing functionality you could use.

Usage and notes in the documentation for the new methods.

ehigham

Looks good. Thanks for making the changes.

patrick-schultz

This looks great, nice and clean! Just have a few simple questions and typos.

patrick-schultz · 2024-10-24T17:39:49Z

hail/python/hail/vds/impex.py

+    if 'END' in ref.entry:
+        ref = ref.drop('END')


I forget, does VDS now have a manditory LEN field, and optional END field?

patrick-schultz · 2024-10-24T17:51:15Z

hail/python/hail/vds/impex.py

+        'information."\n'
+    )
+    if VariantDataset.ref_block_max_length_field in ref.globals:
+        rbml = hl.eval(ref[VariantDataset.ref_block_max_length_field])


This could potentially be bad if the VDS isn't coming directly from disk, though we'd hopefully be able to optimize this read of a global field to not do much work. Do you think we should recommend writing before exporting?

I don't think it's necessary. We do optimize away the entire computation if we can simply read one global field. We're not that bad.

I'm not certain that's always guaranteed (what if we add a global field as the result of an aggregation?), but I think you're right that it would be rare enough we should scare people into writing when it isn't necessary, especially in this case when we can be pretty certain the max length field comes from disk.

hail/python/hail/vds/impex.py

patrick-schultz · 2024-10-30T14:19:10Z

hail/python/hail/vds/impex.py

+    _create_row_uids=bool,
+    _create_col_uids=bool,
+)
+def import_vcf(


I think import_vcf is actually more correct. We use "svcr" to refer to the logical representation, of which "vds" is our native implementation. The new vcf standard now supports an svcr-like representation, which is what this imports from.

patrick-schultz · 2024-10-30T15:01:34Z

hail/python/test/hail/vds/test_vds.py

+    assert 'LAA' in format_md, format_md
+    assert 'LA' not in format_md, format_md
+    assert 'gvcf_info' not in format_md, format_md
+    # LPGT is in this VDS, so we can check for PGT existing and LPGT not existing


Is this comment backwards?

no, but it's clearly unclear

Co-authored-by: Patrick Schultz <[email protected]>

patrick-schultz

Thanks Chris!

chrisvittal requested review from patrick-schultz and ehigham October 23, 2024 18:37

chrisvittal mentioned this pull request Oct 23, 2024

[query] Support VCF v4.5 #14655

Open

7 tasks

ehigham reviewed Oct 24, 2024

View reviewed changes

ehigham requested changes Oct 24, 2024

View reviewed changes

chrisvittal added 4 commits October 25, 2024 14:26

[query] Teach hail to export a VDS to an SVCR-VCF

1234dd6

Usage and notes in the documentation for the new methods.

lint

baed31b

address review comments

0451668

parse ref_block_max_length meta line from header

67602a0

chrisvittal force-pushed the vds/export-vcf branch from ba167ab to 67602a0 Compare October 25, 2024 18:27

chrisvittal requested a review from ehigham October 25, 2024 18:28

more review comments

747eae6

ehigham approved these changes Oct 28, 2024

View reviewed changes

patrick-schultz requested changes Oct 30, 2024

View reviewed changes

chrisvittal and others added 2 commits October 30, 2024 10:51

Apply suggestions from code review

308510c

Co-authored-by: Patrick Schultz <[email protected]>

fix note

32d2f4b

chrisvittal requested a review from patrick-schultz October 30, 2024 15:52

patrick-schultz approved these changes Oct 30, 2024

View reviewed changes

hail-ci-robot merged commit 056d9f2 into hail-is:main Oct 30, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Teach hail to export a VDS to an SVCR-VCF #14743

[query] Teach hail to export a VDS to an SVCR-VCF #14743

chrisvittal commented Oct 23, 2024

ehigham Oct 24, 2024

ehigham Oct 24, 2024

ehigham Oct 24, 2024 •

edited

Loading

chrisvittal Oct 24, 2024

ehigham Oct 24, 2024

chrisvittal Oct 24, 2024

patrick-schultz Oct 30, 2024

ehigham Oct 24, 2024

ehigham Oct 24, 2024

chrisvittal Oct 24, 2024

ehigham Oct 24, 2024

chrisvittal Oct 24, 2024

ehigham left a comment

ehigham left a comment

patrick-schultz left a comment

patrick-schultz Oct 24, 2024

chrisvittal Oct 30, 2024

patrick-schultz Oct 24, 2024

chrisvittal Oct 30, 2024

patrick-schultz Oct 30, 2024

patrick-schultz Oct 30, 2024

patrick-schultz Oct 30, 2024

chrisvittal Oct 30, 2024

patrick-schultz left a comment

[query] Teach hail to export a VDS to an SVCR-VCF #14743

[query] Teach hail to export a VDS to an SVCR-VCF #14743

Conversation

chrisvittal commented Oct 23, 2024

Security Assessment

Impact Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehigham Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehigham left a comment

Choose a reason for hiding this comment

ehigham left a comment

Choose a reason for hiding this comment

patrick-schultz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-schultz left a comment

Choose a reason for hiding this comment

ehigham Oct 24, 2024 •

edited

Loading