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

Add hybpiper #5464

Merged
merged 43 commits into from
Sep 23, 2023
Merged

Add hybpiper #5464

merged 43 commits into from
Sep 23, 2023

Conversation

TomHarrop
Copy link
Contributor

@TomHarrop TomHarrop commented Aug 30, 2023

Adding the tool hybpiper


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)

@TomHarrop TomHarrop marked this pull request as draft August 30, 2023 06:45
@TomHarrop TomHarrop changed the title initial tests Add hybpiper Aug 30, 2023
@bgruening
Copy link
Member

@TomHarrop to trigger tests you need to add a .shed.yml file.

@TomHarrop
Copy link
Contributor Author

Thanks. Will add. I still have a bit of other work to do on the wrapper.

tools/hybpiper/hybpiper.xml Outdated Show resolved Hide resolved
tools/hybpiper/hybpiper.xml Outdated Show resolved Hide resolved
tools/hybpiper/hybpiper.xml Outdated Show resolved Hide resolved
@TomHarrop TomHarrop changed the title Add hybpiper DRAFT / WIP: hybpiper Sep 1, 2023
<conditional name="job_conditional">
<param name="hybpiper_job" value="assemble"/>
<param name="paired_input">
<collection type="paired" name="NZ874">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bgruening @bernt-matthias thanks for your feedback, this is almost ready but I need some help with this test.

Setting name on the collection causes a linting error (The attribute 'name' is not allowed).

If i I remove name, element_identifier is set to Unnamed Collection , hybpiper chokes on the space, and the test fails.

Is there a way to set the element_identifier in <test> -> <collection> ?

Copy link
Contributor

@bernt-matthias bernt-matthias Sep 6, 2023

Choose a reason for hiding this comment

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

Setting the collection name seems not possible at the moment. Could be worth opening an issue for this. But the question is: do the tests depend on that? I guess they should not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I opened an issue: galaxyproject/galaxy#16654

It does work (element_identifier resolves to NZ874), it's just linting that fails.

For now, I've added a check for special characters and set this test to expect_failure="true" in 2eeb313

@TomHarrop TomHarrop marked this pull request as ready for review September 6, 2023 23:35
@TomHarrop TomHarrop changed the title DRAFT / WIP: hybpiper Add hybpiper Sep 6, 2023
@TomHarrop
Copy link
Contributor Author

Thanks again for the help, I think this is ready for review / merge now. 🎉

tools/hybpiper/hybpiper.xml Outdated Show resolved Hide resolved
tools/hybpiper/hybpiper.xml Outdated Show resolved Hide resolved
tools/hybpiper/hybpiper.xml Outdated Show resolved Hide resolved
<option value="supercontig">Supercontig</option>
</param>
<param name="heatmap" type="boolean" checked="false" label="Produce a heatmap for each of the selected statistics" />
<param name="sequence_type_select" type="select" display="checkboxes" label="Choose sequences to extract" multiple="true">
Copy link
Member

Choose a reason for hiding this comment

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

is this one optional or not, is it possible to select none of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are all optional. I marked them optional in 28bcb4490.

Although if the user really deselects everything there won't be any outputs in Galaxy.

Copy link
Member

Choose a reason for hiding this comment

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

We should avoid that, correct? Thats why I have ask. To make sure the users has at least one output.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, if I confused you :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but I'm not sure how to prevent it in the wrapper. I tried a few things e.g.

        #unless $job_conditional.stats_type_select or $job_conditional.sequence_type_select:
            #raise ValueError("No outputs selected")
        #end unless

But the user will never see the error because there are no outputs.

Users should be able to select some stats or some sequences, but not nothing. Although I reckon deselecting all the outputs and then getting no output is quite logical :)

Copy link
Member

Choose a reason for hiding this comment

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

but then we simply make it optional=false?

Copy link
Member

Choose a reason for hiding this comment

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

I mean the output select?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I made stats optional=false in 022a3b2.

stats will be generated no matter what else the user selects. I wanted to avoid it because the underlying software is so slow but it's probably an edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm can't use checkboxes in that case. I think this would make the tool worse than having no output if all the output is de-selected. The user has to actually deselect everything since I have selected="true" on the usual options.

Apart from making it a single param, is there a way to say the user has to select at least one item from $job_conditional.stats_type_select or $job_conditional.sequence_type_select?

tools/hybpiper/hybpiper.xml Outdated Show resolved Hide resolved
@TomHarrop
Copy link
Contributor Author

Thanks for the review @bgruening

@bgruening
Copy link
Member

Damn the linting was not happy with checkbox and optional=false. Are you happy with the last commit. Then we can merge I think

@TomHarrop
Copy link
Contributor Author

I would personally just revert 022a3b2 but I can live with it either way.

@bgruening
Copy link
Member

Ok, please revert.

<validator type="no_options" message="No pre-installed organism DBs are available. Contact your galaxy administrator."/>

This you could try. A validator with type="no_option", maybe that works?

@TomHarrop
Copy link
Contributor Author

Sorry, I had an urgent appointment with a bibimbap last night 🍚

I think the confusion is that there are two optional params here. The user can select nothing from either of the selectors, but has to select at least one output across the two params. So the validator didn't work either.

I'm doing this because the setup for extracting stats (selector one) or extracting sequences (selector two) is expensive and it could be run on hundreds of samples at once. So it's good that the user can do both in one tool run. BUT, the procedure for extracting those things is also slow, so it's good that the user can exclude one if they don't want that output.

I've got the effect by adding an output that Galaxy expects if the user deselects everything. Then there is a check in the <command> and an error will be visible in the history. Added a test for that too. Hopefully that does it!

Thanks again @bgruening

@bgruening
Copy link
Member

Ok, please revert.

<validator type="no_options" message="No pre-installed organism DBs are available. Contact your galaxy administrator."/>

This you could try. A validator with type="no_option", maybe that works?

Have you tried this validator?

@TomHarrop
Copy link
Contributor Author

Yes

@TomHarrop
Copy link
Contributor Author

I can't use the validator because there are two params. This is what it's doing:

1. This works. Stats are optional.

screenshot-localhost_9090-2023 09 22-13_55_47

2. This works. Sequences are optional.

screenshot-localhost_9090-2023 09 22-13_56_15

3. This should fail (and does, with "ERROR: No outputs selected")

screenshot-localhost_9090-2023 09 22-13_56_29

screenshot-localhost_9090-2023 09 22-14_08_01

@bgruening
Copy link
Member

Thanks @TomHarrop!

@bgruening bgruening merged commit b439a8b into galaxyproject:main Sep 23, 2023
10 checks passed
@TomHarrop TomHarrop deleted the hybpiper branch February 12, 2024 00:07
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