Handle WDL structs #97
Conversation
| struct = Struct(name=str(ctx.Identifier())) | ||
| for decl in ctx.unbound_decls(): | ||
| struct.fields.append([str(decl.Identifier()), decl.wdl_type().getText()]) | ||
| self.structs.append(struct) |
There was a problem hiding this comment.
So the core of this change depends on saving the structs when visiting the WDL abstract tree. An alternative would be to fetch them when/if necessary from the tree (not from ast), but I opted for this approach. Happy to change if needed.
| @@ -396,7 +394,7 @@ def get_command( | |||
| return new_command | |||
|
|
|||
|
|
|||
| def get_output(expression: str, input_names: List[str]) -> str: | |||
| def get_expression_output(expression: str, input_names: List[str]) -> str: | |||
| """Get expression for outputs.""" | |||
There was a problem hiding this comment.
I was confused at first as I thought get_output was to get_input, but apparently it did something slightly different (I am not familiar with expressions for outputs as in the docs). I thought it would be OK to rename it and create a get_output that does something similar to get_input?
wdl2cwl/main.py
Outdated
| inputs.append(cwl.CommandInputParameter(id=input_name, type=record_schemas.get(i[0]))) | ||
|
|
||
| else: | ||
| raise ValueError(f"Failed to get input type: {i[0]}") |
There was a problem hiding this comment.
Now raising an error if we know we cannot handle the input type (also the output in another function).
There was a problem hiding this comment.
Some unit tests failed, so I reverted this change. Added the elif that checks if it is a struct before the else block, this last one is handling the types in the same way as before. Only addition will be the extra elif key in structs...
| raise ValueError(f"Failed to get output type: {i[0]}") | ||
|
|
||
| return outputs | ||
|
|
There was a problem hiding this comment.
This ☝️ came from the convert method.
|
(fixing linter & some unit tests…) |
kinow
force-pushed
the
references
branch
2 times, most recently
from
fa15194 to
f7db6d7
Compare
Codecov Report
@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 94.78% 94.13% -0.65%
==========================================
Files 4 4
Lines 1380 1399 +19
==========================================
+ Hits 1308 1317 +9
- Misses 72 82 +10
Continue to review full report at Codecov.
|
kinow
force-pushed
the
references
branch
2 times, most recently
from
2c87b2a to
22bb6f7
Compare
Hi @mr-c ! Yes, please! I was actually going to investigate later how the project is tested, whether workflows are executed, or if we need to write unit tests :) If there's an existing test somewhere, or notes on how to write tests, I can take a look and see if I can follow/use as reference. Thanks! |
Hoopla!
I just merged #94 To make it easier on yourself, feel free to substitute a fake command in the WDL description
|
Also rename the get_output function as it was not equivalent to get_input. Then move code that was creating CWL outputs to get_output (more similar to get_input).
|
I think I understood how to write a test. I have one almost ready, just pending the expected output and learning how to run it to test locally. But it has already identified one issue with my code :-) I was using the second token of @mr-c I will push a WIP commit with my test, but will fix it later. I think I found a couple other issues that are not related to structs. Will see if there are any existing issues, if not will submit new ones 👍 |
Sorry, I should have been more clear. We have two major classes of tests: 1) comparing with stored conversion results and 2) actually running the CWL version with real inputs and comparing with known good outputs For 1) see https://github.com/common-workflow-lab/wdl-cwl-translator#adding-test-cases and for 2) see 96c7c6a#diff-002fd7337e44ea597c874d9b564acf1cfe36a4a7c2fcf3d0fe9cf41c748214cfR8 |
Ah! My bad, I started reading the readme a couple days ago but never finished. Went as far as the part about testing with an existing WDL 😅 I should have revisited it later. So what I have is a test like 2). Which option would be more appropriate for this PR? Thanks! |
|
Created the other two issues @mr-c . Done for the day, but learned a little more about CWL & WDL. My second issue could be something really silly that I didn't understand when using the translator, feel free to point if I missed anything 👍 Thanks! |
Option 1 is mandatory. Option 2 is appreciated! |
|
Closing as this needs to be redone from scratch given the refactoring. Hopefully it will be easier the 2nd time! |
Closes #72
Related to #68 too.
N.B.: pending tests, but a pre-review before I write the tests to confirm it's going in the right direction is welcome 😬
This change has minor cosmetics changes (unused imports, typos), and addresses the #72 as follows:
Struct, but happy to create a dict/tuple/normal object/etc if it's betterStructinto a dictionary withkey=type_id(e.g.References) and thevalue=cwl.RecordSchemaget_input, we pass this dictionary as a new parameterif/elselogic inget_input` we look up the type in the dictionary of records, if a value is found, it will be the type usedget_outputhas been renamed toget_expression_outputas it was not doing the same thing asget_input, I think.mainfunction into a newget_outputfunction, that looks similar toget_input. It handles the structs in the same manner.Resulting workflow using the `warp`'s `BuildIndices.wdl`:
☝️
Thanks!
Bruno