Skip to content

Commit

Permalink
Make users adhere to same priorities as tools, better handle tools wi…
Browse files Browse the repository at this point in the history 
…thout default priority destination, stop destination and priority sets from retaining values between test cases, updating tests

Previously, user's had their own sets of priorities (low, med, high)
that did not match the new system with configurable priorities. As
mapping these user priorities onto tool priorities would be impossible
without extra config sections, or hardcoding of things that map to one
another, the user priorities are now on the same system. IE: all
priorities declared in the user sections much match priorities in the
"default_destination" section.

Dynamic_tool_destination also now uses a less-hacky method of handling
tools that don't have a default destination for the default priority
(which is still unfortunately arbitrarily chosen; this presents a
rather large issue that will need to be addressed in the future.)
Essentially, the global default destination for the default priority
is used if there isn't one defined in the tool's defaults or its rules.

There was also a bug previously that caused dynamic_tool_destination's
priority_list and and destination_list (formerly valid_destinations)
to retain values in between test cases, causes the test results to
differ from the expected results. This bug has been fixed.

Finally, the test_dynamic_tool_destination program has been updated
to reflect the exptected outcome of test cases with the new program's
expected functionality in mind. Note that this does not mean the tests
are testing what they should be -- they simply don't fail anymore for
expected behaviour.
There is test that still fails -- and it deals with the fact that
there no longer is a default priority that all tools need to adhere
to. Because default priorities are determined now in an arbitrary way
(this shouldn't be permanent behaviour), a destination other than the
expected one may be chosen for a given tool that does not implement
that (arbitrarily chosen) default priority.
  • Loading branch information
Matthew Spelchak committed Feb 5, 2018
1 parent e4b07e0 commit 20c7fc5
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 31 deletions.
50 changes: 25 additions & 25 deletions lib/galaxy/jobs/dynamic_tool_destination.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
list of all valid destinations, retrieved from the
job configuration file
"""
valid_destinations = set()
destination_list = set()


class MalformedYMLException(Exception):
Expand Down Expand Up @@ -403,7 +403,7 @@ def __validate_destination(cls, valid_rule, return_bool, rule, tool, counter):
if verbose:
log.debug(error)
valid_rule = False
elif rule["destination"] not in valid_destinations and rule["destination"] != "fail":
elif rule["destination"] not in destination_list and rule["destination"] != "fail":
error = "destination for '" + str(tool) + "', rule "
error += str(counter) + ": '"
error += str(rule["destination"])
Expand All @@ -421,8 +421,9 @@ def __validate_destination(cls, valid_rule, return_bool, rule, tool, counter):

for priority in rule["destination"]["priority"]:
if priority not in priority_list:
error = "Invalid priority: "
error += str(priority)
error = "Invalid priority '"
error += str(priority) + "' for rule "
error += str(counter) + " in '" + str(tool) +"'."
if not return_bool:
error += " Ignoring..."
if verbose:
Expand All @@ -440,7 +441,7 @@ def __validate_destination(cls, valid_rule, return_bool, rule, tool, counter):
log.debug(error)
valid_rule = False

elif rule["destination"]["priority"][priority] not in valid_destinations:
elif rule["destination"]["priority"][priority] not in destination_list:
error = "destination for '" + str(tool) + "', rule "
error += str(counter) + ": '"
error += str(rule["destination"]["priority"][priority])
Expand Down Expand Up @@ -746,15 +747,19 @@ def validate_config(obj, return_bool=False):
@return: validated rule or result of validation (depending on return_bool)
"""

global priority_list
global destination_list

priority_list = set()
destination_list = get_destination_list_from_config()

def infinite_defaultdict():
return collections.defaultdict(infinite_defaultdict)

# Allow new_config to expand automatically when adding values to new levels
new_config = infinite_defaultdict()

global verbose
global valid_destinations
valid_destinations = get_valid_destinations_from_config()
verbose = False
valid_config = True
valid_rule = True
Expand Down Expand Up @@ -785,7 +790,7 @@ def infinite_defaultdict():
if 'default_destination' in obj:
if isinstance(obj['default_destination'], str):
priority_list.add(obj['default_destination'])
if obj['default_destination'] in valid_destinations:
if obj['default_destination'] in destination_list:
new_config["default_destination"] = obj['default_destination']
else:
error = ("Invalid default destination '" +
Expand All @@ -805,7 +810,7 @@ def infinite_defaultdict():
str):
priority_list.add(priority)

if obj['default_destination']['priority'][priority] in valid_destinations:
if obj['default_destination']['priority'][priority] in destination_list:
new_config['default_destination']['priority'][priority] = obj[
'default_destination']['priority'][priority]
else:
Expand Down Expand Up @@ -852,17 +857,14 @@ def infinite_defaultdict():

if isinstance(curr, dict):
if 'priority' in curr and isinstance(curr['priority'], str):
"""
#### my new code

if curr['priority'] in priority_list:
new_config['users'][user]['priority'] = curr['priority']
"""
if curr['priority'] in ['low', 'med', 'high']: ### TODO: ask about user priorities
new_config['users'][user]['priority'] = curr['priority']
else:
error = ("User '" + user + "', priority '" +
str(curr['priority']) + "' is not valid")
str(curr['priority']) + "' is not defined " +
"in the global default_destination section")
if verbose:
log.debug(error)
valid_config = False
Expand Down Expand Up @@ -897,7 +899,7 @@ def infinite_defaultdict():
# default_destination (not mandatory) and rules (mandatory)
if "default_destination" in curr:
if isinstance(curr['default_destination'], str):
if curr['default_destination'] in valid_destinations:
if curr['default_destination'] in destination_list:
new_config['tools'][tool]['default_destination'] = (curr['default_destination'])
tool_has_default = True
else:
Expand Down Expand Up @@ -925,7 +927,7 @@ def infinite_defaultdict():
destination = curr['default_destination']['priority'][priority]
if priority in priority_list:
if isinstance(destination, str):
if destination in valid_destinations:
if destination in destination_list:
new_config['tools'][tool]['default_destination'][
'priority'][priority] = destination
tool_has_default = True
Expand Down Expand Up @@ -1430,10 +1432,9 @@ def map_tool_to_destination(
else:
if priority in default_tool_destination['priority']:
destination = default_tool_destination['priority'][priority]
elif default_priority in default_tool_destination['priority']: ###TODO: There's likely a better way to do this, but we'd need to get a tool's priorities before setting default_priority
elif default_priority in default_tool_destination['priority']:
destination = (default_tool_destination['priority'][default_priority])
else:
destination = (next(iter(default_tool_destination['priority']))) ### worst case scenario we just take the (presumably) lowest priority.
# else global default destination is used
else:
if isinstance(matched_rule["destination"], str):
destination = matched_rule["destination"]
Expand All @@ -1442,8 +1443,7 @@ def map_tool_to_destination(
destination = matched_rule["destination"]["priority"][priority]
elif default_priority in matched_rule["destination"]["priority"]: ###TODO: There's likely a better way to do this, but we'd need to get a tool's priorities before setting default_priority
destination = (matched_rule["destination"]["priority"][default_priority])
else:
destination = (next(iter(matched_rule["destination"]['priority']))) ### worst case scenario we just take the (presumably) lowest priority.
# else global default destination is used

# if "default_destination" in config
else:
Expand Down Expand Up @@ -1474,7 +1474,7 @@ def map_tool_to_destination(
return destination


def get_valid_destinations_from_config(config_location='/config/job_conf.xml'):
def get_destination_list_from_config(config_location='/config/job_conf.xml'):
"""
returns A list of all destination IDs declared in the job configuration
Expand All @@ -1487,7 +1487,7 @@ def get_valid_destinations_from_config(config_location='/config/job_conf.xml'):
@return: A list of all of the destination IDs declared in the job
configuration file.
"""
valid_destinations = set()
global destination_list

# os.path.realpath gets the path of DynamicToolDestination.py
# and then os.path.join is used to go back four directories
Expand All @@ -1499,15 +1499,15 @@ def get_valid_destinations_from_config(config_location='/config/job_conf.xml'):

for destination in job_conf.getroot().iter("destination"):
if isinstance(destination.get("id"), str):
valid_destinations.add(destination.get("id"))
destination_list.add(destination.get("id"))

else:
error = "Destination ID '" + str(destination)
error += "' in job configuration file cannot be"
error += " parsed. Things may not work as expected!"
log.debug(error)

return valid_destinations
return destination_list


if __name__ == '__main__':
Expand Down
11 changes: 5 additions & 6 deletions test/unit/jobs/dynamic_tool_destination/test_dynamic_tool_destination.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ def test_brokenDestYML(self, l):
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'No global default destination specified in config!'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Loading file: input1' + script_dir + '/data/test3.full'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Total size: 3.23 KB')
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Total size: 3.23 KB'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'No priorities found so no default priorty set!')
)

@log_capture()
Expand Down Expand Up @@ -633,7 +634,6 @@ def test_priority_default_destination_without_med_priority_destination(self, l):
dt.parse_yaml(path=yt.ivYMLTest144, test=True)
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Running config validation...'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "No default 'med' priority destination!"),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.')
)

Expand All @@ -642,7 +642,6 @@ def test_priority_default_destination_with_invalid_priority_destination(self, l)
dt.parse_yaml(path=yt.ivYMLTest145, test=True)
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Running config validation...'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Invalid default priority destination 'mine' found in config!"),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.')
)

Expand All @@ -651,7 +650,7 @@ def test_tool_without_med_priority_destination(self, l):
dt.parse_yaml(path=yt.ivYMLTest146, test=True)
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Running config validation...'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "No 'med' priority destination for rule 1 in 'smalt'. Ignoring..."),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Invalid priority 'low' for rule 1 in 'smalt'. Ignoring..."),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.')
)

Expand All @@ -660,7 +659,7 @@ def test_tool_with_invalid_priority_destination(self, l):
dt.parse_yaml(path=yt.ivYMLTest147, test=True)
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Running config validation...'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Invalid priority destination 'mine' for rule 1 in 'smalt'. Ignoring..."),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "Invalid priority 'mine' for rule 1 in 'smalt'. Ignoring..."),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.')
)

Expand All @@ -669,7 +668,7 @@ def test_users_with_invalid_priority(self, l):
dt.parse_yaml(path=yt.ivYMLTest148, test=True)
l.check(
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Running config validation...'),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "User 'user@email.com', priority is not valid! Must be either low, med, or high."),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', "User 'user@email.com', priority 'mine' is not defined in the global default_destination section"),
('galaxy.jobs.dynamic_tool_destination', 'DEBUG', 'Finished config validation.')
)

Expand Down

0 comments on commit 20c7fc5

Please sign in to comment.