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 genomes on server functionality #164

Merged
merged 9 commits into from
Feb 22, 2024
Merged

Conversation

elischberg
Copy link
Contributor

No description provided.

@peterjc
Copy link
Owner

peterjc commented Feb 6, 2024

Can you point me at the Galaxy tool author documentation for this new (to me) functionality? I'm wondering e.g. if it requires a specific version of Galaxy (and if it would fail gracefully on an older one).

@wm75
Copy link
Contributor

wm75 commented Feb 6, 2024

@peterjc https://docs.galaxyproject.org/en/latest/dev/schema.html#tool-inputs-param-options
not sure when this was added, but definitely long ago (before 2015 I'd say) :-)

@elischberg
Copy link
Contributor Author

elischberg commented Feb 13, 2024

I had problems in handling the term "value_label" in the output name, because i didn't understand it quite well. Is it only for having "Protein" instead of "prot" in the name of the output?
I removed it for now, but I can insert it again, if I get to know how to use it.

Maybe an additional test for the genome on server functionality would be nice?

@peterjc
Copy link
Owner

peterjc commented Feb 13, 2024

Would the current nested conditionals break old workflows etc? Might therefore this be a safer approach:

     <param argument="-dbtype" type="select" display="radio" label="Molecule type of input">
        <option value="prot">protein from your history</option>
        <option value="nucl">nucleotide from your history</option>
        <option value="nucl-server">nucleotide from server</option>
      </param>

Copy link
Contributor

@wm75 wm75 left a comment

Choose a reason for hiding this comment

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

Nice :) Thanks for adding the new test!

You will need to bump the @VERSION_SUFFIX@ eventually.

tools/ncbi_blast_plus/ncbi_makeblastdb.xml Outdated Show resolved Hide resolved
@wm75
Copy link
Contributor

wm75 commented Feb 20, 2024

Would the current nested conditionals break old workflows etc?

@peterjc in a sense, yes. Though of course only for people that are trying to run their workflow against a server that doesn't have the old tool version (requested by the workflow) installed. We normally consider this an acceptable drawback though I don't like making the life of users unnecessarily hard either.
In this case, however, there is no way to add the feature without this kind of breakage. Even your proposed simpler solution would require a new outside conditional to treat the new third selection differently, and again that would make workflow auto-updates to the new version impossible.

After testing a few alternatives I suggested to @elischberg to use the repeat elements (instead of the original multi-selects) because that lets you combine FASTAs from the history and server-provided ones when building the DB.
Was hoping to be able to propose a smaller change, but it didn't work out. Sorry for the extra reviewing effort and hope you find this solution acceptable.

@wm75
Copy link
Contributor

wm75 commented Feb 20, 2024

btw, the use case behind the new feature is a viral primer scheme design tool that lets you exclude primer candidates based on them having matches in a custom BLAST DB, e.g. to avoid amplification of host DNA. With the new option, users will be able to create their desired subtraction DB as long as Galaxy Europe has the corresponding host reference installed.

@peterjc
Copy link
Owner

peterjc commented Feb 21, 2024

Given both @elischberg and @wm75 have been testing this (I have not), I'm happy in principle to merge this.

There are not any other imminent pull requests pending, so I suggest you bump the version and describe the changes in https://github.com/peterjc/galaxy_blast/blob/master/tools/ncbi_blast_plus/README.rst - something like:

2.14.1+galaxy1 - Use genome FASTA files on the server with ``makeblastdb`` (contribution from ...)

@peterjc
Copy link
Owner

peterjc commented Feb 21, 2024

Test failures in GitHub actions:

 "failure;ncbi_makeblastdb;0"
"failure;ncbi_makeblastdb;1"
"failure;ncbi_makeblastdb;2"
"failure;ncbi_makeblastdb;3"
"failure;ncbi_makeblastdb;4"

Looking at the HTML file tool_test_output.html via the All tool test results.zip available from the artefacts of the failed run:

Error: Unknown argument: "dbtypne"
Error:  (CArgException::eInvalidArg) Unknown argument: "dbtypne"

Happily that looks like an easy fix [already suggested by @wm75, which I have just committed. Hopefully won't complicate further updates to the pull request - ask if you need help with git]

Copy link
Contributor

@wm75 wm75 left a comment

Choose a reason for hiding this comment

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

Thanks @peterjc and @elischberg - this looks good to me then!

@peterjc peterjc merged commit 028e3e8 into peterjc:master Feb 22, 2024
13 checks passed
@peterjc
Copy link
Owner

peterjc commented Feb 22, 2024

Squash-and-merged, thank you both.

In theory this will auto-deploy to the Tool Shed... https://github.com/peterjc/galaxy_blast/actions/runs/8003981806

@peterjc
Copy link
Owner

peterjc commented Feb 22, 2024

Test Tool Shed failed - could be in flux?

Main Tool Shed also failed:

Could not update ncbi_blast_plus
Unexpected HTTP status code: 500: {"err_msg": "Metadata may have been defined for some items in revision 'b6893f57f8d8'.  Correct the following problems if necessary and reset metadata.<br/><b>ncbi_makeblastdb.xml<\/b> - This file refers to a file named <b>all_fasta.loc.sample<\/b>.  Upload a file named <b>all_fasta.loc.sample.sample<\/b> to the repository to correct this error.<br/>"}
Failed to update repository contents for repository 'ncbi_blast_plus' on the main Tool Shed.
Repository metadata updated successfully for repository 'ncbi_blast_plus' on the main Tool Shed.

Looks like we need to add all_fasta.loc.sample to https://github.com/peterjc/galaxy_blast/blob/master/tools/ncbi_blast_plus/.shed.yml

peterjc added a commit that referenced this pull request Feb 22, 2024
@peterjc
Copy link
Owner

peterjc commented Feb 22, 2024

Huh - the automated deployment did go though even without the all_fasta.loc.sample file, so there are actually two auto-deployments to the main Tool Shed.

Anyway, these changes are now live on the Tool Shed 🎉

@nsoranzo
Copy link
Contributor

Test Tool Shed failed - could be in flux?

The Test TS has been down for the last 2 months.

@peterjc
Copy link
Owner

peterjc commented Feb 22, 2024

@nsoranzo thanks - I didn't double check the TTS status 👍

@wm75
Copy link
Contributor

wm75 commented Feb 22, 2024

Anyway, these changes are now live on the Tool Shed 🎉

... meaning the new version should be available on Galaxy Europe by Monday.

Thanks a lot!

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