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

Merge ToolConfWatchers and separate from Toolbox #3821

Merged
merged 7 commits into from Apr 7, 2017

Conversation

Projects
None yet
3 participants
@mvdbeek
Copy link
Member

commented Mar 27, 2017

This should avoid some complexities associated with managing threads.

t = threading.Thread(target=self.event_handler.on_any_event)
t.daemon = True
t.start()
self.reload_callback()

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 27, 2017

Member

You can reduce the indentation here.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 27, 2017

Author Member

I'm surprised that wasn't flagged by tox!

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 27, 2017

Member

Looks like I'm more 🦅 than flake8 😆

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Mar 27, 2017

Looks much cleaner, thanks @mvdbeek!

Merge ToolConfWatchers and separate from Toolbox
This should avoid some complexities associated with managing threads.

I would also like to eliminate the separate reload_tool_data_tables
and reload_data_managers callbacks and do this when reloading the
toolbox, but every tool data table reload triggers twice the download of
http://igv.broadinstitute.org/genomes/genomes.txt, which each time takes
2 seconds.

@mvdbeek mvdbeek force-pushed the mvdbeek:repository_updates branch from c2e0615 to c857388 Mar 27, 2017

from galaxy.queue_worker import (
reload_data_managers,
reload_toolbox,
)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Mar 27, 2017

Member

Nothing in galaxy.tools.toolbox should depend on galaxy.queue_worker IMO. The former module is meant to stand-alone from the app and models - hence the subclassing of ToolBox from AbstractToolBox and the inclusion on that module in galaxy-lib. Can you refactor this config watcher stuff into say galaxy.tools.watchers or galaxy.webapps.galaxy.config_watchers perhaps.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Apr 3, 2017

Author Member

OK, I think I've done that in 1a5e337.

@mvdbeek mvdbeek changed the title [WIP] Merge ToolConfWatchers and separate from Toolbox Merge ToolConfWatchers and separate from Toolbox Apr 3, 2017

@mvdbeek mvdbeek added status/review and removed status/WIP labels Apr 3, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2017

This is ready for review now.
I think this is good progress in itself, I will try to further merge the individual threads into 1, if I can speed up the data table reloading.



class ConfigWatchers(object):
"""Contains ToolConfWatcher, ToolWatcher and ToolDataWatcher objects."""

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 3, 2017

Member

It seems this class does not contain a ToolWatcher.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Apr 3, 2017

Author Member

Right, I've made that change in f79fb90

@mvdbeek mvdbeek force-pushed the mvdbeek:repository_updates branch from 061d214 to 2515d01 Apr 3, 2017

@mvdbeek mvdbeek force-pushed the mvdbeek:repository_updates branch from 90bd7ed to 63f1efd Apr 3, 2017

@mvdbeek mvdbeek changed the title Merge ToolConfWatchers and separate from Toolbox WIP: Merge ToolConfWatchers and separate from Toolbox Apr 3, 2017

@mvdbeek mvdbeek added status/WIP and removed status/review labels Apr 3, 2017

@mvdbeek mvdbeek changed the title WIP: Merge ToolConfWatchers and separate from Toolbox Merge ToolConfWatchers and separate from Toolbox Apr 3, 2017

@mvdbeek mvdbeek added status/review and removed status/WIP labels Apr 3, 2017

@jmchilton

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

Agreed with @nsoranzo - this is really nice! What a better separation of concerns - very crisp. 💯

@jmchilton jmchilton merged commit e7a4ae5 into galaxyproject:dev Apr 7, 2017

5 checks passed

api test Build finished. 271 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. 28 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

@mvdbeek mvdbeek deleted the mvdbeek:repository_updates branch Jun 12, 2018

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.