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

move the cache from toolbox level to the tool level #5600

Merged
merged 1 commit into from Feb 25, 2018

Conversation

Projects
None yet
3 participants
@martenson
Member

martenson commented Feb 23, 2018

the cache remains toolbox-specific so it is stored within the toolbox object

tool_to_dict = tool.to_dict(trans, link_details=True)
tools.append(tool_to_dict)
self._store_tool_to_dict_in_cache(trans, tool.id, tool_to_dict)
rval = tools
return rval
def check_for_filters(self, trans):

This comment has been minimized.

@nsoranzo

nsoranzo Feb 23, 2018

Member

You can remove the check_for_filters method now.

This comment has been minimized.

@martenson

martenson Feb 23, 2018

Member

good catch

tool_to_dict = tool.to_dict(trans, link_details=True)
tools.append(tool_to_dict)
self._store_tool_to_dict_in_cache(trans, tool.id, tool_to_dict)
rval = tools

This comment has been minimized.

@nsoranzo

nsoranzo Feb 23, 2018

Member

Can you just use rval instead of tools everywhere in lines 968-978 and remove this line?
In that case you could also move the common rval = [] initialisation before the if.

"""Return tool's to_dict if cached.
Note: The cached tool's to_dict is specific to the calls from toolbox.
"""
to_dict_from_cache = None

This comment has been minimized.

@nsoranzo

nsoranzo Feb 23, 2018

Member

This initialisation is not needed.

to_dict_from_cache = self._tool_to_dict_cache.get(tool_id, None)
else:
to_dict_from_cache = self._tool_to_dict_cache_admin.get(tool_id, None)
return to_dict_from_cache

This comment has been minimized.

@nsoranzo

nsoranzo Feb 23, 2018

Member

Alternative implementation (not tested) that combines get, eventual calculation, and store:

    def _get_tool_to_dict_from_cache(self, trans, tool):
        """Return tool's to_dict if cached.
        Note: The cached tool's to_dict is specific to the calls from toolbox.
        """
        if not trans.user_is_admin():
            to_dict = self._tool_to_dict_cache.get(tool.id, None)
            if not to_dict:
                to_dict = tool.to_dict(trans, link_details=True)
                self._tool_to_dict_cache[tool.id] = to_dict
        else:
            to_dict = self._tool_to_dict_cache_admin.get(tool.id, None)
            if not to_dict:
                to_dict = tool.to_dict(trans, link_details=True)
                self._tool_to_dict_cache_admin[tool.id] = to_dict
        return to_dict
move the cache from toolbox level to tool level
the cache remains toolbox-specific so
it is stored within the toolbox object

includes refactor thanks to eagle eyes
@martenson

This comment has been minimized.

Member

martenson commented Feb 23, 2018

refactored and rebased, thanks 🦅 👁

@jmchilton jmchilton merged commit 91a0c03 into galaxyproject:dev Feb 25, 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, 4 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment