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

Overhaul of Tools and Jobs for 16.04 #1688

Merged
merged 18 commits into from Feb 27, 2016

Conversation

Projects
None yet
4 participants
@jmchilton
Copy link
Member

jmchilton commented Feb 5, 2016

This is a fairly substantial cleanup of tool and job handling.

  • Introduce profile="16.04" attribute tools, the resulting tools have all new defaults that should really simplify tool development. #1609
    • #set -e on tool executions.
    • Use exit code by default for error detection.
    • Eliminate some buggy features - interpreter attribute for instance.
  • Make bash the default shell with 16.04 so conda will work out of the box.
  • Ensure tool execution working directories are empty. #1575
  • Enable per-destination configuration of many job properties (makes Galaxy much more usable for multi-cluster deployments). #1573, https://trello.com/c/6w8bples
  • Small cleanups of legacy features and duplicated job runner code.
@@ -1200,7 +1231,8 @@ def finish( self, stdout, stderr, tool_exit_code=None, remote_working_directory=
# either use the metadata from originating output dataset, or call set_meta on the copies
# it would be quicker to just copy the metadata from the originating output dataset,
# but somewhat trickier (need to recurse up the copied_from tree), for now we'll call set_meta()
if ( self.app.config.retry_metadata_internally and
retry_internally = util.asbool(self.get_destination_configuration("retry_metadata_internally", True))
if ( retry_internally and

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

brackets can be removed

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

Will rebase with this change, thanks.

@@ -310,11 +311,12 @@ def stop_job( self, job ):
try:
ext_id = job.get_job_runner_external_id()
assert ext_id not in ( None, 'None' ), 'External job id is None'
if self.external_killJob_script is None:
kill_script = job.get_destination_configuration(self.app.config, "external_killJob_script", None)

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

In Galaxy config it is called drmaa_external_killjob_script. It is only called external_killJob_script in drmaa.py

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

Great catch. Will rebase with this change, thanks.

@@ -96,6 +96,10 @@ def check_script_integrity(config):


def write_script(path, contents, config, mode=0o755):
dir = os.path.dirname(path)
if not os.path.exists(dir):

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

safe_makedirs() ?

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

No worry of contention here - so it isn't really needed IMO. I don't know if we should start globally replacing os.makedirs with safe_makedirs... I'm going to vote for no I guess.

@@ -466,6 +468,11 @@ def parse( self, tool_source, guid=None ):
if not self.id:
raise Exception( "Missing tool 'id'" )

if not self.legacy_defaults and VERSION_MAJOR < tool_profile:

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

Should we really load this tool in this case or disable it.

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

😕 I don't know. You think we should disable it I take it?

# For backward compatibility, some tools may not have versions yet.
self.version = "1.0.0"
else:
raise Exception( "Missing tool version.")

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

Can we include the tool identifier in this message?

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

Did a pass at this - when I push this branch again it will add more data to all these exceptions.

@@ -223,7 +223,9 @@ def execute(self, tool, trans, incoming={}, return_job=False, set_output_hid=Tru

# Deal with input dataset names, 'dbkey' and types
input_names = []
input_ext = 'data'
# format='input" used to give you are random extension,

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

a random extension?

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

Well a random extension from the input extensions, I'll update this language.

@@ -140,6 +155,12 @@ def parse_outputs(self, tool):
"""

@abstractmethod
def parse_strict_shell(self):

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

Is something missing here and in parse_profile? A default value to legacy?

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

No - I only want XML to default to legacy.

@@ -195,7 +214,7 @@ def _parse(data_elem, **kwds):

dataset_collector_descriptions = None
if collection_elem.find( "discover_datasets" ) is not None:
dataset_collector_descriptions = dataset_collector_descriptions_from_elem( collection_elem )
dataset_collector_descriptions = dataset_collector_descriptions_from_elem( collection_elem, legacy=False )

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

Why is legacy here hardcoded and not self.legacy_default?

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

Well... the default is never used in this case because a discover_datasets tag is found - so no reason to enable legacy. Arguably I guess I could just ignore the parameter since it should be unused.

def parse_profile(self):
"""
"""
return self.root_dict.get("profile", "16.04")

This comment has been minimized.

@bgruening

bgruening Feb 21, 2016

Member

This should also default to legacy isn't it?

This comment has been minimized.

@jmchilton

jmchilton Feb 22, 2016

Author Member

No - I don't want YAML tools to ever exhibit the older behaviors.

Just to clarify - I'm going to drag this minimum profile along with each new release and only ever freeze it if we enable things by default. That way the defaults for the YAML tools will be as good as possible and the minimum feature set they support will be as rich as possible.

This comment has been minimized.

@bgruening

bgruening Feb 26, 2016

Member

Ok, got it, thanks.

jmchilton added some commits Feb 2, 2016

Revert "Switch default job shell back to /bin/sh for 16.01."
This reverts commit 602dc7f.

16.04 will use bash as the default.
Introduce strict shell command option.
Newer tool formats (CWL and YAML just enable this by default), but XML tools can add a strict="true" to the command block to force "set -e" handling on the shell part of tool execution.

Run the framework tests verifying the new and older default behavior using the following commands:

```
./run_tests.sh -framework -id strict_shell_default_off
./run_tests.sh -framework -id strict_shell
```
Make more job options configurable at the destination level.
Makes a host of job-related parameters configurable at the destination level instead of the of only at the app level. This solves major problems like allowing the local job runner and the drmma run as real-user runner to operate within in the same environment and provides little conveniences like allowing configuration of cleanup job at the destination level.
@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Feb 27, 2016

Removed WIP tag, thanks for the detailed review @bgruening and @guerler.

@bgruening

This comment has been minimized.

Copy link
Member

bgruening commented Feb 27, 2016

👍

@guerler

This comment has been minimized.

Copy link
Contributor

guerler commented Feb 27, 2016

Looks good to me (:

@guerler guerler changed the title [WIP] Overhaul of Tools and Jobs for 16.04 Overhaul of Tools and Jobs for 16.04 Feb 27, 2016

bgruening added a commit that referenced this pull request Feb 27, 2016

Merge pull request #1688 from jmchilton/16.04_jobs
Overhaul of Tools and Jobs for 16.04

@bgruening bgruening merged commit db59b9b into galaxyproject:dev Feb 27, 2016

4 checks passed

api test Build finished. 207 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 103 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 590 tests run, 0 skipped, 0 failed.
Details
@bgruening

This comment has been minimized.

Copy link
Member

bgruening commented Feb 27, 2016

Thanks @jmchilton and @guerler!

@yhoogstrate yhoogstrate referenced this pull request Mar 1, 2016

Merged

featureCounts wrapper #588

@yhoogstrate

This comment has been minimized.

Copy link
Member

yhoogstrate commented Mar 1, 2016

@jmchilton @guerler Do you mind to give an example of Introduce profile="16.04" at the wiki: https://wiki.galaxyproject.org/Admin/Tools/ToolConfigSyntax

@jmchilton jmchilton referenced this pull request Mar 8, 2016

Closed

Stacks #665

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 22, 2016

Fix docker tool execution for galaxyproject#1688.
Thanks for @kellrott for the bug report.

\#minor

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 22, 2016

Fix docker tool execution for galaxyproject#1688.
Thanks for @kellrott for the bug report.

\#minor

dannon added a commit that referenced this pull request Mar 22, 2016

Merge pull request #1974 from jmchilton/docker_fix
Fix docker tool execution for #1688.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 31, 2016

@jmchilton jmchilton referenced this pull request Mar 31, 2016

Merged

Pulsar-As-A-Dependency #2052

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 31, 2016

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Apr 1, 2016

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Apr 5, 2016

Fix job_wrapper.cleanup_job.
Broken in 16.04 with 81b5ca5 in PR galaxyproject#1688.

Minor.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request May 19, 2016

dannon added a commit that referenced this pull request May 20, 2016

@yhoogstrate yhoogstrate referenced this pull request Jun 21, 2016

Merged

FeatureCounts: added profile #771

1 of 1 task complete

manabuishii added a commit to manabuishii/galaxy that referenced this pull request Aug 5, 2016

Fix directory of local_container_script
When docker jobs is run, it try to execute script in working directory.
I think this related galaxyproject#1688 and 975937e

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Aug 5, 2016

Fix to mount job directory in Docker for galaxyproject#1688.
The default for this was including GALAXY_ROOT so in testing (and many deployments) the job directory would have been picked up anyway because it is in `database/` but if job directory was outside of `$GALAXY_ROOT/database/` one needed to add it to the volumes mounted by Docker. This fix mounts just that directory - so no need for instance to mount all job directories which one could have done.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Aug 5, 2016

Fix to mount job directory in Docker for galaxyproject#1688.
The default for this was including GALAXY_ROOT so in testing (and many deployments) the job directory would have been picked up anyway because it is in `database/` but if job directory was outside of `$GALAXY_ROOT/database/` one needed to add it to the volumes mounted by Docker. This fix mounts just that directory - so no need for instance to mount all job directories which one could have done.

martenson added a commit that referenced this pull request Aug 8, 2016

Merge pull request #2745 from jmchilton/16.04_fix_docker_job_director…
…y_default

[16.04] Fix to mount job directory in Docker for #1688.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment