From 8172811babd23833c02fbe187aa6019647235ad7 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Mon, 23 Oct 2017 10:53:45 -0500 Subject: [PATCH 1/6] Use enqueue logic for rule jobs --- api/jobs/rules.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/api/jobs/rules.py b/api/jobs/rules.py index 85900e713..e4282be67 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -5,6 +5,7 @@ from . import gears from .jobs import Job +from .queue import Queue log = config.log @@ -237,10 +238,13 @@ def create_jobs(db, container_before, container_after, container_type): spawned_jobs = [] for pj in potential_jobs: - pj['job'].insert() + job_map = pj['job'].map() + try: + Queue.enqueue_job(job_map, None) # passing no origin results in system origin + except Exception as e: # pylint: disable=broad-except + config.log.exception('Unable to enqueue job from rules: {}'.format(e)) spawned_jobs.append(pj['rule']['alg']) - return spawned_jobs From 12d700e0e6d2740a0124ca0a9f10f65e16064a7c Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Mon, 23 Oct 2017 10:55:42 -0500 Subject: [PATCH 2/6] Remove broad exception handling --- api/jobs/rules.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/api/jobs/rules.py b/api/jobs/rules.py index e4282be67..e4c57c8e9 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -239,10 +239,8 @@ def create_jobs(db, container_before, container_after, container_type): for pj in potential_jobs: job_map = pj['job'].map() - try: - Queue.enqueue_job(job_map, None) # passing no origin results in system origin - except Exception as e: # pylint: disable=broad-except - config.log.exception('Unable to enqueue job from rules: {}'.format(e)) + Queue.enqueue_job(job_map, None) # passing no origin results in system origin + spawned_jobs.append(pj['rule']['alg']) return spawned_jobs From ceb735fe66d605ce48e98833dc807801567537e1 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Mon, 23 Oct 2017 15:59:13 -0500 Subject: [PATCH 3/6] Check to see if config key exists --- api/jobs/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/jobs/handlers.py b/api/jobs/handlers.py index 43cee4ab5..eb9d28ea6 100644 --- a/api/jobs/handlers.py +++ b/api/jobs/handlers.py @@ -335,7 +335,7 @@ def get_config(self, _id): # Detect if config is old- or new-style. # TODO: remove this logic with a DB upgrade, ref database.py's reserved upgrade section. - if c.get('config') is not None and c.get('inputs') is not None: + if 'config' in c and c.get('inputs') is not None: # New behavior # API keys are only returned in-flight, when the job is running, and not persisted to the job object. From 17f9f82dbe96d67e9525ac58a25cd7155030303a Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Tue, 24 Oct 2017 12:10:23 -0500 Subject: [PATCH 4/6] Add fill defaults --- api/jobs/gears.py | 10 ++++++++++ api/jobs/queue.py | 3 ++- api/jobs/rules.py | 7 ++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/api/jobs/gears.py b/api/jobs/gears.py index 38859a4bf..52bb4f33c 100644 --- a/api/jobs/gears.py +++ b/api/jobs/gears.py @@ -124,6 +124,16 @@ def validate_gear_config(gear, config_): }) return True +def fill_gear_default_values(gear, config_): + """ + Given a gear and a config map, fill any missing keys using defaults from the gear's config + """ + + for k,v in gear['gear'].get('config', {}).itervalues: + if 'default' in v: + config_.setdefault(k, v['default']) + + def insert_gear(doc): gear_tools.validate_manifest(doc['gear']) diff --git a/api/jobs/queue.py b/api/jobs/queue.py index d3d173a95..523d1e281 100644 --- a/api/jobs/queue.py +++ b/api/jobs/queue.py @@ -9,7 +9,7 @@ from .. import config from .jobs import Job -from .gears import get_gear, validate_gear_config +from .gears import get_gear, validate_gear_config, fill_gear_default_values from ..validators import InputValidationException from ..dao.containerutil import create_filereference_from_dictionary, create_containerreference_from_dictionary, create_containerreference_from_filereference @@ -157,6 +157,7 @@ def enqueue_job(job_map, origin, perm_check_uid=None): raise InputValidationException('Gear marked as invalid, will not run!') config_ = job_map.get('config', {}) + fill_gear_default_values(gear, config_) validate_gear_config(gear, config_) # Translate maps to FileReferences diff --git a/api/jobs/rules.py b/api/jobs/rules.py index e4c57c8e9..e9eb7ecc4 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -1,6 +1,7 @@ import fnmatch from .. import config +from ..types import Origin from ..dao.containerutil import FileReference from . import gears @@ -236,10 +237,14 @@ def create_jobs(db, container_before, container_after, container_type): spawned_jobs = [] + origin ={ + 'type': str(Origin.system), + 'id': None + } for pj in potential_jobs: job_map = pj['job'].map() - Queue.enqueue_job(job_map, None) # passing no origin results in system origin + Queue.enqueue_job(job_map, origin) # passing no origin results in system origin spawned_jobs.append(pj['rule']['alg']) From 732d8bcfb04f086786afaf764f3b628fd44aa176 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Tue, 24 Oct 2017 14:01:28 -0500 Subject: [PATCH 5/6] Add tests --- api/jobs/gears.py | 7 ++- api/jobs/queue.py | 3 +- test/unit_tests/python/test_gear_util.py | 56 ++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 test/unit_tests/python/test_gear_util.py diff --git a/api/jobs/gears.py b/api/jobs/gears.py index 52bb4f33c..5f4d6d635 100644 --- a/api/jobs/gears.py +++ b/api/jobs/gears.py @@ -129,10 +129,15 @@ def fill_gear_default_values(gear, config_): Given a gear and a config map, fill any missing keys using defaults from the gear's config """ - for k,v in gear['gear'].get('config', {}).itervalues: + if config_ is None: + config_ = {} + + for k,v in gear['gear'].get('config', {}).iteritems(): if 'default' in v: config_.setdefault(k, v['default']) + return config_ + def insert_gear(doc): gear_tools.validate_manifest(doc['gear']) diff --git a/api/jobs/queue.py b/api/jobs/queue.py index 523d1e281..3f057c20f 100644 --- a/api/jobs/queue.py +++ b/api/jobs/queue.py @@ -156,8 +156,7 @@ def enqueue_job(job_map, origin, perm_check_uid=None): if gear.get('gear', {}).get('custom', {}).get('flywheel', {}).get('invalid', False): raise InputValidationException('Gear marked as invalid, will not run!') - config_ = job_map.get('config', {}) - fill_gear_default_values(gear, config_) + config_ = fill_gear_default_values(gear, job_map.get('config', {})) validate_gear_config(gear, config_) # Translate maps to FileReferences diff --git a/test/unit_tests/python/test_gear_util.py b/test/unit_tests/python/test_gear_util.py new file mode 100644 index 000000000..758804075 --- /dev/null +++ b/test/unit_tests/python/test_gear_util.py @@ -0,0 +1,56 @@ + +import copy +from api.jobs import gears + +# DISCUSS: this basically asserts that the log helper doesn't throw, which is of non-zero but questionable value. +# Could instead be marked for pytest et. al to ignore coverage? Desirability? Compatibility? +def test_fill_defaults(): + + gear_config = { + 'key_one': {'default': 1}, + 'key_two': {'default': 2}, + 'key_three': {'default': 3}, + 'key_no_de': {} + } + + gear = { + 'gear': { + 'config': gear_config + } + } + + # test sending in complete config does not change + config = { + 'key_one': 4, + 'key_two': 5, + 'key_three': 6 + } + + result = gears.fill_gear_default_values(gear, config) + assert result['key_one'] == 4 + assert result['key_two'] == 5 + assert result['key_three'] == 6 + + # test sending in empty config + result = gears.fill_gear_default_values(gear, {}) + assert result['key_one'] == 1 + assert result['key_two'] == 2 + assert result['key_three'] == 3 + + # test sending in None config + result = gears.fill_gear_default_values(gear, None) + assert result['key_one'] == 1 + assert result['key_two'] == 2 + assert result['key_three'] == 3 + + # test sending in semi-complete config + config = { + 'key_one': None, + 'key_two': [] + #'key_three': 6 # missing + } + + result = gears.fill_gear_default_values(gear, config) + assert result['key_one'] == None + assert result['key_two'] == [] + assert result['key_three'] == 3 From c6aef26bff6a5f850db4ab13177fcda34da39a74 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 25 Oct 2017 09:33:26 -0500 Subject: [PATCH 6/6] Remove unneeded comment --- api/jobs/rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/jobs/rules.py b/api/jobs/rules.py index e9eb7ecc4..3e6e37585 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -244,7 +244,7 @@ def create_jobs(db, container_before, container_after, container_type): for pj in potential_jobs: job_map = pj['job'].map() - Queue.enqueue_job(job_map, origin) # passing no origin results in system origin + Queue.enqueue_job(job_map, origin) spawned_jobs.append(pj['rule']['alg'])