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

docs: full task compliance with style-guide #110

Merged
merged 45 commits into from
Sep 28, 2023
Merged

docs: full task compliance with style-guide #110

merged 45 commits into from
Sep 28, 2023

Conversation

a-frantz
Copy link
Member

@a-frantz a-frantz commented Sep 4, 2023

I believe all task meta/parameter meta should be fully spec compliant now. Except the ngsderive file which is being handled in #108 . If any tasks are noticed with non-compliant meta sections, please bring to my attention. (Sure would be nice to have a tool that would verify this for us)

And a new style rule! All "deprecated" tasks should be at the bottom of the file. I figure that's an uncontroversial rule.

While working through this, I also made some updates to tasks, mainly exposing more parameters in samtools.wdl.

Also set a new soft limit for "least amount of memory to allocate a task", which is 4gb. Implementing this meant lowering a few tasks' default allocation from 5gb to 4gb for consistency. No allocations were raised (as none were less than 4).

Still TODO in other PRs:

  • meta sections are currently missing from all our workflows
    • hopefully the outputs section will be easier to write now, as we can just copy+paste from the task outputs sections
  • Headers are still inconsistent, and to boot I don't think a single one conforms to what we "enforce" in the style-guide.
  • I added a whole bunch of small TODO comments that I didn't feel like addressing at the time. They can be dealt with in another PR
  • Investigate whether we can get away with a lower memory allocation of 3gb. This would allow running on most of the smallest cloud instances.

@a-frantz a-frantz self-assigned this Sep 4, 2023
@a-frantz a-frantz changed the title Docs docs: full task compliance with style-guide Sep 5, 2023
@a-frantz a-frantz requested a review from adthrasher September 5, 2023 13:59
tools/deeptools.wdl Outdated Show resolved Hide resolved
tools/estimate.wdl Show resolved Hide resolved
tools/estimate.wdl Outdated Show resolved Hide resolved
tools/htseq.wdl Outdated Show resolved Hide resolved
tools/kraken2.wdl Outdated Show resolved Hide resolved
tools/picard.wdl Outdated Show resolved Hide resolved
tools/qualimap.wdl Outdated Show resolved Hide resolved
tools/samtools.wdl Outdated Show resolved Hide resolved
tools/samtools.wdl Outdated Show resolved Hide resolved
tools/samtools.wdl Outdated Show resolved Hide resolved
@a-frantz
Copy link
Member Author

New repository rule! Beginning with this PR, ALL tasks must be covered by at least one test and all tests must be passing (unless there's good reason for a test to be failing). No CI has been implemented yet, so we're relying on ad-hoc testing and self-reporting of success.

I have added tests that were missing, and have gotten everything passing. EXCEPT:

  • the cellranger tasks were not validated. It's a pain to configure my cluster singularity environment to test them, and we've discussed moving the cellranger files to their own repository. AKA I'm treating their tests as OOS.
  • the qc_summary test is also failing. The underlying task is out of sync with the multiqc task, and qc_summary makes bad assumptions about what fields will be present. It needs to be re-written from scratch. That will likely be the final task before the QCv2 official release. It's a wasted effort fixing it till that later stage.

@a-frantz a-frantz requested a review from adthrasher September 21, 2023 15:34
tools/samtools.wdl Show resolved Hide resolved
@a-frantz a-frantz merged commit 30a7731 into main Sep 28, 2023
@a-frantz a-frantz deleted the docs branch September 28, 2023 15:34
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