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

List resolver toolshed packages #2750

Merged

Conversation

Projects
None yet
4 participants
@mvdbeek
Copy link
Member

commented Aug 8, 2016

This will include installed tool shed packages both in the manage repository UI and in response
to API requests to /api/tools/{tool_id}/requirements.
screen shot 2016-08-08 at 15 59 38

This should give admins a better idea of which dependency is going to be used (Point 4 in galaxyproject/tools-iuc#905 (comment)).

I'm targeting 16.07 since one could say this is a "useability bug fix", let me know if I better target dev.

mvdbeek added some commits Aug 8, 2016

List toolshed deps in 'Dependency Resolver Details'
This should make it easier to understand/predict for admins how tool
dependencies will be resolved during tool execution.

@mvdbeek mvdbeek added this to the 16.07 milestone Aug 8, 2016

@mvdbeek mvdbeek force-pushed the mvdbeek:list_resolver_toolshed_packages branch from 49f6dda to b0bd9a3 Aug 8, 2016

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

Very nice - I'm happy calling this 16.07 personally. If it is going to show up in the GUI based on the type of dependency object returned and not the resolver used to return it - I think we should subclass GalaxyDependency into ToolShedDependency - to make it clear the tool shed dependencies are going to be used instead of simple Galaxy packages.

class BaseGalaxyPackageDependencyResolver(DependencyResolver, UsesToolDependencyDirMixin):
dict_collection_visible_keys = DependencyResolver.dict_collection_visible_keys + ['base_path', 'versionless']
DependencyType = GalaxyPackageDependency

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 8, 2016

Author Member

@jmchilton I'm not sure this is the way to do this, otherwise I would need to duplicate _galaxy_package_dep in ToolShedPackageDependencyResolver

This comment has been minimized.

Copy link
@jmchilton

jmchilton Aug 8, 2016

Member

My inclination would be to call this variable dependency_type - I'm not positive I am right though. Ping @nsoranzo

This comment has been minimized.

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

I am 👍 on this - will use the API in #2250

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 8, 2016

Looks good to me!

return commands

__all__ = ['GalaxyPackageDependencyResolver', 'GalaxyPackageDependency']
__all__ = ['GalaxyPackageDependencyResolver', 'GalaxyPackageDependency', 'GalaxyToolShedDependency']

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 8, 2016

Member

s/GalaxyToolShedDependency/ToolShedDependency/

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 8, 2016

Author Member

Done :)

def get_requirements_status(self, requested_requirements):
def get_requirements_status(self, requested_requirements, installed_tool_dependencies=None):
for req in requested_requirements:
req['installed_tool_dependencies'] = installed_tool_dependencies

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 8, 2016

Member

Isn't this modification of the elements of requested_requirements an unexpected side-effect?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 8, 2016

Author Member

Hmm, yes they will be modified, but I couldn't quite figure out a better way to add the 'installed_tool_dependencies' to the kwd dictionary passed on to self.manager_dependency.
Do you think creating a new list would be better?

def get_requirements_status(self, requested_requirements):
return [self.manager_dependency(**req) for req in requested_requirements]
def get_requirements_status(self, requested_requirements, installed_tool_dependencies=None):
dependencies = deepcopy(requested_requirements) # Make a copy to avoid changing contents of requested_requirements

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 9, 2016

Author Member

@nsoranzo does that look OK to you?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 9, 2016

Member

This should be a better fix: https://gist.github.com/nsoranzo/02a301a12ebf8a650ea8e1175ede0444

I can commit it to your branch if you want.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 9, 2016

Author Member

Sure, go ahead!

mvdbeek and others added some commits Aug 9, 2016

def get_requirements_status(self, requested_requirements):
return [self.manager_dependency(**req) for req in requested_requirements]
def get_requirements_status(self, requested_requirements, installed_tool_dependencies=None):
return [self.manager_dependency(installed_tool_dependencies=installed_tool_dependencies, **req) for req in requested_requirements]

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 9, 2016

Author Member

that is way better, thanks @nsoranzo !

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

👍

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

Excellent! Thanks to all.

@martenson martenson merged commit a297a04 into galaxyproject:release_16.07 Aug 9, 2016

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 224 tests run, 0 skipped, 0 failed.
Details
framework test Build finished. 111 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 582 tests run, 0 skipped, 0 failed.
Details
@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

@mvdbeek mvdbeek deleted the mvdbeek:list_resolver_toolshed_packages branch Nov 23, 2016

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.