Fix many issues with examples #669

jdidion · 2024-06-26T01:15:53Z

Closes #653
Closes #654
Closes #661
Closes #662
Closes #663
Closes #664
Closes #665
Closes #666
Closes #667
Closes #668

Checklist

Pull request details were added to CHANGELOG.md

spec for map types

Various fixes for WDL examples

jdidion · 2024-07-24T23:40:06Z

@stxue1 @adamnovak would also appreciate your reviews

adamnovak

This looks like everything should work.

adamnovak · 2024-07-25T20:03:56Z

SPEC.md

@@ -534,7 +535,12 @@ The following primitive types exist in WDL:
 * A `File` represents a file (or file-like object).
    * A `File` declaration can have a string value indicating a relative or absolute path on the local file system.
    * Within a WDL file, literal values for files may only be local (relative or absolute) paths.
+    * The path assigned to a `File` is not required to be valid unless and until it is accessed.
+      * To read from a file, it must exist and be assigned appropriate permissions.
+      * To write to a file, the parent directory must be assigned appropriate permissions.


WDL File objects are snapshots, right? Any WDL function that writes a File returns the written File and doesn't take a File as input to overwrite.

Or does this mean within a task, if you substitute a File's name into a command that tries to overwrite it, the parent directory that the File initially came from should be writable?

It means the directory in the execution environment where the file is to be written must have writable permissions.

For example, if I define File outfile = "outfile" and then in the command do printf "hello" > ~{outfile}, the directory where the execution engine resolves outfile (presumably, the working directory, but it doesn't have to be) must be writable.

There is not really any reason to use a File for this purpose, and (obviously) it adds a lot of confusion. Hence the impetus to disallow relative paths.

adamnovak · 2024-07-25T20:07:59Z

SPEC.md

+    File infile
+  }
+
+  String path1 = "~{infile}"


This makes the test now depend on file to string conversion as well as string to file conversion. But I don't think the test framework lets you set up actual file fixtures to make this work a different way.

Or could we just use hello.txt, since it's available to use in the input JSON?

SPEC.md

adamnovak · 2024-07-25T20:22:32Z

SPEC.md

-  "import_structs.bill": 175000
+  "import_structs.bill": 175000.0


At least under the interpretation of JSON that JSON Schema uses, these are the JSON value.

Are we expecting the runner to produce one serialization here and not the other? Or @stxue1 are you doing something that needs to key on actually having a decimal point?

JSON doesn't make a distinction between int and float. The test framework should compare these values as equal.

Are we expecting the runner to produce one serialization here and not the other? Or @stxue1 are you doing something that needs to key on actually having a decimal point?

I currently do try to key on the decimal point to test for floats, but since the JSON schema doesn't actually support a distinction for floats and ints, there isn't much reason to actually key on decimal points, so I'll remove the check from our test suite.

adamnovak · 2024-07-25T20:30:15Z

SPEC.md

    hisat2 \
      -p ~{threads} \
      ~{if defined(max_reads) then "-u ~{select_first([max_reads])}" else ""} \
-      -x index/~{index_id} \
+      -x index/grch38/genome \


This looks like we're hardcoding for the name of the genome that actually happens to be in this file; we also no longer use index_id at all.

Maybe we meant to generate this string from the filename?

Suggested change

-x index/grch38/genome \

-x index/"$(echo ~{index_id} | tr '_' '/')" \

I'm guessing the tgz file contains the reference nested two levels deep. Rather than hard-code the path, I used the --strip-components 2 argument to tar.

stxue1 · 2024-07-26T17:33:00Z

SPEC.md

+    Array[Array[Int]] out = transpose(input_array)
+    Array[Array[Int]] expected = expected_output_array


The example output section is missing the 2 new outputs.
The example output json below is currently defined as:

{ "test_transpose.is_true": true }

and should become

{ "test_transpose.expected": [[0, 3], [1, 4], [2, 5]], "test_transpose.out": [[0, 3], [1, 4], [2, 5]], "test_transpose.is_true": true }

stxue1 · 2024-07-26T18:05:04Z

SPEC.md


  output {
-    Boolean paths_equal = path2 == path3
+    Boolean paths_equal = infile == path3


path3 is undefined, I think it should be replaced with path2?

stxue1 · 2024-09-30T20:17:57Z

SPEC.md

@@ -3916,7 +3938,9 @@ Example output:
 ```json
 {
  "optional_output.example2": null,
-  "optional_output.file_array_len": 1
+  "optional_output.file_array_len": 1,
+  "optional_output.example1": "example1.txt",


The exclude_output section below specifies both of these outputs to not be validated. Is this intentional or should one or the other be removed?

The unit tests with ids outputs, glob, relative_and_absolute, change_extension, and gen_files all omit the outputs when specified in exclude_output. I think one or the other should be removed here.

stxue1 · 2024-09-30T22:11:16Z

The unit test test_hints wc command is malformed:

wdl/SPEC.md

Line 4584 in 20ff54d

wc -l ~{foo}

And should be:

  wc -l < ~{foo}

The expected output should change from 3 to 2 as well:

wdl/SPEC.md

Line 4619 in 20ff54d

"test_hints.num_lines": 3

{
  "test_hints.num_lines": 2
}

stxue1 · 2024-09-30T22:23:46Z

input_hint is missing an output:

wdl/SPEC.md

Lines 4703 to 4705 in 20ff54d

I'm not too certain what the expected output should be. MiniWDL currently returns

{
    "input_hint.experience": []
}

stxue1 and others added 10 commits June 18, 2024 19:52

Add fixes

00a75d5

update versions

b0b0713

Update SPEC.md (#660)

6401dfa

Specify container with lspci for GPU WDL example. (#659)

5f139ec

fix #661

28123ee

update changelog, fix #662

0983069

update changelog, fix #663

4535d55

update changelog, fix #664

71070a0

update changelog, fix #665, #666

c5c7c0f

update changelog, fix #668

caff59d

jdidion changed the title ~~Fix may issues with tests~~ Fix may issues with examples Jun 26, 2024

jdidion and others added 8 commits June 26, 2024 12:59

fix #667

6453a92

update changelog

59bbd40

Various fixes to get most of the tests running

41ffadd

Move infile to input

4935071

Bring back whitespace as that should be supported by the spec

7ea277d

Bring back whitespace as that should be supported by the spec

28f3503

Convert string back to int as coercion is apparenlty not part of the

e396d64

spec for map types

Merge branch 'wdl-1.1-fixes' of github.com:stxue1/wdl into wdl-1.1-fixes

ae73a6f

stxue1 mentioned this pull request Jul 3, 2024

Various fixes for WDL examples #674

Merged

2 tasks

stxue1 and others added 7 commits July 2, 2024 18:54

Revert read_boolean changes

c5a44a5

671: fix python json serialization of string list

d2a8b9e

update readme

c0cd737

Merge branch 'wdl-1.1.3' into wdl-1.1-fixes

8da3e0a

Merge pull request #674 from stxue1/wdl-1.1-fixes

82acf71

Various fixes for WDL examples

fix

81dc127

fix

1626777

jdidion requested review from wleepang and geoffjentry July 24, 2024 23:40

jdidion requested review from vsmalladi, patmagee, cjllanwarne, markjschreiber and rhpvorderman July 24, 2024 23:40

adamnovak approved these changes Jul 25, 2024

View reviewed changes

use index_id rather than hard-coded path, add missing variable

20ff54d

stxue1 reviewed Jul 26, 2024

View reviewed changes

jdidion changed the title ~~Fix may issues with examples~~ Fix many issues with examples Jul 26, 2024

adamnovak mentioned this pull request Jul 30, 2024

Removing whitespace in WDL doesn't work for overindented code blocks DataBiosphere/toil#4983

Closed

stxue1 reviewed Sep 30, 2024

View reviewed changes

stxue1 mentioned this pull request Nov 1, 2024

Apply some fixes to WDL examples #689

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix many issues with examples #669

Fix many issues with examples #669

jdidion commented Jun 26, 2024 •

edited

Loading

jdidion commented Jul 24, 2024

adamnovak left a comment

adamnovak Jul 25, 2024

jdidion Jul 25, 2024

jdidion Jul 25, 2024 •

edited

Loading

jdidion Jul 26, 2024

adamnovak Jul 25, 2024

adamnovak Jul 25, 2024

jdidion Jul 26, 2024

stxue1 Jul 26, 2024

adamnovak Jul 25, 2024

jdidion Jul 26, 2024

stxue1 Jul 26, 2024 •

edited

Loading

stxue1 Jul 26, 2024

stxue1 Sep 30, 2024

stxue1 Sep 30, 2024

stxue1 commented Sep 30, 2024 •

edited

Loading

stxue1 commented Sep 30, 2024

		"import_structs.bill": 175000
		"import_structs.bill": 175000.0

	-x index/grch38/genome \
	-x index/"$(echo ~{index_id} \| tr '_' '/')" \

		Array[Array[Int]] out = transpose(input_array)
		Array[Array[Int]] expected = expected_output_array

Fix many issues with examples #669

Are you sure you want to change the base?

Fix many issues with examples #669

Conversation

jdidion commented Jun 26, 2024 • edited Loading

Checklist

jdidion commented Jul 24, 2024

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdidion Jul 25, 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

stxue1 Jul 26, 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

stxue1 commented Sep 30, 2024 • edited Loading

stxue1 commented Sep 30, 2024

jdidion commented Jun 26, 2024 •

edited

Loading

jdidion Jul 25, 2024 •

edited

Loading

stxue1 Jul 26, 2024 •

edited

Loading

stxue1 commented Sep 30, 2024 •

edited

Loading