Skip to content
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

Add WDL parser to Janis #69

Merged
merged 21 commits into from
Jun 3, 2021
Merged

Add WDL parser to Janis #69

merged 21 commits into from
Jun 3, 2021

Conversation

illusional
Copy link
Member

@illusional illusional commented Mar 15, 2021

Adds:

  • "foreach" which is similar to scatter. Perform some operation prior to scattering - this is the analogue to how WDL performs "scattering", and has greater flexibility for doing things like allowing you to iterate over a range of values.
    • this is implemented for CWL and WDL, and contains a small test.
  • to_python for every operator. This isn't exactly useful for this PR (it will be for my future Hail Batch PR), but it was difficult to separate.

Known limitations:

  • Doesn't support structs, maps, or objects
  • Some methods don't have Janis equivalents (read_* and write_*, cross, dot)
  • Not sure how it handles expression placeholder options
  • Doesn't support nested scatters
  • Setting variables in the call-space in workflow execution is not well supported.

@codecov-io
Copy link

Codecov Report

Merging #69 (3d5bcea) into master (bcbc1da) will decrease coverage by 1.53%.
The diff coverage is 28.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   69.12%   67.58%   -1.54%     
==========================================
  Files          78       79       +1     
  Lines       10282    10697     +415     
==========================================
+ Hits         7107     7230     +123     
- Misses       3175     3467     +292     
Impacted Files Coverage Δ
janis_core/ingestion/fromwdl.py 0.00% <0.00%> (ø)
janis_core/operators/stringformatter.py 83.82% <16.66%> (-3.10%) ⬇️
janis_core/operators/logical.py 65.65% <34.78%> (-1.39%) ⬇️
janis_core/operators/standard.py 55.69% <45.07%> (-3.14%) ⬇️
janis_core/operators/selectors.py 69.53% <52.17%> (-1.39%) ⬇️
janis_core/operators/operator.py 65.34% <53.33%> (-0.82%) ⬇️
janis_core/translations/cwl.py 77.26% <70.83%> (-0.20%) ⬇️
janis_core/workflow/workflow.py 47.86% <71.42%> (-0.08%) ⬇️
janis_core/tests/test_operators.py 98.80% <100.00%> (ø)
janis_core/tests/test_translation_cwl.py 99.53% <100.00%> (+<0.01%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcbc1da...3d5bcea. Read the comment docs.

Copy link
Member

@junyk junyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@junyk junyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@illusional
Copy link
Member Author

Hey @junyk, is there anything outstanding on this PR? This isn't packaged in a complete catch-all script, nor is it "production-ready" (it probably doesn't gracefully handle errors).

I have a few ideas on how we could add some of the "read_*" operators into Janis, but it's a little bit of work.

@junyk
Copy link
Member

junyk commented May 25, 2021

@janis-bot

@illusional
Copy link
Member Author

Hey @junyk, is it possible to get more information about the tests? I see some are failing, some are passing, keen to know if it's this PR which is causing something to go astray.

@junyk
Copy link
Member

junyk commented May 25, 2021

Hey @illusional , sorry I haven't responded cause I am still trying to figure out what caused the failures (it could be environment). I'll get back to you!

Copy link
Member

@junyk junyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janis-bot try again!

Copy link
Member

@junyk junyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@junyk junyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2021

Codecov Report

Merging #69 (7d8b7b3) into master (e74e6fe) will decrease coverage by 1.75%.
The diff coverage is 28.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   69.11%   67.36%   -1.76%     
==========================================
  Files          78       79       +1     
  Lines       10320    10789     +469     
==========================================
+ Hits         7133     7268     +135     
- Misses       3187     3521     +334     
Impacted Files Coverage Δ
janis_core/ingestion/fromwdl.py 0.00% <0.00%> (ø)
janis_core/operators/stringformatter.py 84.44% <20.00%> (-2.48%) ⬇️
janis_core/operators/logical.py 65.65% <34.78%> (-1.39%) ⬇️
janis_core/operators/standard.py 54.70% <46.00%> (-4.13%) ⬇️
janis_core/operators/selectors.py 69.18% <50.00%> (-1.75%) ⬇️
janis_core/operators/operator.py 65.34% <53.33%> (-0.82%) ⬇️
janis_core/translations/cwl.py 77.26% <70.83%> (-0.20%) ⬇️
janis_core/workflow/workflow.py 48.45% <71.42%> (-0.08%) ⬇️
janis_core/tests/test_operators.py 98.80% <100.00%> (ø)
janis_core/tests/test_translation_cwl.py 99.53% <100.00%> (+<0.01%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e74e6fe...7d8b7b3. Read the comment docs.

Copy link
Member

@junyk junyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@illusional
Copy link
Member Author

Oooh, this is pretty! Just need to update the title of the page :p

image

@janis-bot
Copy link

Test Failed for group 60af2b627eeab92ee27259f1.

Bioinformatics Tools:

  • ParseFastqcAdaptors.None (Nectar - cromwell): Test Succeeded
  • ParseFastqcAdaptors.None (Spartan - cromwell): Test Succeeded
  • SamToolsFlagstat.None (Nectar - cromwell): Test Succeeded
  • SamToolsFlagstat.None (Spartan - cromwell): Test Succeeded
  • Gatk4HaplotypeCaller.None (Nectar - cromwell): Test Succeeded
  • Gatk4HaplotypeCaller.None (Spartan - cromwell): Test Succeeded
  • performanceSummary.None (Nectar - cromwell): Test Succeeded
  • performanceSummary.None (Spartan - cromwell): Test Succeeded

WGSGermlineGATK:

  • WGSGermlineGATK.brca1 (Spartan - cromwell): Test Failed 60af2b627eeab92ee27259fa

Small Bioinformatics Workflow:

  • bwaaligner.None (Nectar - cromwell): Test Succeeded
  • bwaaligner.None (Nectar - cwltool): Test Succeeded

@illusional
Copy link
Member Author

Thanks for the report @junyk! I tracked it down to a double unwrap of the basename function introduced in 260a02c, and resolved in 0ee5a82. Great thing we had these tests!

@junyk
Copy link
Member

junyk commented May 31, 2021

@illusional thanks for looking into it, Michael! I will run the tests one more time :)

Copy link
Member

@junyk junyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janis-bot
Copy link

Test Failed for group 60b4314f7423dc52134f1c56.

Bioinformatics Tools:

  • ParseFastqcAdaptors.None (Nectar - cromwell): Test Succeeded
  • ParseFastqcAdaptors.None (Spartan - cromwell): Test Failed 60b4314f7423dc52134f1c58
  • SamToolsFlagstat.None (Nectar - cromwell): Test Succeeded
  • SamToolsFlagstat.None (Spartan - cromwell): Test Succeeded
  • Gatk4HaplotypeCaller.None (Nectar - cromwell): Test Succeeded
  • Gatk4HaplotypeCaller.None (Spartan - cromwell): Test Succeeded
  • performanceSummary.None (Nectar - cromwell): Test Succeeded
  • performanceSummary.None (Spartan - cromwell): Test Succeeded

WGSGermlineGATK:

  • WGSGermlineGATK.brca1 (Spartan - cromwell): Test Succeeded

Small Bioinformatics Workflow:

  • bwaaligner.None (Nectar - cromwell): Test Succeeded
  • bwaaligner.None (Nectar - cwltool): Test Succeeded

@junyk
Copy link
Member

junyk commented Jun 3, 2021

I have manually re-tested ParseFastqcAdaptors , it works. Merging this now.

@junyk junyk merged commit ef1ba7b into master Jun 3, 2021
@junyk
Copy link
Member

junyk commented Jun 3, 2021

thanks @illusional !

@mr-c
Copy link
Contributor

mr-c commented Aug 30, 2021

@illusional How do I test this? I installed the latest janis-core and janis repositories locally but janisdk fromwdl doesn't work for me

@illusional
Copy link
Member Author

Hey @mr-c, I think you've figured it out already, but you have to run the script directly. I've opened a PR to add it to janisdk: PMCC-BioinformaticsCore/janis#58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants