Fix: Uncapitalize pair fields in inputs json

[cjllanwarne]

Discovered by a user here: https://gatkforums.broadinstitute.org/wdl/discussion/12025/error-scattering-array-pair-string-string#latest

The fields to initialize a Pair in the inputs JSON should be left and right, not Left and Right

[geoffjentry]

@cjllanwarne Is the spec incorrect or is Cromwell incorrect?

[cjllanwarne]

I think whichever we choose (Left vs left and Right vs right) should be consistent throughout. Right now we have both cases in the spec and it's causing confusion:

• The inputs.json example using Left and Right
• Pair Indexing in WDL (which is .left and .right)

This feels like a typo to me rather than a conscious choice to have upper case only in the inputs JSON. Since Cromwell is already doing lowercase throughout and that's the "in WDL code" example, I'm advocating for that, but if people disagree, I'm open to the alternatives.

[geoffjentry]

@cjllanwarne Sorry I should have been more direct but you caught my drift. At the moment the spec says one thing but Cromwell is doing something else (and presumably other implementations as well). IMO that means that it's an implementation bug and thus not an automatic merge here.

I'd suggest drawing attention to this in order to assess:

• What other implementations are doing
• What the answer should be

And then go forward armed with that information.

[patmagee]

To be a bit controversion, I wonder if the syntax for Pair should even include left and right, or instead if we should just make it a two element tuple? I guess the major drawback to this solution is an inconsistency with the JSON representation, which does not support the () tuple:

Ie:

Pair[String,File] foo = ("bar", "file://biz")

String left = foo[0]
String right = foo[1]

[geoffjentry]

@patmagee as a bit of history and not stated to make any sort of point, when Pair was put in we were debating instead doing n-tuples. It was decided that the motivating use case (forget what it was) always was a 2-tuple and thus Pair worked, and in a future world if we were to add in n-tuples to WDL then Pair could always be turned into syntactic sugar for a 2-tuple

[cjllanwarne]

I agree whole-heartedly about there being a better way to handle tuples in general. Right now I just want the spec to:

• be consistent between inputs.json and WDL files
• match implementations

If this proposed scheme change doesn't actually match all implementations (ie if there's an engine which reads in Left and Right and people have started writing inputs files for it) then I don't want to break those!

If that is the case then maybe "case insensitive" in both cases is the least disruptive change here (because I don't want to break Cromwell users' inputs files and force them to change to Left and Right either!)

[patmagee]

@cjllanwarne coming back to this, I like the idea of restricing the pair definition to only use lowercase. uppercase seems a bit... odd.

Feel free to send this to voting

[patmagee]

@cjllanwarne so this did not really seem to go anywhere! But I still like this idea. I do wonder if this should be phased out, as opposed to just changed. Ie allow uppercase OR lowercase for the next release, thjen add a deprecation notice

[geoffjentry]

This feels like it's falling into the same "should there be an inputs json at all?" trap as #301 although no one has seemed to notice that, not even anti-inputs json stalwart @patmagee :) I hae the same stance here that I did there which is we should just proceed and if someone wants to change how inputs are done separately they can.

[pshapiro4broad]

Is Pair needed at all? WDL has struct, which lets you do what Pair does, although with a little more text. Looking at the WDLs I work on, there is no Pair in the "green" WDLs, and only a couple uses in the HCA WDLs. Was there a particular user or use case that advocated for this feature?

If WDL really needs Pair, I would advocate for lowercase left and right in the inputs json so the json syntax matches the WDL.

[DavyCats]

@pshapiro4broad Pairs could be useful for generating Maps through scatters (using as_map), which may be a nice feature when you are working on multisample case-control pipelines. Eg.:

# map all the samples
scatter (sample in samples) {
    call bwa #...
    Pair idAndBam = (sample.id, bwa.bam)
}

# create a mapping between the sample ids and their bam files
Map[String, File] idToBam = as_map(idAndBam)

# do some case-control analysis for the samples which have a control specified
scatter (sample in samples) {
    if (defined(sample.control) {
        call somatic_variant_calling {
            input: 
                # use the mapping made earlier to retrieve the appropriate 
                # BAM files for the sample and matched control
                case=idToBam[sample.id], 
                control=idToBam[sample.control]
        }
    }
}

[patmagee]

@geoffjentry @cjllanwarne I have opened this for voting, but I parly feel like this can just be merged as is as a typo fix. WDYT?

[orodeh]

👍

[patmagee]

👍

[DavyCats]

@geoffjentry @patmagee Isn't this already implemented in cromwell?

[patmagee]

@cjllanwarne can you please rebase? (if you do not have time I can look into it)

[cjllanwarne]

Merging as approved and implemented, now that the merge conflict has been resolved