-
Notifications
You must be signed in to change notification settings - Fork 4
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
Condaify all the tools #468
Conversation
@armish super awesome! I'll try it out! |
src/environment_setup/conda.mli
Outdated
| `PythonCustom of string | ||
| `PythonToolDependency of string | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a regular variant? I would also document this type, as the meaning these variants isn't obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
for the documentation
I don't mind the polymorphic variant Vs regular variant.
The guideline is to use underscores and never CamelCase (so → `Python_tool_dependency
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regular variant it is then. I will give revisit this whole branch once all the implementation is done and will make sure I document all the new parts thoroughly.
Switching between language contexts has this weird effect of involuntarily mixing/matching coding conventions on me. Good catch - will also address current styling issues once all is done.
This will allow easy and isolated installations of many of the bfx tools in a way that is also easy to maintain (need to upgrade? Just increase the version number..)
ditches the biopam setup in favor of the bioconda to fix many of our dependency problems.
The part we are most insterested in is the conversion of the PHRED scores as part of the `seq` subcommand.
This will make it easier to manage 3rd party installers (e.g. Bioconda, conda, Pip, ...)
ditch opam in favor of bioconda for uniformity across hla callers
features code extraction, optimization, and documentation
This gets really useful when the only information that we know about the location of a particular folder/file depends on the init procedure and we need to let the condition checking mechanism know about the ENV. bash > sh due to `source` support (cf. conda)
Still WIP so ignore my recent push, @smondet. I was just for @tavinathanson to test something from this branch. This is almost done but need a few Stay tuned! |
run_program run_with | ||
~requirements:[ `Quick_run; ] | ||
Program.( | ||
shf "cat %s \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the point of hlarp
was to avoid needing to do munging like this?
@rleonid can hlarp
offer this is an output format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tavinathanson: yep - this is the controversial code that made fun of during the retreat and also the same exact thing @rleonid mentioned here: hammerlab/discohorts#9
I just factored it out from the hlarp module since this was mostly about making it compatible with mhctools
-family of libraries. I have another experimental hlarp
branch that I am currently working on which will allow printing plain lists instead of annotated CSVs and it will also enable have a subcommand to extract the consensus calls across multiple typing runs when we have more than one tool/data-type for a single patient.
Once that is done, I will get rid of this bash foo and will save @ihodes from all the headache ;) Hopefully cohorts/discohorts will also benefit from those additional hlarp functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that piece of code does stand out, doesn't it? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hlarp
's chosen output format was something agreed upon that the Python sweet of tools should support.
also adds 'wkhtmltopdf' as a vaxrank dependency (fixes #480). Also see hammerlab/secotrec#78.
The problem was that when we have this in its default form any search or subdirectory listing within the strelka toolkit folder was failing due to permission errors. This should make a 'find' run smoother.
Fixed conflicts: - `src/environment_setup/python_package.ml`: new setup Vs version changes. - changed the versions form master in `src/run_environment/machine.ml`. - `src/environment_setup/tool_providers.ml`: only white space. - `src/pipeline_edsl/to_workflow.ml`: passing around provenance information.
Now the debug log also gets moved.
(belated) thank you so much for all the help with getting this branch up to speed and merged, @smondet! Hope these changes won't cause too much of a trouble for the epidico folks. |
The idea here is to install as many tools as possible via conda's bioconda channel and worry less about all the weird setup/dependency issues.
Currently testing OptiType first (cc: @tavinathanson and @rleonid)