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 ConfiguresGalaxyMixin.reload_toolbox() in _configure_toolbox() #3732

Merged
merged 2 commits into from Mar 9, 2017

Conversation

Projects
None yet
4 participants
@nsoranzo
Copy link
Member

commented Mar 9, 2017

It's called only once from there when app is initialised, real toolbox reloads are performed by executing reload_toolbox() from lib/galaxy/queue_worker.py .

nsoranzo added some commits Mar 9, 2017

Merge ConfiguresGalaxyMixin.reload_toolbox() in _configure_toolbox()
It's called only once from there when app is initialised, real toolbox
reloads are performed by executing reload_toolbox() from
lib/galaxy/queue_worker.py .
@mvdbeek

mvdbeek approved these changes Mar 9, 2017

Copy link
Member

left a comment

Solid improvements, thanks @nsoranzo !

@jmchilton

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

Awesome - thanks.

If either of you want to keep going with this - lib/galaxy/queue_worker.py and lib/galaxy/config.py are both kind of crappy places for application logic. Both of these should be calling something like app.reload_toolbox() IMO so we aren't spilling app internals out into files meant for interfacing with queues or configuration objects.

@jmchilton jmchilton merged commit bcc4e02 into galaxyproject:dev Mar 9, 2017

5 checks passed

api test Build finished. 263 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. 24 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

+1 to keeping queue_worker slim, good reminder. When I wrote it the intent was definitely that this is simply task-style handles to application functionality living elsewhere, but extra logic does seem to have crept in over time (including me, adding the quota recalc stuff very recently -- I'll move that out in a sec).

@nsoranzo nsoranzo deleted the nsoranzo:reload_toolbox_merge branch Mar 9, 2017

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.