feat: Remove hardcoded snappy-pipeline steps #174
Conversation
|
looks good to me in principle, please continue |
|
TODO:
|
xiamaz
changed the title
|
Docs PR added in bihealth/snappy-pipeline#400 |
|
@holtgrewe This now closes #165, as support for arbitrarily named workflow step folders has been added. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
- Coverage 75.87% 75.58% -0.29%
==========================================
Files 95 98 +3
Lines 7668 7807 +139
==========================================
+ Hits 5818 5901 +83
- Misses 1850 1906 +56
☔ View full report in Codecov by Sentry. |
holtgrewe
requested review from
Nicolai-vKuegelgen
and removed request for
holtgrewe
|
Meta question: is there any way to get rid of / hide these warning boxes in the code review? They make the code quite unreadable :( |
There was a problem hiding this comment.
Most important general points, rest is in details:
- I don't think we should add the snappy code as a general requirement/dependency of cubi-tk, this can (or maybe should be) discussed though. I think checking for the possibility to import snappy_pipeline and then using that is reasonable though.
- Several of the new functions defined in cubi-tk/snappy/common.py can probably be replaced by importing things already defined in snappy_pipeline. Doing this should also reduce the amount of new - and not test-covered - code. Also it might make sense to bundle all of the functionality to interface with the snappy code into a single class.
cubi_tk/snappy/common.py
Outdated
| config, lookup_paths, config_paths = expand_ref( | ||
| str(config_path), | ||
| config_data, | ||
| [str(workflow_step_path), str(workflow_step_path.parent / ".snappy_pipeline")], | ||
| ) | ||
| return config, lookup_paths, config_paths |
There was a problem hiding this comment.
The config object returned as first element of the retruned tuple here is a string. However, most functions working with this expect it to be a (yaml representing) dictionary, likely objects are switched?
|
@Nicolai-vKuegelgen All comments have been adressed. Import is now optional and the logic for optional imports is also tested. |
This wip code removes the need to have hardcoded pipeline steps in
cubi-tk. Instead pipeline dependencies will be automatically parsed from the installedsnappy-pipeline.These changes only depend on some conventions in the
snappy-pipelineproject that should be fairly static (Snakefile needs to contain awfvariable), as otherwise finding the correct pipeline class is difficult.This partially fixes #165, but can be easily extended to fully support custom pipeline stage names.