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

Condaify all the tools #468

Merged
merged 39 commits into from
Aug 22, 2017
Merged

Condaify all the tools #468

merged 39 commits into from
Aug 22, 2017

Conversation

armish
Copy link
Member

@armish armish commented May 17, 2017

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)

@armish armish changed the title WIP: Condaify all the tools WIP: Condaify all the tools (a.k.a. Dreaming of condafication) May 17, 2017
@tavinathanson
Copy link
Member

@armish super awesome! I'll try it out!

@tavinathanson
Copy link
Member

Successfully ran this on 75 samples (after a few retries)! And made some additions in f899333.

Will defer to @rleonid on the remainder of the review.

| `PythonCustom of string
| `PythonToolDependency of string
]

Copy link
Member

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.

Copy link
Member

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)

Copy link
Member Author

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.

armish added 19 commits June 2, 2017 05:55
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)
@armish
Copy link
Member Author

armish commented Jun 8, 2017

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 rebases and squashs, and also some documentation on all the major additions/changes.

Stay tuned!

run_program run_with
~requirements:[ `Quick_run; ]
Program.(
shf "cat %s \
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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? :)

Copy link
Member

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.

@smondet smondet self-assigned this Jun 12, 2017
@smondet smondet added the WIP label Jun 12, 2017
armish and others added 13 commits June 17, 2017 01:21
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.
@smondet smondet changed the title WIP: Condaify all the tools (a.k.a. Dreaming of condafication) Condaify all the tools Aug 21, 2017
@smondet smondet removed the WIP label Aug 21, 2017
@smondet smondet merged commit b234f5b into master Aug 22, 2017
smondet added a commit that referenced this pull request Aug 22, 2017
@armish
Copy link
Member Author

armish commented Sep 6, 2017

(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.

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.

4 participants