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

Improvements to the script validating WDLs using miniwdl #402

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

VJalili
Copy link
Member

@VJalili VJalili commented Sep 12, 2022

  • Take imports dir as an input argument;
  • Allow setting max depth of import, and set it to max 2 (compared to max 10 of miniwdl default); # - Improve displaying messages on the console;
  • Add tips for debugging errors.

# - Take imports dir as an input argument;
# - Allow setting max depth of import, and set it to max 2 (compared to max 10 of miniwdl default);
# - Improve displaying messages on the console.
@VJalili VJalili marked this pull request as ready for review September 12, 2022 18:05
Copy link
Collaborator

@mwalker174 mwalker174 left a comment

Choose a reason for hiding this comment

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

Great, this looks fine to me. The max import depth is hard-coded in mini-wdl correct? I see the default of 2 which seems very shallow to me. Also just curious - I may have misheard you at last standup but isn't the max depth 10? Did you actually encounter this anywhere in our repo?

@VJalili
Copy link
Member Author

VJalili commented Sep 15, 2022

Thanks for the review @mwalker174. 10 is the default value for max import depth; I am changing the default to 2 mainly to simplify debugging/tracking import errors.

For instance, with max import depth set to 10, the validation process throws an import error validating wdl/AnnoRdPeSr.wdl complaining 'Failed to import VaPoR.wdl', which implies the script cannot find VaPoR.wdl in the import path; while the root cause is in wdl/TasksMakeCohortVcf.wdl with the error Expected Boolean instead of Boolean?.

@VJalili VJalili requested a review from mwalker174 September 15, 2022 16:51
@VJalili VJalili merged commit 2da5e29 into broadinstitute:master Sep 15, 2022
@VJalili VJalili deleted the miniwdl_val2 branch September 15, 2022 19:15
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.

2 participants