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

Allow explicit configuration of handlers for workflow scheduling. #3844

Merged
merged 2 commits into from Apr 13, 2017

Conversation

Projects
None yet
4 participants
@jmchilton
Copy link
Member

commented Mar 29, 2017

Currently every job handler is a workflow scheduler handler and vice versa. This (in particular bb1bb67) allow specifying a separate (or potentially overlapping) pool of workflow schedulers. Includes lots of testing for figuring out if the current process is a workflow handler and for determining what handler should be assigned for queued workflows.

Builds on the testing from #3820 so it includes that PR and a merge forward of everything into dev.

A long discussion of how this may potentially improve performance, robustness, and quality of logs for workflow scheduling can be found as part of #3816 (comment).

@galaxybot galaxybot added this to the 17.05 milestone Mar 30, 2017

@jmchilton jmchilton force-pushed the jmchilton:dev_workflow_conf branch from dde6276 to 4edc099 Apr 4, 2017

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2017

Ping @natefoo 🙏

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

👍 to the use of _single_leading_underscore for internal methods.

@nsoranzo nsoranzo self-requested a review Apr 7, 2017

@nsoranzo
Copy link
Member

left a comment

I guess this needs a sample config file and/or some admin documentation?

# Parse handlers
if config_element is not None:
for handler in self._findall_with_required(config_element, 'handler'):
id = handler.get('id')

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 7, 2017

Member

I'll put here my usual complain about the reserved word id used as a variable name ;)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Apr 7, 2017

Author Member

This code was refactored out of jobs/init.py that had the same problem but I'll definitely change it - good catch.

@@ -330,6 +313,12 @@ def __parse_job_conf_xml(self, tree):

log.debug('Done loading job configuration')

def _parse_handler(self, handler_element):
for plugin in handler_element.findall('plugin'):
if id not in self.handler_runner_plugins:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 7, 2017

Member

id (again bad variable name...) is not defined in this method, should be passed as parameter!

This comment has been minimized.

Copy link
@jmchilton

jmchilton Apr 7, 2017

Author Member

😱 So we would seem to have no test coverage in any of our tests for that. Thanks for the catch 🦅👀!

def _parse_handler(self, handler_def):
pass

def _get_default(self, app, parent, names):

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 7, 2017

Member

You may want to pass config instead of app here, since that's the only part that's used, but not a showstopper.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Apr 7, 2017

Author Member

I like that change for sure - I'll make it.

@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2017

I guess this needs a sample config file and/or some admin documentation?

I was hoping to follow this up quickly with a switch to YAML - I don't want to write or encourage others to edit more XML.

If you insist on documentation, I'm going to rework the configuration files and add it to this PR - but this PR will be less focused as a result.

@jmchilton jmchilton force-pushed the jmchilton:dev_workflow_conf branch from 4edc099 to fd21616 Apr 7, 2017

@jmchilton jmchilton referenced this pull request Apr 8, 2017

Open

Multi-threaded job scheduling in workflows. #3903

0 of 2 tasks complete
@bgruening

This comment has been minimized.

Copy link
Member

commented Apr 8, 2017

Nice idea! Looking forward to use this. Would be great if you can add some docs in a follow up PR to also use different hosts for wf-scheduling.

@bgruening bgruening merged commit 040635d into galaxyproject:dev Apr 13, 2017

5 checks passed

api test Build finished. 274 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. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 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
You can’t perform that action at this time.