-
Notifications
You must be signed in to change notification settings - Fork 92
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
WIP: Update and test fastx-tools #375
base: main
Are you sure you want to change the base?
Conversation
5fa2607
to
739dcf1
Compare
<outputs> | ||
<data format="png" name="outfile" metadata_source="input" /> | ||
</outputs> | ||
<tests> | ||
<test> |
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.
@bgruening unfortunately this test only passes if I set galaxy's conda version to 4.2.7 :(.
Otherwise I'm getting fastx_toolkit==0.0.14-2 instead of -3. See https://travis-ci.org/galaxyproject/tools-devteam/jobs/160679791#L355
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.
Urgs, this offline bug needs to be fixed. This is really killing us.
@@ -1,10 +1,14 @@ | |||
<tool id="cshl_fastx_barcode_splitter" version="1.0.0" name="Barcode Splitter"> | |||
<tool id="cshl_fastx_barcode_splitter" version="@VERSION@" name="Barcode Splitter"> |
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.
@lparsons would you mind if I pull in your fastx_barcode_splitter_enhanced tool as a replacement? I think your version is much more useful.
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.
@mvdbeek Not at all. How can I help with 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.
The PR has been opened from the test_fastx
branch, if you copy over the tool files here and do a PR selecting the test_fastx
brach, we can get your version into this PR.
Just so I don't forget this: This is stuck until the conda --offline mode is fixed, so that we can use a newer conda in galaxy. 3.19.3 will not install the latest fastx tool version (0.0.14 build 3 or higher) that is needed for tests to pass. |
Unfortunately tests are missing
</macros> | ||
<macros> | ||
<import>fastx_macros.xml</import> | ||
</macros> |
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.
The macro is imported twice.
@@ -2,52 +2,50 @@ sudo: false | |||
language: python | |||
python: 2.7 | |||
|
|||
# This is a big repository so break Travis CI work into 4 CI jobs. |
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.
s/4/2/
<macros> | ||
<import>fastx_macros.xml</import> | ||
</macros> | ||
<command> |
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.
Indentation
</macros> | ||
<macros> | ||
<import>fastx_macros.xml</import> | ||
</macros> |
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.
Macro is imported twice.
<macros> | ||
<import>fastx_macros.xml</import> | ||
</macros> | ||
<command detect_errors="aggressive">fastx_nucleotide_distribution_graph.sh -t '$input.name' -i $input -o $output</command> |
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.
Indentation
@nsoranzo what do you say about the conda upgrade? |
@mvdbeek urgs, do you have your branch still around? I think I reverted your changes, sorry. |
yes, i still have them, resolving conflicts now. |
-Q 33 is the default, but it needs to be set to -Q 64 for fastqillumina and fastqsolexa.
Thanks @mvdbeek! |
Hmm, looks like there's still something wrong with the dependencies, I'll have a look. At the minimum fastq_quality_boxplot appears to miss gnuplot ... how could I have missed that?! |
And why are the builds passing? Something is weird here :(. |
Not sure. Can you rebase against master and maybe update the travis testing yaml. |
Ahh, slowly things are coming back ... gnuplot is a dependency of fastx_toolkit in the latest build ... which only install with conda > 3 |
Let's upgrade the travis testing to conda4 ... in the spirit of galaxyproject/galaxy#3378 |
Fine for me! |
@mvdbeek I'm not sure I agree with 70695e1, shouldn't be necessary. The previous Travis build fails because of bioconda/bioconda-recipes#3256 (comment) |
Yep, but the thing I'm preventing with that commit (which is an alternative version of f1fcf0b) is https://travis-ci.org/galaxyproject/tools-devteam/jobs/188128054#L1131 |
f1fcf0b seems to work for getting planemo back up, but installing planemo within the updated conda env didn't work. I have removed those two commits, but the current situation is not very nice either. |
- $PLANEMO_CONDA_PREFIX/bin/conda update -y conda | ||
- $PLANEMO_CONDA_PREFIX/bin/conda config --get channels # verify conda-forge is not highest priority | ||
- $PLANEMO_CONDA_PREFIX/bin/conda config --add channels conda-forge # moves conda-forge to the top | ||
- $PLANEMO_CONDA_PREFIX/bin/conda config --get channels # now conda-forge should be highest priority |
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.
conda-forge should have the lowest (not highest) priority, the order (from highest to lowest priority) should be:
- iuc
- bioconda
- r
- defaults
- conda-forge
and they should be added in reverse order.
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.
yep, I'll have to revert this ... it's being ignored anyway, because planemo use the condarc from ~/.planemo/condarc. Though last time I checked the order was not correct.
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.
If ~/.planemo/condarc is not correct, can we try to overwrite it?
@@ -1,18 +1,25 @@ | |||
<tool id="cshl_fasta_clipping_histogram" name="Length Distribution" version="1.0.0"> | |||
<tool id="cshl_fasta_clipping_histogram" name="Length Distribution" version="@VERSION@"> |
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 wonder if this would cause problems, unless the version lineage code recognizes that 0.0.14 > 1.0.0
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.
Oh yes, this is true. Too bad. Are you working on this?
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 was planning on moving it to IUC in galaxyproject/tools-iuc#1413, and of course I can merge in your changes there to avoid reinventing effort.
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.
cool, please go ahead and close this when you are ready.
… wrapper versions.
This PR also need a new IUC package
package_fastx_toolkit_0_0_14
.