Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Make runner task sorting pay special attention to integers which come be... #26

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 25 additions & 1 deletion runner/lib/graph.py
Expand Up @@ -40,6 +40,26 @@ def _missing_tasks(self):
lst = [node._missing_dependencies() for node in self._nodes.values()]
return set(itertools.chain.from_iterable(lst))

@staticmethod
def task_comparator(x, y):
"""
A comparator which takes into account the common task name format:
<int:prefix>-<str:taskname>
"""
if '-' in x and '-' in y:
x_pre = x.split('-')[0]
y_pre = y.split('-')[0]
if x_pre.isdigit() and y_pre.isdigit() and (x_pre != y_pre):
x = int(x_pre)
y = int(y_pre)
# double zeros are a special case: when compared to a single
# zero, it should be considered as "less." To make this work
# for any number of zeros, we'll convert them into negatives.
if (x == 0 and y == 0) and (len(x_pre) > 1 or len(y_pre) > 1):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is very confusing...
00 < 0, but its not the same for 10 vs 010. I believe they will be equal.
I'd rather leave everything as is and just make our task files use consistent number of digits.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need a change IMO. I can take out the 00 part, but right now a task prefixed with 100, runs before a task prefixed with 99. Leaving in the split would fix that up.

x = len(x_pre) * -1
y = len(y_pre) * -1
return cmp(x, y)

def sequential_ordering(self):
"""Topological sort, ignores parallelisation possibilities
Algorithm is Kahn (1962),
Expand All @@ -50,7 +70,11 @@ def sequential_ordering(self):
graph = copy.deepcopy(self._nodes.values())
# our starting nodes should be sorted for users who expect init
# [runlevel] type behavior
no_inc_edges = sorted(self._start_nodes(graph), key=lambda x: x.name)
no_inc_edges = sorted(
self._start_nodes(graph),
key=lambda x: x.name,
cmp=TaskGraph.task_comparator
)
while no_inc_edges:
n = no_inc_edges.pop()
to_ret.append(n.name)
Expand Down
17 changes: 17 additions & 0 deletions tests/test-graph.py
Expand Up @@ -12,6 +12,23 @@
)


def test_graph_comparator():
"""
We use a custom comparator to ensure that tasks sort the way we want
by name (if no config is specified)
"""
assert TaskGraph.task_comparator('99-abc', '100-abc') == -1
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rail This assertion fails in our current runner for instance.

assert TaskGraph.task_comparator('100-abc', '99-abc') == 1
assert TaskGraph.task_comparator('99-abc', '99-abc') == 0
assert TaskGraph.task_comparator('0-bob', '00-jim') == 1
assert TaskGraph.task_comparator('abcd', '0-abcd') == 1
assert TaskGraph.task_comparator('abc', 'abc') == 0
assert TaskGraph.task_comparator('abc', 'def') == -1
assert TaskGraph.task_comparator('1', '2') == -1
assert TaskGraph.task_comparator('1', '1') == 0
assert TaskGraph.task_comparator('2', '1') == 1


def test_graph_ok_no_deps():
ok = [('a', []), ('z', []), ('y', []), ('0', []), ('9', [])]
graph = TaskGraph(map(TaskConfig.fromtuple, ok))
Expand Down