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: Wrapper crashing if dirs not clean #464

Closed
wants to merge 2 commits into from
Closed

Conversation

xiamaz
Copy link
Member

@xiamaz xiamaz commented Nov 12, 2023

Closes #443

Not elegant but will always avoid the crash

@coveralls
Copy link

coveralls commented Nov 12, 2023

Coverage Status

coverage: 85.866%. remained the same
when pulling df94bb2 on 443-contig-ploidy-fix
into 4874074 on main.

@Nicolai-vKuegelgen
Copy link
Contributor

I have also started work on a fix for this (see branch 443-gcnv_contig_ploidy-wrapper-script-crahses-because-gatk-determinegermlinecontigploidy-output-is-not-cleaned-but-always-fully-read).

I see some issues with your solution:

  1. Skipping unknown samples silently is not very future proof, since we won't get any clear error message if somehow the sample names output of gcnv doesn't match the samplesheet anymore.
  2. If the additional sample folders are not removed it is possible that later GCNV steps will pick them up again (i.e. call_cnvs which reads in the output dir of the contig_ploidy step).
  3. Applying this fix with the same expectations for the case mode and the corohort mode for gcnv is maybe not correct, since the 'usual' problem of re-running snappy in the same project with a smaller set of (new) samples should only apply to case-mode.

@xiamaz
Copy link
Member Author

xiamaz commented Nov 13, 2023

Can your fix be merged? I'm impartial as long as behavior for germline gcnv pipelines does not require one to clean the output directories.

@Nicolai-vKuegelgen
Copy link
Contributor

My fix could be merged right now (I've tested it & it works) - will make a pull request.
The only reliable & clean solution I could come up with is to have the affected wrapper scripts remove the directories they will write into. Leaving old files there is problematic since GCNV often just requires a base path if i.e. the contigs and will then read whatever is there.

@Nicolai-vKuegelgen
Copy link
Contributor

Should be fixed by #468 now, so I'm closing this

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