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

Make ToolConfWatcher watch ToolCache #3814

Merged
merged 6 commits into from Mar 25, 2017

Conversation

Projects
None yet
3 participants
@mvdbeek
Copy link
Member

commented Mar 24, 2017

If any item in the tool cache has changed (local tool update, repository
update, tool deleted) the toolbox will be reloaded and any tool changes are
applied.

Should fix #3813, ping @blankenberg

I have tested this with both data managers and regular repository updates. You can also test this by modifying any loaded tool, these will automatically be updated now (Is there any reason left to keep the reload toolbox / reload tool xml file buttons around?).

mvdbeek added some commits Mar 24, 2017

Make ToolConfWatcher watch ToolCache
If any item in the tool cache has changed (local tool update, repository
update, tool deleted) the toolbox will be reloaded and any tool changes are
applied.

Should fix #3813.

def cache_tool(self, config_filename, tool):
tool_hash = md5_hash_file(config_filename)
tool_id = str( tool.id )
self._hash_by_tool_paths[config_filename] = tool_hash
self._mod_time_by_path[config_filename] = time.ctime(os.path.getmtime(config_filename))

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 24, 2017

Member

Maybe it's better to leave it as seconds since epoch instead of local time? Local time would make sense only if we expose this value to a human. Also, if it is epoch time you can change the comparison in line 46 to use < .

return []

def _should_cleanup(self, config_filename):
"""Return True of `config_filename` does not exist or if modtime and hash have changes, else return False."""

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 24, 2017

Member

s/of/if/

if not os.path.exists(config_filename):
return True
new_mtime = time.ctime(os.path.getmtime(config_filename))
if self._mod_time_by_path.get(config_filename) != new_mtime:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 24, 2017

Member

Did you add the check on mtime to make the method faster? Is md5_hash_file() slow even on the usually small tool XML files?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 25, 2017

Author Member

I was assuming it would be slightly faster to look at the mtime than to read in all tool xml files and calculate the hash on them. Though I think this takes a negligible amount of time in both cases. We do the same thing in the ToolConfWatcher (it was initially only mtime, I added the hashing to not trigger unnecessary reloads during the install process).

"""
try:
paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)}
removed_tool_ids = []

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 24, 2017

Member

Move this line before the try:

paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)}
removed_tool_ids = []
for config_filename, tool_ids in paths_to_cleanup.items():
removed_tool_ids.extend(tool_ids)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 24, 2017

Member

Move this line after the next for loop.

except Exception:
# If by chance the file is being removed while calculating the hash or modtime
# we don't want the thread to die.
return []

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 24, 2017

Member

So you can return removed_tool_ids

tool_cache = self.app.tool_cache
else:
tool_cache = None
self._tool_conf_watcher = get_tool_conf_watcher(reload_callback=lambda: self.handle_reload_toolbox(), tool_cache=tool_cache)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 24, 2017

Member

You may also want to modify the get_tool_conf_watcher() calls in lib/galaxy/tools/data_manager/manager.py

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 25, 2017

Author Member

It's enough to do it once for the toolbox, since the data managers are loaded in the tool cache like regular tools. I prefer not checking the tool_cache multiple times, but if you think it's better for consistency I can do that.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 25, 2017

Member

I get it now, we may think about a separate tool_cache watcher.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 25, 2017

Author Member

That was my first instinct, but I was thinking that it may be better to have just a single thread watching for toolbox reloads. If we trigger a toolbox reload because the shed_tool_conf and the tool xml have changed, we reload just once now, but if we have separate threads doing the same thing we would need to synchronize them somehow.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 25, 2017

Member

Actually after posting the previous comment I thought that maybe it would be better to have a single toolbox watcher monitoring tool and data manager config files plus the tool_cache, i.e. get rid of the data manager separate tool conf watcher. It seems we are on the same line!

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 25, 2017

Author Member

Yep, I'll have a look at doing that.

Improve ToolCache cleanup logic
Pull `removed_tool_ids` out of try/except, so that we can return
`removed_tool_ids` even if an exception occured. Fix a typo in the
`_should_cleanup` docstring. Many thanks for the suggestions @nsoranzo.

@blankenberg blankenberg merged commit a47828b into galaxyproject:dev Mar 25, 2017

5 checks passed

api test Build finished. 267 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 140 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 25 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@blankenberg

This comment has been minimized.

Copy link
Member

commented Mar 25, 2017

Awesome, thanks @mvdbeek, and @nsoranzo for the review!

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.