Skip to content

Addresses https://github.com/galaxyproject/tools-iuc/issues/7999 - #1880

Open
scottcain wants to merge 3 commits into
bgruening:masterfrom
scottcain:scottcain-hicup-versioned-accessions
Open

Addresses https://github.com/galaxyproject/tools-iuc/issues/7999#1880
scottcain wants to merge 3 commits into
bgruening:masterfrom
scottcain:scottcain-hicup-versioned-accessions

Conversation

@scottcain

Copy link
Copy Markdown

(non-alphanumeric characters in genome names causes failure)

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

There are two labels that allow to ignore specific (false positive) tool linter errors:

  • skip-version-check: Use it if only a subset of the tools has been updated in a suite.
  • skip-url-check: Use it if github CI sees 403 errors, but the URLs work.

Description

For HiCUP tools that have --genome as an option, HiCUP has a strict rule that only alphanumeric characters plus underscores are allowed, meaning that a genome with a label like NCBI RefSeq-style accession with a version, like GCA_003177105.1, causes the execution of the tool to stop. This patch sanitizes those labels, replacing all non-allowed characters to underscores.

About tests

I was unable to run complete tests on my M4 Mac, but tests with --no_dependency_resolution did pass after I removed some strict version pins in the macro. Specifically, The gmp=6.2.1 pin causes a hard conda conflict with current nettle (which requires gmp>=6.3.0). For bowtie2 and samtools, I was unable to install the pinned version but with the pin removed, it got newer versions that did install. Those changes to the macro are included in this PR. To the test file, I added a test that provides an versioned accession to test these changes, otherwise reusing the existing test data.

(non-alphanumeric characters in genome names causes failure)

@bernt-matthias bernt-matthias left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought --genome is the cached index file.

Values from a text parameter can be sanitized much more easily:

https://docs.galaxyproject.org/en/master/dev/schema.html#tool-inputs-param-sanitizer

you do not need to do this in the command section.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove lines, maybe just a few are sufficient for the test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to say "the top ten lines of the result should look like this"?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use compare="contains" then the 10 lines must be included .. but not necessarily at the top. Otherwise have a look at https://docs.galaxyproject.org/en/master/dev/schema.html#tool-tests-test-output-assert-contents

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I changed to assert that the file contains the sanitized label and not the original label and removed the redundant output file.

@scottcain

Copy link
Copy Markdown
Author

Ah, I thought --genome is the cached index file.

Values from a text parameter can be sanitized much more easily:

https://docs.galaxyproject.org/en/master/dev/schema.html#tool-inputs-param-sanitizer

you do not need to do this in the command section.

Yeah, that's what I thought too until I dug in a little more deeply. I was unaware of the sanitizer thing; I'll update with that.

Comment thread tools/hicup/hicup_macros.xml Outdated
Comment on lines +8 to +9
<requirement type="package">bowtie2</requirement>
<requirement type="package">samtools</requirement>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should keep the versions (if you want you can update to the latest)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

samtools is really problematic for testing of this package: HiCUP has a dependence on the BioPerl samtools module, but that module requires samtools to be between >=0.1.19,<0.2.0a. This conflicts with the pin of this package of 1.16.1. Meaning tests could never pass (at least not in a normal testing environment). Should I put it back to 1.16.1 and live with failing tests on my machine?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bowtie2 is defined here: https://github.com/bioconda/bioconda-recipes/blob/master/recipes/hicup/meta.yaml so it can be removed I think.

bioperl as a dependency on bioperl-samtools https://github.com/bioconda/bioconda-recipes/blob/master/recipes/perl-bio-samtools/meta.yaml

And that one has also samtools defined and pinned, so I think you can drop this here also completely.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I dropped explicit dependencies on bowtie and samtools and tests still pass with --no_dependency_resolution which is what I would expect, but good that it does

@scottcain

Copy link
Copy Markdown
Author

@bernt-matthias I don't understand the nature of the two failing tests (check workflow success and combine chunked test results); one has so little output as to be useless to me, the other says to check a file that I don't know how to find.

@bgruening

Copy link
Copy Markdown
Owner

@scottcain please run planemo test --biocontainers ./tools/foo/var ... thats all you need.

The failing tests are due to changes in the diff of the result files. You can see them here: https://github.com/bgruening/galaxytools/actions/runs/26597546236?pr=1880

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.

3 participants