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

Update datatype converter tools #3305

Merged
merged 2 commits into from Dec 13, 2016

Conversation

Projects
None yet
3 participants
@nsoranzo
Copy link
Member

commented Dec 12, 2016

  • dos2unix
  • Fix indentation
  • Add missing tool version
  • Remove deprecated <page> elements
  • Remove deprecated interpreter attribute of <command>
  • Quote some data parameters in <command>
  • Remove some duplicated requirement
  • Add CDATA
  • Use && to concatenate commands
  • Quote $chromInfo (it's a file)
@bgruening
Copy link
Member

left a comment

One bug and a lot of small comments.
I think we should fix the dependencies to prevent errors later on.

</help>
<tool id="CONVERTER_Bam_Bai_0" name="Bam to Bai" version="1.0.0" hidden="true">
<requirements>
<requirement type="package">samtools</requirement>

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

Should we pin this to a fixed version?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Dec 13, 2016

Member

I think that would be a good idea

<inputs>
<page>
<!-- <description>__NOT_USED_CURRENTLY_FOR_CONVERTERS__</description> -->
<command>python '$__tool_directory__/bcf_to_bcf_bgzip_converter.py '$input1' '$output1'</command>

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

here is one ' missing

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Dec 12, 2016

Author Member

Thanks, I've checked multiple times but still I have missed one! 🐦 👀

<param format="bcf" name="input1" type="data" label="Choose bcf file"/>
</page>
</inputs>

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

Can we add python2 as a dependency here and in the other tools?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Dec 12, 2016

Author Member

All converter tools should already support both Python 2 and 3.

<page>
<!-- <description>__NOT_USED_CURRENTLY_FOR_CONVERTERS__</description> -->
<command>python '$__tool_directory__/interval_to_tabix_converter.py' -P bed '$input1' '$bgzip' '$output1'</command>
<inputs>

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

tabix and python as dependency?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Dec 13, 2016

Author Member

This is using pysam, which is in the Galaxy virtualenv.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Dec 13, 2016

Member

But that's not necessarily available on cluster nodes.

<tool id="CONVERTER_bedgraph_to_bigwig" name="Convert BedGraph to BigWig" version="1.0.0" hidden="true">
<!-- Used internally to generate track indexes -->
<requirements>
<requirement type="package">ucsc_tools</requirement>

This comment has been minimized.

</help>
</tool>
<tool id="CONVERTER_gff_to_bgzip_0" name="Convert GFF to BGZIP" version="1.0.0" hidden="true">
<!-- <description>__NOT_USED_CURRENTLY_FOR_CONVERTERS__</description> -->

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

dependencies missing

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Dec 13, 2016

Author Member

This also use pysam from the Galaxy virtualenv.

</help>
<tool id="CONVERTER_interval_to_bed12_0" name="Convert Genomic Intervals To Strict BED12" version="1.0.0">
<!-- <description>__NOT_USED_CURRENTLY_FOR_CONVERTERS__</description> -->
<command>python '$__tool_directory__/interval_to_bedstrict_converter.py' '$output1' '$input1' ${input1.metadata.chromCol} ${input1.metadata.startCol} ${input1.metadata.endCol} ${input1.metadata.strandCol} ${input1.metadata.nameCol} ${input1.extension} 12</command>

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

I would break this column into mulitple lines.

<!-- <description>__NOT_USED_CURRENTLY_FOR_CONVERTERS__</description> -->
<requirements>
<requirement type="package">ucsc_tools</requirement>
<requirement type="package">bedtools</requirement>

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

Adding versions and maybe the conda package name?

## | wigToBigWig stdin $chromInfo '$output'

## This can be used anywhere.
> temp.bg ; bedGraphToBigWig temp.bg $chromInfo '$output'

This comment has been minimized.

Copy link
@bgruening

bgruening Dec 12, 2016

Member

CDATA and &&

<requirement type="package">ucsc_tools</requirement>
</requirements>
<command>
grep -v "^track" '$input' | wigToBigWig -clip stdin $chromInfo '$output'

This comment has been minimized.

Copy link
@bgruening
@bgruening

This comment has been minimized.

Copy link
Member

commented Dec 12, 2016

Thanks @nsoranzo!

nsoranzo added some commits Dec 12, 2016

Update datatype converter tools
- dos2unix
- Fix indentation
- Add missing tool version
- Remove deprecated <page> elements
- Remove deprecated 'interpreter' attribute of <command>
- Quote some data parameters in <command>
- Remove some duplicated requirement

@nsoranzo nsoranzo force-pushed the nsoranzo:converter_tools branch from 7ba9fab to ac8450f Dec 13, 2016

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2016

@bgruening Thanks for reviewing! I've fixed all comments (and other issues) expect for the requirements, I'm a bit worried this can break working setups. Can we postpone this change to when we enable Conda by default?

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Dec 13, 2016

👍 I think we can work on the requirements in a separate PR.

@bgruening

This comment has been minimized.

Copy link
Member

commented Dec 13, 2016

Sure, fine with me, thanks for fixing Nicola!

@mvdbeek mvdbeek merged commit cf7119c into galaxyproject:dev Dec 13, 2016

4 checks passed

api test Build finished. 243 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 131 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details

@nsoranzo nsoranzo deleted the nsoranzo:converter_tools branch Dec 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.