-
Notifications
You must be signed in to change notification settings - Fork 992
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
Merge ShedToolLineage and StockLineage to ToolLineage #4119
Changes from 6 commits
c2897c2
0017d16
db5d009
aabb933
77e016b
6864187
81e7396
0830ea2
db5e41c
0257248
68981c1
e9658e8
41d739b
84e0e7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -589,51 +589,11 @@ def __init__( self, id=None, create_time=None, tool_id=None, tool_shed_repositor | |
self.tool_id = tool_id | ||
self.tool_shed_repository = tool_shed_repository | ||
|
||
def get_previous_version( self, app ): | ||
parent_id = app.tool_version_cache.tool_id_to_parent_id.get(self.id, None) | ||
if parent_id: | ||
return app.tool_version_cache.tool_version_by_id[parent_id] | ||
else: | ||
return None | ||
|
||
def get_next_version( self, app ): | ||
child_id = app.tool_version_cache.parent_id_to_tool_id.get(self.id, None) | ||
if child_id: | ||
return app.tool_version_cache.tool_version_by_id[child_id] | ||
else: | ||
return None | ||
|
||
def get_versions( self, app ): | ||
tool_versions = [] | ||
|
||
# Prepend ancestors. | ||
def __ancestors( app, tool_version ): | ||
# Should we handle multiple parents at each level? | ||
previous_version = tool_version.get_previous_version( app ) | ||
if previous_version: | ||
if previous_version not in tool_versions: | ||
tool_versions.insert( 0, previous_version ) | ||
__ancestors( app, previous_version ) | ||
|
||
# Append descendants. | ||
def __descendants( app, tool_version ): | ||
# Should we handle multiple child siblings at each level? | ||
next_version = tool_version.get_next_version( app ) | ||
if next_version: | ||
if next_version not in tool_versions: | ||
tool_versions.append( next_version ) | ||
__descendants( app, next_version ) | ||
|
||
__ancestors( app, self ) | ||
if self not in tool_versions: | ||
tool_versions.append( self ) | ||
__descendants( app, self ) | ||
return tool_versions | ||
|
||
def get_version_ids( self, app, reverse=False ): | ||
version_ids = [ tool_version.tool_id for tool_version in self.get_versions( app ) ] | ||
lineage = app.toolbox._lineage_map.get(self.tool_id) | ||
version_ids = lineage.get_versions() | ||
if reverse: | ||
version_ids.reverse() | ||
version_ids = reversed(version_ids) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's not correct, reversed returns an iterator, while I think the expected return type of this method is a list. |
||
return version_ids | ||
|
||
def to_dict( self, view='element' ): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -441,8 +441,8 @@ def sa_session( self ): | |
|
||
@property | ||
def tool_version( self ): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Crazy idea: would it make sense to rename this property to tool.lineage = tool_lineage from I think this property is only used and would need to be renamed:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. absolutely, I think that makes it clearer. |
||
"""Return a ToolVersion if one exists for our id""" | ||
return self.app.tool_version_cache.tool_version_by_tool_id.get(self.id) | ||
"""Return a StockLineage if one exists for our id.""" | ||
return self.app.toolbox._lineage_map.get(self.id) | ||
|
||
@property | ||
def tool_versions( self ): | ||
|
@@ -1832,11 +1832,7 @@ def to_json( self, trans, kwd={}, job=None, workflow_building_mode=False ): | |
tool_help = unicodify( tool_help, 'utf-8' ) | ||
|
||
# create tool versions | ||
tool_versions = [] | ||
tools = self.app.toolbox.get_loaded_tools_by_lineage( self.id ) | ||
for t in tools: | ||
if t.version not in tool_versions: | ||
tool_versions.append( t.version ) | ||
tool_versions = self.tool_version.tool_versions | ||
|
||
# update tool model | ||
tool_model.update({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
from .factory import LineageMap | ||
from .interface import ToolLineage | ||
from .tool_shed import ToolVersionCache | ||
|
||
|
||
__all__ = ("LineageMap", "ToolLineage", "ToolVersionCache") | ||
__all__ = ("LineageMap", "ToolLineage") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,6 @@ | ||
from .stock import StockLineage | ||
from .tool_shed import ToolShedLineage | ||
|
||
from galaxy.util.tool_version import remove_version_from_guid | ||
|
||
def remove_version_from_guid( guid ): | ||
""" | ||
Removes version from toolshed-derived tool_id(=guid). | ||
""" | ||
if "/repos/" not in guid: | ||
return None | ||
last_slash = guid.rfind('/') | ||
return guid[:last_slash] | ||
from .stock import StockLineage | ||
|
||
|
||
class LineageMap(object): | ||
|
@@ -20,28 +11,39 @@ def __init__(self, app): | |
self.lineage_map = {} | ||
self.app = app | ||
|
||
def register(self, tool, from_toolshed=False): | ||
def register(self, tool): | ||
tool_id = tool.id | ||
versionless_tool_id = remove_version_from_guid( tool_id ) | ||
if from_toolshed: | ||
lineage = ToolShedLineage.from_tool(self.app, tool) | ||
else: | ||
versionless_tool_id = remove_version_from_guid( tool_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you meant to also remove the space after the |
||
lineage = self.lineage_map.get(versionless_tool_id) | ||
if not lineage: | ||
lineage = StockLineage.from_tool( tool ) | ||
if versionless_tool_id and versionless_tool_id not in self.lineage_map: | ||
self.lineage_map[versionless_tool_id] = lineage | ||
if tool_id not in self.lineage_map: | ||
self.lineage_map[tool_id] = lineage | ||
lineage.register_version(tool.version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's needed: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first glance I agree with you, but removing this line breaks the lineages ... I'll have to dig into why that happens There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a threading issue? See my comment below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I think this is what happens: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see! Then you can move this up a bit changing lines 18-19 above to: if lineage:
lineage.register_version(tool.version)
else:
lineage = ToolLineage.from_tool(tool) |
||
return self.lineage_map[tool_id] | ||
|
||
def get(self, tool_id): | ||
""" | ||
Get lineage for `tool_id`. | ||
|
||
By preference the lineage for a version-agnostic tool_id is returned. | ||
Falls back to fetching the lineage only when this fails. | ||
This happens when the tool_id does not contain a version. | ||
""" | ||
lineage = self._get_versionless(tool_id) | ||
if lineage: | ||
return lineage | ||
if tool_id not in self.lineage_map: | ||
lineage = ToolShedLineage.from_tool_id( self.app, tool_id ) | ||
tool = self.app.toolbox._tools_by_id.get(tool_id) | ||
if tool: | ||
lineage = StockLineage.from_tool( tool ) | ||
if lineage: | ||
self.lineage_map[tool_id] = lineage | ||
return self.lineage_map.get(tool_id) | ||
|
||
return self.lineage_map.get(tool_id, None) | ||
|
||
def get_versionless(self, tool_id): | ||
def _get_versionless(self, tool_id): | ||
versionless_tool_id = remove_version_from_guid(tool_id) | ||
return self.lineage_map.get(versionless_tool_id, None) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not used any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still used by https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/webapps/galaxy/controllers/admin_toolshed.py#L2066, but I think we can replace this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed that! Up to you if you want to replace it or leave it as it is.