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

Install via conda #2554

Merged
merged 58 commits into from Jul 27, 2016

Conversation

Projects
None yet
8 participants
@mvdbeek
Copy link
Member

commented Jun 26, 2016

This is @bwlang and my project for the gcc hackathon (also see bxlab#39).
The actual place where we're doing the install still needs to move,
we should somehow show the requirement resolution to the admin in the UI
and the API could use a little reworking.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

@bwlang might be a good idea to also include/review https://github.com/galaxyproject/galaxy/pull/2538/files

@bwlang

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

I think this is now ready for some feedback.
In my hands, it installs tools via conda.

Todo: give the user some feedback about whether the installation was successful.
not sure if we should try to do that on the tool installation status page, or on a dedicated requirements page. I see advantages to both approaches.

@bwlang

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2016

@mvdbeek what do you think...
ready to remove the WIP tag?

</table>
</td>
</tr>
</%def>

<%def name="render_resolver_dependency_items( resolver_dependencies )">
<%def name="render_resolver_dependencies ( resolver_dependencies )">

This comment has been minimized.

Copy link
@bgruening

bgruening Jun 30, 2016

Member

Can we remove the spaces?

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2016

There are still a few things to sort out:

  • When the dependency is resolved with the versionless resolver, there should be a warning symbol (it's green right now. We should probably also differentiate between requested requriement version and resolved requirement version)
    • There is the "CondaDepenency" typo in lib/, which is carried on into the UI
    • Toolshed packages are not shown as resolved (I think that may be in the nature of the toolshed resolver, not sure a "fix" would be simple)
    • We may be breaking the (display) of other resolver modules, we should at least test that those are working
    • A set of tests would probably be a good idea.
    • Return the resolution status through the API (#2554 (comment))

We also discussed an overview page of all requirements ("Manage tool requirements" ?) with @jmchilton , so that the admin can see all requirements of all tools, the resolver status of these and a list of tools/repositories that depend on these requirements, but I think that would be another PR.

for requirement in tool.requirements:
if requirement.type == 'package':
reqs.append(json.dumps(requirement.to_dict()))
reqs = [json.loads(req) for req in list(set(reqs))]

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

No need to cast to list.


def tools_requirements(self):
reqs = []
for desc, tool in self.app.toolbox.tools():

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

Maybe

s/self.app.toolbox.tools()/self.tools()/

is better?

for desc, tool in self.app.toolbox.tools():
for requirement in tool.requirements:
if requirement.type == 'package':
reqs.append(json.dumps(requirement.to_dict()))

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

You need to sort the dictionary keys here, otherwise 2 identical dicts may be serialized differently:

reqs.append(json.dumps(requirement.to_dict(), sort_keys=True))
@@ -107,6 +108,35 @@ def __init__( self, config_filenames, tool_root_dir, app ):
tool_root_dir=tool_root_dir,
app=app,
)
self._view = views.DependencyResolversView(app)

def tools_requirements(self):

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

What about
s/tools_requirements/all_tools_requirements/
?

reqs = []
for requirement in tool.requirements:
if requirement.type == 'package':
reqs.append(requirement.to_dict())

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member
reqs = [req.to_dict() for req in tool.requirements if req.type == 'package']
@@ -110,6 +110,26 @@ def reload( self, trans, id, **kwd ):

@expose_api
@web.require_admin
def list_requirements(self, trans, **kwds):
"""
GET /api/tools/list_requirements

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

This is different from what is defined below in lib/galaxy/webapps/galaxy/buildapp.py , should be /api/tools/all_requirements

@web.require_admin
def tool_requirements_status(self, trans, id, **kwds):
"""
GET /api/tools/{tool_id}/requirement_status

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

s/requirement_status/requirements/

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jul 2, 2016

Author Member

Let me go through this, but my idea here was to return something like

 [{'name'='r-deseq2', 'resolver': 'CondaDependencyResolver', 'exact': True}, {'name'='r-edger', 'resolver': 'CondaDependencyResolver',  exact=False} ]

It doesn't do this yet (I'll add it to the things that still need to be done).
I could rename this to requirements_status, but I like the status in the name ...

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jul 2, 2016

Author Member

In fact I should probably make all_requirements return the same structure ... huh, still some more work ahead :)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

On the other hand, requirements is more general and can be extended to contain more information. Anyway, it should be synced with lib/galaxy/webapps/galaxy/buildapp.py

def tool_requirements_status(self, trans, id, **kwds):
"""
GET /api/tools/{tool_id}/requirement_status
Return list of unique requirements for all tools.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

This is copy/paste of above method documentation, should be updated.

webapp.mapper.connect( '/api/tools/{id:.+?}/build', action='build', controller="tools" )
webapp.mapper.connect( '/api/tools/{id:.+?}/reload', action='reload', controller="tools" )
webapp.mapper.connect( '/api/tools/{id:.+?}/diagnostics', action='diagnostics', controller="tools" )
webapp.mapper.connect( '/api/tools/{id:.+?}/citations', action='citations', controller="tools" )
webapp.mapper.connect( '/api/tools/{id:.+?}/download', action='download', controller="tools" )
webapp.mapper.connect( '/api/tools/{id:.+?}/requirement', action='tool_requirements_status', controller="tools")

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

s/requirement/requirements/

if req == ireq['requirement']:
req['status'] = "installed through %i" % ireq['index']
result.append(req)
return result

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 2, 2016

Member

I think these 2 methods (tool_requirements() and tool_requirements_status()) should probably be moved to class Tool.

@mvdbeek mvdbeek force-pushed the bxlab:install_via_conda branch 2 times, most recently from 90acf71 to 7db313b Jul 21, 2016

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

This is amazing - what is left to do? Is there some chance to get some of this into 15.07?

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

Just need to fix the failing tests and rebase... Other than that it's pretty complete by now.

bwlang and others added some commits Jun 26, 2016

WIP re #39 - run dependency management during repository installation
working:
	- fetching valid tool list
	- identifying unique requirements

not working:
	File '/Users/langhorst/src/galaxy_hackathon/lib/galaxy/webapps/galaxy/controllers/admin_toolshed.py', line 1138 in prepare_for_install
  [view.manager_dependency(req) for req in uniq_reqs.values()]
TypeError: manager_dependency() takes exactly 1 argument (2 given)
@@ -48,7 +48,7 @@ def _find_dep_versioned( self, name, version, type='package', **kwds ):
possibles = self._find_possible_depenencies(name, version, type)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 27, 2016

Member

s/depenencies/dependencies/


# Finds all possible dependency to use
# Should be extended as required
# Returns CandidateDepenency objects with data for preference picking
# Returns CandidateDependency objects with data for preference picking
def _find_possible_depenencies(self, name, version, type):

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 27, 2016

Member

s/depenencies/dependencies/

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

I'd like to rework some of the user-facing GUI aspects of this:

Currently the wording is something like:

  • Install resolver dependencies
  • Install repository dependencies

I'd like it to be something more like:

  • When available, install externally managed dependencies (e.g. conda) _New_
  • When available, install tool shed managed dependencies
  • Is this reasonable?
  • Should we consider this beta? If so should we move the conda stuff after the shed stuff? The argument for it being considered beta is that the logging of the install process I think is a bit worse than that of the shed dependencies right?
  • I want to get the conda checkbox to show up even if there is no tool_dependencies.xml file available. I'm going to take a crack at this.
@martenson

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

The UI does seem a bit confusing, mostly because it has been confusing before.

Is there a reasonable scenario where you want to get both Conda and TS dependency installations? I feel like a radiobutton would be a better UI choice here. I also agree with @jmchilton that we should reword the text descriptions (like the 'un-check to skip' language)

Should we consider this beta? If so should we move the conda stuff after the shed stuff?

Yes. Yes.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

@martenson I don't think the admin has a way of really knowing which they want - and for a large suite the answer is probably going to be both for a while.

I've been playing around with the biom stuff - where there is no tool_dependencies.xml and I do think with a small patch I have - things are more clear than they have even been.

Show conda install option even when no tool_dependencies.xml is avail…
…able.

Also reword different install options, I hope this is more clear - I do think it exposes les of the internal language to the end user.
@martenson

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

Also this builds on the older UI for tool installations. Would be nice to have this merged into the currently Beta API installation UI - #1392

The schedule of such is a bit tricky, I guess we continue with this PR as is and hope to port the changes to the new UI in 16.10?

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

Thanks @mvdbeek for fixing all my nitpicking! I leave it to @martenson and @jmchilton for more review, testing and hopefully merging!

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

Opened a PR with some of the changes @martenson and I mentioned here: bxlab#56.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

I'm getting an itchy merge finger ... and other important requests before this merged? I think we can make clarifications after the merge as usability bug fixes.

@blankenberg blankenberg merged commit a731ce2 into galaxyproject:dev Jul 27, 2016

0 of 4 checks passed

api test Test scheduled.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
framework test Test scheduled.
Details
toolshed test Test scheduled.
Details
@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2016

Awesome, thanks everyone!

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2016

Also this builds on the older UI for tool installations. Would be nice to have this merged into the currently Beta API installation UI - #1392

This should work for new and old, we'll just have to change the wording to that of bxlab#56
screen shot 2016-07-27 at 23 08 59

dannon added a commit that referenced this pull request Jul 28, 2016

Merge pull request #2680 from mvdbeek/toolshed_test_fixes
Fix broken toolshed tests with #2554

nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request Jul 29, 2016

@mvdbeek mvdbeek referenced this pull request Aug 9, 2016

Closed

New release (with 16.07?) #193

mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Nov 4, 2016

Remove verbose_install_check option
This option was introduced with galaxyproject#2554 and galaxyproject#2538, in reponse to a false
positive conda install.  This would have been caught by simply checking
the exit code, which we are doing by default now. The additional
verification is too strigent and not in line with the conda resolver.
Activating this option will report perfectly well installed conda
environments as having failed and will cause them to be uninstalled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.