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

fix: link reads was not properly account for coassemble flag #139

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

rhysnewell
Copy link
Owner

@rhysnewell rhysnewell commented Sep 11, 2023

  • Fix symbolic linking of many reads
  • Add gpu support for polishing step
  • Add gpu support for semibin
  • Update documentation for coassemble
  • Remove channel_priority: strict. This is maybe no longer required?
  • Allow multiple input for reference-filter
  • rework reference-filter, rules no longer get prioritised thus reference filter is currently non-functional

@AroneyS, Have you used the gpu flag in the resources portion of snakemake rules before? I am thinking of adding it in as an option for the polishing rule, but I don't want it to fail if it can't request a GPU properly

Tests

  • Run local tests with reference filter
  • multiple reference filters
  • run short assembly test
  • run long test
  • run hybrid test

@AroneyS
Copy link
Collaborator

AroneyS commented Sep 12, 2023

I looked into a bit to see if I could add it for Semibin, but couldn't work out how to make it optional. I guess we could use the retries system to remove the gpu requirement on the second attempt, but its a bit clunky.

@rhysnewell
Copy link
Owner Author

Yeah, that's what I was thinking but then it just gets removed upon failure not related to GPU usage. Maybe if we tried like X times with increasing resource requests + GPU. Then if they all fail, then just reset the attempts but set no GPU

@AroneyS
Copy link
Collaborator

AroneyS commented Sep 12, 2023

That could work. Add retries to the rule itself as double the input retries (https://snakemake.readthedocs.io/en/stable/snakefiles/rules.html#defining-retries-for-fallible-rules)?

@rhysnewell
Copy link
Owner Author

rhysnewell commented Sep 13, 2023

Okay, so I couldn't work out a way to access the global retries variable from within a snakemake rule without overwriting it. Didn't spend too much time on that, but figured it would probs just be easier to add a --request-gpu flag that users can specify so now rules that have GPU support can maybe get thrown on a GPU node.

I revamped the reference filtering and QC for both long and short reads. I've tested locally with a bunch of different configurations so I'm fairly confident everything is okay. The test suites seem okay although it did try to rebuild my conda envs because the conda path got changed so I had to skip the integration test and run the same commands from command line. They seemed fine

@rhysnewell rhysnewell requested a review from AroneyS September 13, 2023 02:54
@AroneyS
Copy link
Collaborator

AroneyS commented Sep 13, 2023

LGTM. I think the --request-gpu is a much better idea anyway.

@rhysnewell rhysnewell merged commit 809cea1 into dev Sep 13, 2023
@rhysnewell rhysnewell deleted the link_reads_fix branch September 13, 2023 04:17
@AroneyS
Copy link
Collaborator

AroneyS commented Sep 13, 2023

@rhysnewell Looks like we need to fix the long reads test:

qc_long_reads

Kept 4 reads out of 360 reads
Running chopper on 1 files
Reference filter: 
Shell style : zcat /mnt/hpccs01/scratch/microbiome/aroneys/src/aviary/test/data/pbsim.fq.gz | chopper -l 100 -q 10 -t 16 | pigz -p 16 > data/long_reads.fastq.gz
cat return: 0
chopper return: 0
pigz return: 0

flye_assembly

[2023-09-13 14:00:41] INFO: Starting Flye 2.9.2-b1786
[2023-09-13 14:00:41] INFO: >>>STAGE: configure
[2023-09-13 14:00:41] INFO: Configuring run
[2023-09-13 14:00:41] ERROR: No reads above minimum length threshold (1000)
[2023-09-13 14:00:41] ERROR: Pipeline aborted

@rhysnewell
Copy link
Owner Author

Oh true, I was running with a lower min read length and min quality

@AroneyS AroneyS mentioned this pull request Sep 13, 2023
@AroneyS
Copy link
Collaborator

AroneyS commented Sep 13, 2023

If you add them to the test then I can rerun it.

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