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

toolbox to_dict cache fix and improvements #5616

Merged
merged 3 commits into from Mar 1, 2018

Conversation

Projects
None yet
3 participants
@martenson
Copy link
Member

martenson commented Feb 27, 2018

Fixes the code for cases where panel elements have to_dict with different signature.

We want to only cache Tools anyways, so this fixes other possible bug where section/label etc would have same id as a Tool

  • change the detection of Tool object to avoid circular import errors.
  • use cache for dictifying sections, we should gain more speed from that.

One of the limitations of this cache is that tools that inherit from Tool are not cached, this affects mainly datasource tools, which is not that big of a group I think.

xref: #5570

fix for cases where panel elements have to_dict with different signature
we want to only cache Tools anyways, so this fixes other possible bug
where section/label etc would have same id as a Tool

change the detection of Tool object

to avoid circular import errors
Also use cache for dictifying sections, we should
gain more speed from that.

@martenson martenson added this to the 18.01 milestone Feb 27, 2018

@martenson martenson requested review from jmchilton and nsoranzo Feb 27, 2018

@martenson martenson changed the title fix for cases where panel elements have to_dict with different signature toolbox to_dict cache fix and improvements Feb 27, 2018

@@ -79,7 +79,10 @@ def to_dict(self, trans, link_details=False):
link_details=link_details
)
for elt in self.elems.values():
section_elts.append(elt.to_dict(**kwargs))
if elt.__class__.__name__ == 'Tool':
section_elts.append(trans.app.toolbox.get_tool_to_dict(trans, elt))

This comment has been minimized.

Copy link
@martenson

martenson Feb 27, 2018

Author Member

this fails unit test since the mock trans does not have an app, ideas?

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 27, 2018

Member

Pass a new method parameter along with this call get_tool_to_dict=self.get_tool_to_dict. Is this only used from the toolbox?

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 27, 2018

Member

It may seem like going out of your way but it actually fixes a threading issue where app.toolbox might not be self and I think it has nice cohesion that way as well.

This comment has been minimized.

Copy link
@martenson

martenson Feb 28, 2018

Author Member

Do you mean something like this?

diff --git a/lib/galaxy/tools/toolbox/base.py b/lib/galaxy/tools/toolbox/base.py
index fe3dc839d7..c38383ce00 100644
--- a/lib/galaxy/tools/toolbox/base.py
+++ b/lib/galaxy/tools/toolbox/base.py
@@ -957,7 +957,7 @@ class AbstractToolBox(Dictifiable, ManagesIntegratedToolPanelMixin):
                 if elt.__class__.__name__ == 'Tool':
                     rval.append(self.get_tool_to_dict(trans, elt))
                 else:
-                    kwargs = dict(trans=trans, link_details=True)
+                    kwargs = dict(trans=trans, link_details=True, get_tool_to_dict=self.get_tool_to_dict)
                     rval.append(elt.to_dict(**kwargs))
         else:
             filter_method = self._build_filter_method(trans)
diff --git a/lib/galaxy/tools/toolbox/panel.py b/lib/galaxy/tools/toolbox/panel.py
index cf547605c9..fc914b70c7 100644
--- a/lib/galaxy/tools/toolbox/panel.py
+++ b/lib/galaxy/tools/toolbox/panel.py
@@ -69,7 +69,7 @@ class ToolSection(Dictifiable, HasPanelItems, object):
         copy.elems = self.elems.copy()
         return copy

-    def to_dict(self, trans, link_details=False):
+    def to_dict(self, trans, link_details=False, get_tool_to_dict=None):
         """ Return a dict that includes section's attributes. """

         section_dict = super(ToolSection, self).to_dict()
@@ -79,8 +79,8 @@ class ToolSection(Dictifiable, HasPanelItems, object):
             link_details=link_details
         )
         for elt in self.elems.values():
-            if elt.__class__.__name__ == 'Tool':
-                section_elts.append(trans.app.toolbox.get_tool_to_dict(trans, elt))
+            if elt.__class__.__name__ == 'Tool' and get_tool_to_dict:
+                section_elts.append(get_tool_to_dict(trans, elt))
             else:
                 section_elts.append(elt.to_dict(**kwargs))
         section_dict['elems'] = section_elts
rval.append(self.get_tool_to_dict(trans, elt))
else:
kwargs = dict(trans=trans, link_details=True)
rval.append(elt.to_dict(**kwargs))

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Feb 27, 2018

Member

Just rval.append(elt.to_dict(trans, link_details=True))?

This comment has been minimized.

Copy link
@martenson

martenson Feb 28, 2018

Author Member

That is actually the reason Main was down earlier today I think. Some panel elements have different to_dict signature. full traceback in #5615

TypeError: to_dict() takes exactly 1 argument (3 given)

  File "galaxy/tools/toolbox/base.py", line 938, in _get_tool_to_dict
    to_dict = tool.to_dict(trans, link_details=True)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Feb 28, 2018

Member

Then maybe you can use this kwargs syntax directly inside get_tool_to_dict() and cache also the other tool panel elements (i.e. revert these changes above).

This comment has been minimized.

Copy link
@martenson

martenson Feb 28, 2018

Author Member

@nsoranzo I don't think they have reliable ids (especially not in a union with tools). Also their numbers are low and their 'to_dict's are much simpler/faster. The added complexity for caching other elements is not worth it at this point I think.

@martenson

This comment has been minimized.

Copy link
Member Author

martenson commented Feb 28, 2018

@jmchilton I have added a commit that passes the toolbox itself to the to_dict calls from panel to make use of the cache

@@ -79,7 +79,10 @@ def to_dict(self, trans, link_details=False):
link_details=link_details
)
for elt in self.elems.values():
section_elts.append(elt.to_dict(**kwargs))
if elt.__class__.__name__ == 'Tool' and toolbox:

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 28, 2018

Member

This was probably bad advice because of inheritance like you said, I like hasattr(elt, "tool_type") better here - it is more Pythonic and it will mean the collection tools and such are still cached.

This comment has been minimized.

Copy link
@martenson

martenson Feb 28, 2018

Author Member

that is a better indicator, thanks!

@martenson martenson force-pushed the martenson:improve-tb-caching branch from 13e2ac6 to 713bf6f Feb 28, 2018

@jmchilton jmchilton merged commit 740c78c into galaxyproject:dev Mar 1, 2018

6 checks passed

api test Build finished. 351 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 173 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 79 tests run, 4 skipped, 0 failed.
Details
selenium test Build finished. 119 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Mar 1, 2018

Looks great - thanks for sticking with this!

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.