Implement VCF bundles. #1703

cmnbroad · 2024-01-23T21:16:59Z

Draft until #1702 goes in.

lbergelson

@cmnbroad I'm pretty sure the resolve index method is completely broken. I think either delete it or add some tests.

I'm finding it very confusing to differentiate where to use VARIANT_CONTEXTS vs VARIANTS_VCF. Maybe we should change the naming or put them in different classes? Alternatively, it might be less confusing if we added VARIANTS_BCF and constants for the different indexes? It's not super important but I found it confusing, especially in the tests where in some places you use VARIANTS_VCF to specify the extension while VARIANTS_INDEX is different. We can definitely punt on it in this pr.

Other than that, intellij found a few minor suggestions.

lbergelson · 2024-01-24T18:20:14Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ * this type.
+ */
+public class VariantsBundle<T extends IOPath> extends Bundle implements Serializable {
+ private static final long serialVersionUID = 1L;


It's probably good practice to start annotating these with @Serial.

Done (on the variable itself - though I'm still not sure exactly what that does).

It throws a compiler error if the variable or method isn't actually part of the serialization system. It prevents things like serialVersionID which has been a problem for us back in the spark days. Annoyingly hard to notice. Sort of like the Override annotation, just a helpful sanity check.

lbergelson · 2024-01-24T18:21:16Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ * for variants sources that are backed by streams or otherv{@link BundleResource} types, the {@link Bundle} and
+ * {@link BundleBuilder} classes can be used to construct such bundles directly.
+ *
+ * @param <T> The type to use when creating a new IOPathResource for a {@link htsjdk.beta.plugin.variants.VariantsBundle}.


Maybe someday java will add self typed calls.

lbergelson · 2024-01-24T18:22:15Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ * @param vcfPath An {@link IOPath}-derived object that represents a source of variants.
+ */
+ public VariantsBundle(final T vcfPath) {
+ this(Arrays.asList(toInputResource(


We're free from the tyranny of Arrays.asLis()! List.of(), unless of course that causes some horrible confusing serialization error...

lbergelson · 2024-01-24T18:28:31Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ */
+ public VariantsBundle(final T vcfPath, final T indexPath) {
+ this(Arrays.asList(
+ toInputResource(BundleResourceType.VARIANT_CONTEXTS,


toInputResource() is called with this verbose nonNull construct everywhere. It looks like that could be just moved into the method itself since the first parameter seems to always want to be the same as error message name in the null check.

(I think in this case it actually should VARIANT_CONTEXT and not VARIANTS_VCF also. It seems inconsistent.)

It looks like there is an identical copy of this method in ReadsBundle that has the same unnecessary verbosity also. Maybe it should be pulled out into a utility

I got rid of the nonNull business -it turns out the called method already had that anyway. As for the reads one, its slightly different in that they each use different (content type-specific) getInferredContentTypes methods.

lbergelson · 2024-01-24T18:31:47Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ }
+
+ /**
+ * return the {@link BundleResourceType#VARIANTS_VCF} {@link BundleResource} for this {@link htsjdk.beta.plugin.variants.VariantsBundle}


Drop the duplicate comment line and just keep the `@returns'?

lbergelson · 2024-01-24T18:38:46Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ }
+
+ public static boolean looksLikeAVariantsBundle(final IOPath rawVCFPath) {
+ return rawVCFPath.getURI().getPath().endsWith(BundleJSON.BUNDLE_EXTENSION);


Shouldn't this use rawVCFPath.hasExtension()?

Yes. But I've removed this method until such a time as we actually need it (so far I don't).

lbergelson · 2024-01-24T18:43:25Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ // and correctly infer the types in all cases without reproducing all the logic embedded in all the
+ // codecs (i.e., an htsget IOPath ends in ".bam", but has format HTSGET_BAM, not BAM - detecting
+ // that here would require parsing the entire IOPath structure, which is best left to the codecs
+ // themselves). So for now its advisory, but maybe it should be abandoned altogether.


Seems like this check is better than nothing.

Retained this for now, since it does some checking, though it could do a lot more.

lbergelson · 2024-01-24T18:49:03Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ /**
+ * return the {@link BundleResourceType#VARIANTS_VCF} {@link BundleResource} for this {@link htsjdk.beta.plugin.variants.VariantsBundle}
+ *
+ * @return the {@link BundleResourceType#VARIANTS_VCF} {@link BundleResource} for this {@link htsjdk.beta.plugin.variants.VariantsBundle}


Should the doc say VARIANT_CONTEXTS instead off VARIANTS_VCF? There are few times it refers to requiring a resource with the type VARIANTs_VCF but the actual check seems to be for the more general VARIANTS_CONTEXT type instead of the VCF specifically.

Yes - those names are a bit of a mess. I'll update them.

lbergelson · 2024-01-24T18:51:38Z

src/test/java/htsjdk/beta/plugin/variants/VariantsBundleTest.java

+ "ALIGNED_READS":{"path":"my.cram","format":"READS_CRAM"},
+ "primary":"ALIGNED_READS"
+ }
+ """.formatted();


unnecessary format

Fixed. I replaced some of the static text in these cases with additional parameters anyway.

lbergelson · 2024-01-24T18:58:44Z

src/test/java/htsjdk/beta/plugin/variants/VariantsBundleTest.java

+ }
+ """.formatted(VCF_FILE, VCF_INDEX_FILE),
+ // VariantsBundle doesn't automatically infer format, so create one manually
+ new VariantsBundle(


VariantsBundle<>

I removed the type params from VariantsBundle altogether, since they really weren't needed anyway.

lbergelson

It seems pretty straight forward. Just a lot of it. I think it might makes ense to change the new utility method to be two different ones instead of one with a mode toggle. It's not a blocker though.

Another musing:
I wonder if t would make more sense to write the schema as:

schema: {verson: x, name: htsbundle} 

instead of:

schemaVersion: x
schemaName: htsbundle

Maybe for version x+1...

lbergelson · 2024-08-15T18:20:01Z

src/main/java/htsjdk/beta/io/IOPathUtils.java

+
+ /**
+ * Takes an IOPath and returns a new IOPath object that keeps the same basename as the original but has
+ * a new extension. If append is set to false, only the last component of an extension will be replaced.


only the last -> the last
I read that as implying that append == true will replace all of the components.

Oh I see. Will separate append/replace to eliminate confusion.

lbergelson · 2024-08-15T18:23:32Z

src/main/java/htsjdk/beta/io/IOPathUtils.java

+ * @param ioPathConstructor a function that takes a string and returns an IOPath-derived class of type <T>
+ * @return A new IOPath object with the new extension
+ */
+ public static <T extends IOPath> T replaceExtension(


As a favor to my future self seeing this fail at runtime, is there a reason we shouldn't make it accept it either with or without the dot?

Would it make sense split these into two different functions? replaceExtension() and appendExtension()?

Yeah, I like that better. I back ported this from one of Takuto's Picard PRs since I needed it. Also, I made him add tests for it ;-). Will separate these and add some tests.

Oh, wait I did add tests. Good for me. Now I have to update them.

lbergelson · 2024-08-15T18:38:35Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

+ String.format("A JSON string with more than one bundle was provided but only a single bundle is allowed in this context (%s)",
+ e.getMessage()));
+ }
+ return bundles.stream().findFirst().get();


this seems like a weird way to get the first element of a list. why not just bundles.get(0)? Or update to java 21 and just use bundles .getFirst() :)

Hm, yeah. Done.

lbergelson · 2024-08-15T18:41:45Z

src/main/java/htsjdk/beta/io/bundle/BundleJSON.java

 }
+ }
+ if (bundles.isEmpty()) {


I think we should refer to a collection of bundles as a bindle for added alliteration. Definitely won't make it more confusing.

lbergelson · 2024-08-15T18:47:11Z

src/main/java/htsjdk/beta/plugin/reads/ReadsBundle.java

 } else if (ext.equals((FileExtensions.SAM))) {
- return Optional.of(new Tuple<>(BundleResourceType.ALIGNED_READS, BundleResourceType.READS_SAM));
+ return Optional.of(new Tuple<>(BundleResourceType.CT_ALIGNED_READS, BundleResourceType.FMT_READS_SAM));
 }
 //TODO: finish this, else SRA, htsget,...


We can probably leave this as a todo for now.

Yeah, ReadsBundle needs some work since it was never really finished. But its not used by my current work, so I'll do that in a separate PR.

lbergelson · 2024-08-15T18:49:41Z

src/main/java/htsjdk/beta/plugin/variants/VariantsBundle.java

+ * this type.
+ */
+public class VariantsBundle<T extends IOPath> extends Bundle implements Serializable {
+ private static final long serialVersionUID = 1L;


It throws a compiler error if the variable or method isn't actually part of the serialization system. It prevents things like serialVersionID which has been a problem for us back in the spark days. Annoyingly hard to notice. Sort of like the Override annotation, just a helpful sanity check.

lbergelson · 2024-08-15T18:54:49Z

src/main/java/htsjdk/io/IOPath.java

@@ -77,6 +77,14 @@ default String getScheme() {
 return getURI().getScheme();
 }

+// default String resolveSibling(final String sibling) {


Should this be deleted?

Om, yeah, I have no idea why this is here. Deleted.

lbergelson · 2024-08-15T18:56:01Z

src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java

+ "schemaVersion":"%s",
+ "primary":"%s",
+ "%s":{"path":"%s","format":"BAM"}
+ }""".formatted(


The triple quotes string is so much more readable.

Next year in string interpolation!

lbergelson · 2024-08-15T18:57:57Z

src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java

@@ -194,49 +213,143 @@ public void testFromJSONValidWithPathOverride(final String jsonString, final Str
 });
 }

+ // Make sure that any JSON that is a valid single Bundle can also be parsed as a Bundle collection, so that users
+ // can provide an individual bundle to any client that expects a collection of bundles.


This requirement added complexity but I think it will save so much frustration.

cmnbroad marked this pull request as draft January 23, 2024 21:17

lbergelson requested changes Jan 24, 2024

View reviewed changes

cmnbroad force-pushed the cn_vcf_bundles branch 3 times, most recently from e418dc7 to 2c9878f Compare June 11, 2024 20:23

This was referenced Aug 6, 2024

Support reference bundles. #1713

Open

Bundle support for FeatureInputs, references; add CreateBundle tool. broadinstitute/gatk#8944

Draft

lbergelson approved these changes Aug 15, 2024

View reviewed changes

cmnbroad added 9 commits August 19, 2024 15:21

Update bundle content type names.

a822c58

Update ReadsBundleTest to use more defined constants.

48493b2

Update ReadsBundleTest to use more defined constants.

86ae11b

Implement VariantBundles.

cced91e

Update based on GATK usage and rebase.

b01d2e5

Code review comments part 1 - temp WIP.

7910a15

Updates after rebase on new content type strings.

f29a23a

More review comments.

edb4168

Code review comments.

e45cd78

cmnbroad force-pushed the cn_vcf_bundles branch from 2c9878f to e45cd78 Compare August 19, 2024 21:33

Move bundle JSON schema into a nested map.

3f702cf

cmnbroad marked this pull request as ready for review September 3, 2024 14:45

cmnbroad merged commit 46f6f0f into master Sep 3, 2024
4 checks passed

cmnbroad deleted the cn_vcf_bundles branch September 3, 2024 18:36

Implement VCF bundles. #1703

Implement VCF bundles. #1703

Conversation

cmnbroad commented Jan 23, 2024 • edited Loading

lbergelson 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson 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

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

cmnbroad commented Jan 23, 2024 •

edited

Loading