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

Rename shed-install to shed-tools install and add --latest and --revisions flag. Add shed-tools update subcommand. #64

Merged
merged 38 commits into from Dec 22, 2017

Conversation

@rhpvorderman
Copy link
Contributor

@rhpvorderman rhpvorderman commented Nov 24, 2017

Solves #63

Shed-install was diversified into an install and update command. The syntax now is changed.:

  • To install: shed-install [flags] -> shed-tools install [flags]
  • To update: shed-tools update

This uses built in functionality:

  • shed-install shed-tools install already defaulted to the latest installable revision when no revision is given for a tool. The --latest flag forces this behaviour on all tools in the tool list. The latest revision will be installed regardless of the revision in the tool list.
  • shed-install --update shed-tools update now uses get_tool_list_from_galaxy to get a tool list from the existing galaxy as a dictionary. Then it does the same as shed-tools install --latest.

To test all three possible methods of tool specifying for shed-install shed-tools install, --revisions was added to allow revisions installation from the command line.

Some other changes had to be made to enable this.

  • refactor shed-install shed-tools variables to be more readable
  • move some common methods out of shed-install shed-tools to reduce the file length. This includes the logging
  • write extra tests
  • get_tool_list now does not write a file on instantiation anymore, but instead creates a class that can write a file or just simply give the dictionary.

Other nice changes that resulted from the refactoring:

  • run-data-managers and shed-install shed-tools now use the same logging method. Logging can now also be extended to other tools.
  • the get_galaxy_connection function was updated so it can optionally also use a file (i.e. a tool_list) to determine connection parameters.
  • get_tool_list now uses bioblend api instead of custom connection.
  • A check_url method checks the galaxy_url now for get_tool_list and run-data-managers as well broadening the scope of #49

EDIT: 2017-11-27, add changes to reflect further changes.
EDIT: 2017-12-27, clarify that --latest works on the whole tool list.

@rhpvorderman
Copy link
Contributor Author

@rhpvorderman rhpvorderman commented Nov 24, 2017

@jmchilton It would be nice if it could be included in the following ephemeris release. #61 . After someone reviews it of course.

Loading

Copy link
Member

@mvdbeek mvdbeek left a comment

This looks really good!

Loading

itl = tsc.get_repositories()
for it in itl:
if it['status'] == 'Installed':
installed_tool_list = tool_shed_client.get_repositories()
Copy link
Member

@mvdbeek mvdbeek Nov 24, 2017

Choose a reason for hiding this comment

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

Now that you're at it, can we change this to installed_repository_list, to avoid any confusion, and to leave some wigglespace should we ever implement a more tool-centric installation approach?

In fact I think you can s/tool/repository/g and s/tools/repositories/g in this file, this should be much clearer.

Loading

Copy link
Contributor Author

@rhpvorderman rhpvorderman Nov 24, 2017

Choose a reason for hiding this comment

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

On my way

Loading

@rhpvorderman
Copy link
Contributor Author

@rhpvorderman rhpvorderman commented Nov 24, 2017

Done with all the refactoring.

Loading

@rhpvorderman rhpvorderman force-pushed the GALAXY-93 branch 2 times, most recently from caec176 to 808ea70 Nov 24, 2017
@rhpvorderman rhpvorderman reopened this Nov 24, 2017
@jmchilton
Copy link
Member

@jmchilton jmchilton commented Nov 24, 2017

Oh wow - this is fantastic!

Loading

Copy link
Member

@bgruening bgruening left a comment

Impressive work @rhpvorderman! I have included a few comments inline.

Loading

tool panel that were installed on the target Galaxy instance
from the Tool Shed;
- `tool_panel_custom_tools` with a list of tools available in
- `tool_panel_custom_tools` with a list of repositories available in
Copy link
Member

@bgruening bgruening Nov 24, 2017

Choose a reason for hiding this comment

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

repositories should here be tool, right?

Loading

Copy link
Contributor Author

@rhpvorderman rhpvorderman Nov 27, 2017

Choose a reason for hiding this comment

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

Yes. I did some find -> replace (in selection) stuff. It worked better than just doing it for the entire file. But I still missed some things. Thanks for pointing them out.

Loading

"""
if not omit:
omit = []
tp_tools = [] # Tools available in the tool panel and installe via a TS
tool_panel_repos = [] # The repositories from tools available in the tool panel and installable via a TS
Copy link
Member

@bgruening bgruening Nov 24, 2017

Choose a reason for hiding this comment

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

s/from/of/

maybe the 'the Tool Shed repositories of tools available ...'

Loading

default=None,
dest="revisions",
help="The revisions of the tool repository that will be installed. "
"Revisions must be specified in on the command line in YAML format:"
Copy link
Member

@bgruening bgruening Nov 24, 2017

Choose a reason for hiding this comment

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

s/in//

But this is rather odd for the UX. Can we use nargs="" here instead?

Loading

parser.add_argument("--update",
action="store_true",
dest="update_tools",
help="This updates all tools in the Galaxy to the latest revision. "
Copy link
Member

@bgruening bgruening Nov 24, 2017

Choose a reason for hiding this comment

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

s/the//

Loading

Copy link
Member

@bgruening bgruening Nov 24, 2017

Choose a reason for hiding this comment

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

I'm wondering if we should make this a positional command.

Like shed_tools update and shed_tools install ...

Loading


function start_new_container {
echo "Start new container"
docker rm -f $CID
Copy link
Member

@bgruening bgruening Nov 24, 2017

Choose a reason for hiding this comment

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

Interesting will this automatically stop the container? Or should we stop the container before?

Loading

Copy link
Contributor Author

@rhpvorderman rhpvorderman Nov 27, 2017

Choose a reason for hiding this comment

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

rm -f automatically stops the container. That is what the -f flag is for.

Loading

echo "Check tool installation with yaml on the commandline"
# CD Hit was chosen since it is old and seems to be unmaintained. Last update was 2015.
# Anyone know a smaller tool that could fit its place?
OLD_TOOL="{'owner':'jjohnson','name':'cdhit','revisions':['34a799d173f7'],'tool_panel_section_label':'CD_HIT'}"
Copy link
Member

@bgruening bgruening Nov 24, 2017

Choose a reason for hiding this comment

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

We should probably create a dummy tool in the TS. As soon as this container gets updated we will have failing tests that are hard to understand.

Loading

Copy link
Contributor Author

@rhpvorderman rhpvorderman Nov 27, 2017

Choose a reason for hiding this comment

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

I agree. Better even to use the test toolshed for this.

Loading

Copy link
Member

@mvdbeek mvdbeek Nov 27, 2017

Choose a reason for hiding this comment

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

No, this is fine, the test toolshed is not a good idea. We do similar stuff in galaxy's tests, and even if the repository get's updated this will continue to work.

Loading

Copy link
Member

@mvdbeek mvdbeek Nov 27, 2017

Choose a reason for hiding this comment

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

actually we can just make sure that there is a new revision, we don't care which revision that is.

Loading

Copy link
Member

@bgruening bgruening Nov 27, 2017

Choose a reason for hiding this comment

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

@mvdbeek correct me, but if we test the latest repository as we do a few lines below, this test will fail as soon as the repo is updated.

Loading

Copy link
Member

@mvdbeek mvdbeek Nov 27, 2017

Choose a reason for hiding this comment

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

right, but we can test that there is an additional revision in the tool list

Loading

Copy link
Contributor Author

@rhpvorderman rhpvorderman Nov 28, 2017

Choose a reason for hiding this comment

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

right, but we can test that there is an additional revision in the tool list

Additional revisions are only installed if there are multiple versions. If the revision is of the same version, then it will override the previous revision and not show up with get-tool-list.

CD-HIT was chosen because it was last updated more than 2 years ago. The last update before that was 5 years ago. It seems to be pretty stable. If it changes, it is not too hard to figure out why the tests fail.

If we really want to do this right, we use a docker image to start up a custom toolshed, upload a custom tool to it, and then test it. But I am not sure this is worth the effort. Personally I think the test as it is now is sufficient.

Loading

mvdbeek added 2 commits Nov 25, 2017
This avoids iterating over the same listy twice and deleting items from the
list. Also should be more robust to empty sections.
@mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Nov 26, 2017

@rhpvorderman I added some minor tweaks here: rhpvorderman#2

This works really well for me!

Loading

@rhpvorderman
Copy link
Contributor Author

@rhpvorderman rhpvorderman commented Nov 27, 2017

Will now work on making update a subcommand.

Loading

@rhpvorderman rhpvorderman mentioned this pull request Nov 27, 2017
@rhpvorderman rhpvorderman changed the title Add --latest flag , --update flag and --revisions flag to shed-install. Rename shed-install to shed-tools install and add --latest and --revisions flag. Add shed-tools update subcommand. Nov 27, 2017
@rhpvorderman
Copy link
Contributor Author

@rhpvorderman rhpvorderman commented Nov 27, 2017

Done with everything except the dummy tool. I do not have time to work on this though. (I only have uploaded one tool in the toolshed and that was some time ago, so this will take quite some time for me to get right again) Can somebody else pick this up? Or we could file an issue and leave this for a later date.

I have altered the message at the top to reflect the further changes.

Loading

@bgruening
Copy link
Member

@bgruening bgruening commented Dec 22, 2017

This is a great Christmas present for the Galaxy community. Thanks a bunch @rhpvorderman!

xref: #66

Loading

@bgruening bgruening merged commit cb1d49c into galaxyproject:master Dec 22, 2017
1 check passed
Loading
@martenson
Copy link
Member

@martenson martenson commented Dec 22, 2017

Simply great, awesome contribution @rhpvorderman, thank you!

Loading

@rhpvorderman rhpvorderman deleted the GALAXY-93 branch Dec 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants