-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix many issues with examples #669
base: wdl-1.1
Are you sure you want to change the base?
Conversation
spec for map types
@stxue1 @adamnovak would also appreciate your reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like everything should work.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means the directory in the execution environment where the file is to be written must have writable permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
File infile | ||
} | ||
|
||
String path1 = "~{infile}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
"import_structs.bill": 175000 | ||
"import_structs.bill": 175000.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON doesn't make a distinction between int and float. The test framework should compare these values as equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
SPEC.md
Outdated
hisat2 \ | ||
-p ~{threads} \ | ||
~{if defined(max_reads) then "-u ~{select_first([max_reads])}" else ""} \ | ||
-x index/~{index_id} \ | ||
-x index/grch38/genome \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
-x index/grch38/genome \ | |
-x index/"$(echo ~{index_id} | tr '_' '/')" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
Array[Array[Int]] out = transpose(input_array) | ||
Array[Array[Int]] expected = expected_output_array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
|
||
output { | ||
Boolean paths_equal = path2 == path3 | ||
Boolean paths_equal = infile == path3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path3
is undefined, I think it should be replaced with path2
?
Closes #653
Closes #654
Closes #661
Closes #662
Closes #663
Closes #664
Closes #665
Closes #666
Closes #667
Closes #668
Checklist