Skip to content

Commit

Permalink
Remove the EqualTimeChunker
Browse files Browse the repository at this point in the history 
This hasn't been used by anyone in several years, and trying to fix
the tests as part of the work for web-platform-tests#13507 tends to suggest that the
chunker is itself broken. It is, in many ways, the least useful
chunker as it results in test execution order changing every time the
test execution time metadata is updated, and this leads to further
flakiness.
  • Loading branch information
@gsnedders
gsnedders committed Feb 15, 2019
1 parent 8e76fc7 commit 4050ae5
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 331 deletions.
274 changes: 2 additions & 272 deletions tools/wptrunner/wptrunner/testloader.py
View file
Expand Up @@ -68,275 +68,6 @@ def __call__(self, manifest):
yield test_type, test_path, tests


class EqualTimeChunker(TestChunker):
def _group_by_directory(self, manifest_items):
"""Split the list of manifest items into a ordered dict that groups tests in
so that anything in the same subdirectory beyond a depth of 3 is in the same
group. So all tests in a/b/c, a/b/c/d and a/b/c/e will be grouped together
and separate to tests in a/b/f
Returns: tuple (ordered dict of {test_dir: PathData}, total estimated runtime)
"""

class PathData(object):
def __init__(self, path):
self.path = path
self.time = 0
self.tests = []

by_dir = OrderedDict()
total_time = 0

for i, (test_type, test_path, tests) in enumerate(manifest_items):
test_dir = tuple(os.path.split(test_path)[0].split(os.path.sep)[:3])

if test_dir not in by_dir:
by_dir[test_dir] = PathData(test_dir)

data = by_dir[test_dir]
time = sum(test.default_timeout if test.timeout !=
"long" else test.long_timeout for test in tests)
data.time += time
total_time += time
data.tests.append((test_type, test_path, tests))

return by_dir, total_time

def _maybe_remove(self, chunks, i, direction):
"""Trial removing a chunk from one chunk to an adjacent one.
:param chunks: - the list of all chunks
:param i: - the chunk index in the list of chunks to try removing from
:param direction: either "next" if we are going to move from the end to
the subsequent chunk, or "prev" if we are going to move
from the start into the previous chunk.
:returns bool: Did a chunk get moved?"""
source_chunk = chunks[i]
if direction == "next":
target_chunk = chunks[i+1]
path_index = -1
move_func = lambda: target_chunk.appendleft(source_chunk.pop())
elif direction == "prev":
target_chunk = chunks[i-1]
path_index = 0
move_func = lambda: target_chunk.append(source_chunk.popleft())
else:
raise ValueError("Unexpected move direction %s" % direction)

return self._maybe_move(source_chunk, target_chunk, path_index, move_func)

def _maybe_add(self, chunks, i, direction):
"""Trial adding a chunk from one chunk to an adjacent one.
:param chunks: - the list of all chunks
:param i: - the chunk index in the list of chunks to try adding to
:param direction: either "next" if we are going to remove from the
the subsequent chunk, or "prev" if we are going to remove
from the the previous chunk.
:returns bool: Did a chunk get moved?"""
target_chunk = chunks[i]
if direction == "next":
source_chunk = chunks[i+1]
path_index = 0
move_func = lambda: target_chunk.append(source_chunk.popleft())
elif direction == "prev":
source_chunk = chunks[i-1]
path_index = -1
move_func = lambda: target_chunk.appendleft(source_chunk.pop())
else:
raise ValueError("Unexpected move direction %s" % direction)

return self._maybe_move(source_chunk, target_chunk, path_index, move_func)

def _maybe_move(self, source_chunk, target_chunk, path_index, move_func):
"""Move from one chunk to another, assess the change in badness,
and keep the move iff it decreases the badness score.
:param source_chunk: chunk to move from
:param target_chunk: chunk to move to
:param path_index: 0 if we are moving from the start or -1 if we are moving from the
end
:param move_func: Function that actually moves between chunks"""
if len(source_chunk.paths) <= 1:
return False

move_time = source_chunk.paths[path_index].time

new_source_badness = self._badness(source_chunk.time - move_time)
new_target_badness = self._badness(target_chunk.time + move_time)

delta_badness = ((new_source_badness + new_target_badness) -
(source_chunk.badness + target_chunk.badness))
if delta_badness < 0:
move_func()
return True

return False

def _badness(self, time):
"""Metric of badness for a specific chunk
:param time: the time for a specific chunk"""
return (time - self.expected_time)**2

def _get_chunk(self, manifest_items):
by_dir, total_time = self._group_by_directory(manifest_items)

if len(by_dir) < self.total_chunks:
raise ValueError("Tried to split into %i chunks, but only %i subdirectories included" % (
self.total_chunks, len(by_dir)))

self.expected_time = float(total_time) / self.total_chunks

chunks = self._create_initial_chunks(by_dir)

while True:
# Move a test from one chunk to the next until doing so no longer
# reduces the badness
got_improvement = self._update_chunks(chunks)
if not got_improvement:
break

self.logger.debug(self.expected_time)
for i, chunk in chunks.iteritems():
self.logger.debug("%i: %i, %i" % (i + 1, chunk.time, chunk.badness))

assert self._all_tests(by_dir) == self._chunked_tests(chunks)

return self._get_tests(chunks)

@staticmethod
def _all_tests(by_dir):
"""Return a set of all tests in the manifest from a grouping by directory"""
return set(x[0] for item in by_dir.itervalues()
for x in item.tests)

@staticmethod
def _chunked_tests(chunks):
"""Return a set of all tests in the manifest from the chunk list"""
return set(x[0] for chunk in chunks.itervalues()
for path in chunk.paths
for x in path.tests)


def _create_initial_chunks(self, by_dir):
"""Create an initial unbalanced list of chunks.
:param by_dir: All tests in the manifest grouped by subdirectory
:returns list: A list of Chunk objects"""

class Chunk(object):
def __init__(self, paths, index):
"""List of PathData objects that together form a single chunk of
tests"""
self.paths = deque(paths)
self.time = sum(item.time for item in paths)
self.index = index

def appendleft(self, path):
"""Add a PathData object to the start of the chunk"""
self.paths.appendleft(path)
self.time += path.time

def append(self, path):
"""Add a PathData object to the end of the chunk"""
self.paths.append(path)
self.time += path.time

def pop(self):
"""Remove PathData object from the end of the chunk"""
assert len(self.paths) > 1
self.time -= self.paths[-1].time
return self.paths.pop()

def popleft(self):
"""Remove PathData object from the start of the chunk"""
assert len(self.paths) > 1
self.time -= self.paths[0].time
return self.paths.popleft()

@property
def badness(self_): # noqa: N805
"""Badness metric for this chunk"""
return self._badness(self_.time)

initial_size = len(by_dir) / self.total_chunks
chunk_boundaries = [initial_size * i
for i in xrange(self.total_chunks)] + [len(by_dir)]

chunks = OrderedDict()
for i, lower in enumerate(chunk_boundaries[:-1]):
upper = chunk_boundaries[i + 1]
paths = by_dir.values()[lower:upper]
chunks[i] = Chunk(paths, i)

assert self._all_tests(by_dir) == self._chunked_tests(chunks)

return chunks

def _update_chunks(self, chunks):
"""Run a single iteration of the chunk update algorithm.
:param chunks: - List of chunks
"""
#TODO: consider replacing this with a heap
sorted_chunks = sorted(chunks.values(), key=lambda x:-x.badness)
got_improvement = False
for chunk in sorted_chunks:
if chunk.time < self.expected_time:
f = self._maybe_add
else:
f = self._maybe_remove

if chunk.index == 0:
order = ["next"]
elif chunk.index == self.total_chunks - 1:
order = ["prev"]
else:
if chunk.time < self.expected_time:
# First try to add a test from the neighboring chunk with the
# greatest total time
if chunks[chunk.index + 1].time > chunks[chunk.index - 1].time:
order = ["next", "prev"]
else:
order = ["prev", "next"]
else:
# First try to remove a test and add to the neighboring chunk with the
# lowest total time
if chunks[chunk.index + 1].time > chunks[chunk.index - 1].time:
order = ["prev", "next"]
else:
order = ["next", "prev"]

for direction in order:
if f(chunks, chunk.index, direction):
got_improvement = True
break

if got_improvement:
break

return got_improvement

def _get_tests(self, chunks):
"""Return the list of tests corresponding to the chunk number we are running.
:param chunks: List of chunks"""
tests = []
for path in chunks[self.chunk_number - 1].paths:
tests.extend(path.tests)

return tests

def __call__(self, manifest_iter):
manifest = list(manifest_iter)
tests = self._get_chunk(manifest)
for item in tests:
yield item


class TestFilter(object):
def __init__(self, test_manifests, include=None, exclude=None, manifest_path=None, explicit=False):
if manifest_path is None or include or explicit:
Expand Down Expand Up @@ -445,9 +176,8 @@ def __init__(self,

self.chunker = {"none": Unchunked,
"hash": HashChunker,
"dir_hash": DirectoryHashChunker,
"equal_time": EqualTimeChunker}[chunk_type](total_chunks,
chunk_number)
"dir_hash": DirectoryHashChunker}[chunk_type](total_chunks,
chunk_number)

self._test_ids = None

Expand Down
58 changes: 0 additions & 58 deletions tools/wptrunner/wptrunner/tests/test_chunker.py
View file
Expand Up @@ -5,7 +5,6 @@

sys.path.insert(0, join(dirname(__file__), "..", "..", ".."))

from wptrunner.testloader import EqualTimeChunker
from manifest.sourcefile import SourceFile

structured.set_default_logger(structured.structuredlog.StructuredLogger("TestChunker"))
Expand Down Expand Up @@ -37,62 +36,5 @@ def make_mock_manifest(*items):
return rv


class TestEqualTimeChunker(unittest.TestCase):

def test_include_all(self):
tests = make_mock_manifest(("test", "a", 10), ("test", "a/b", 10),
("test", "c", 10))

chunk_1 = list(EqualTimeChunker(3, 1)(tests))
chunk_2 = list(EqualTimeChunker(3, 2)(tests))
chunk_3 = list(EqualTimeChunker(3, 3)(tests))

self.assertEquals(tests[:10], chunk_1)
self.assertEquals(tests[10:20], chunk_2)
self.assertEquals(tests[20:], chunk_3)

def test_include_all_1(self):
tests = make_mock_manifest(("test", "a", 5), ("test", "a/b", 5),
("test", "c", 10), ("test", "d", 10))

chunk_1 = list(EqualTimeChunker(3, 1)(tests))
chunk_2 = list(EqualTimeChunker(3, 2)(tests))
chunk_3 = list(EqualTimeChunker(3, 3)(tests))

self.assertEquals(tests[:10], chunk_1)
self.assertEquals(tests[10:20], chunk_2)
self.assertEquals(tests[20:], chunk_3)

def test_long(self):
tests = make_mock_manifest(("test", "a", 100), ("test", "a/b", 1),
("test", "c", 1))

chunk_1 = list(EqualTimeChunker(3, 1)(tests))
chunk_2 = list(EqualTimeChunker(3, 2)(tests))
chunk_3 = list(EqualTimeChunker(3, 3)(tests))

self.assertEquals(tests[:100], chunk_1)
self.assertEquals(tests[100:101], chunk_2)
self.assertEquals(tests[101:102], chunk_3)

def test_long_1(self):
tests = make_mock_manifest(("test", "a", 1), ("test", "a/b", 100),
("test", "c", 1))

chunk_1 = list(EqualTimeChunker(3, 1)(tests))
chunk_2 = list(EqualTimeChunker(3, 2)(tests))
chunk_3 = list(EqualTimeChunker(3, 3)(tests))

self.assertEquals(tests[:1], chunk_1)
self.assertEquals(tests[1:101], chunk_2)
self.assertEquals(tests[101:102], chunk_3)

def test_too_few_dirs(self):
with self.assertRaises(ValueError):
tests = make_mock_manifest(("test", "a", 1), ("test", "a/b", 100),
("test", "c", 1))
list(EqualTimeChunker(4, 1)(tests))


if __name__ == "__main__":
unittest.main()
2 changes: 1 addition & 1 deletion tools/wptrunner/wptrunner/wptcommandline.py
View file
Expand Up @@ -230,7 +230,7 @@ def create_parser(product_choices=None):
help="Total number of chunks to use")
chunking_group.add_argument("--this-chunk", action="store", type=int, default=1,
help="Chunk number to run")
chunking_group.add_argument("--chunk-type", action="store", choices=["none", "equal_time", "hash", "dir_hash"],
chunking_group.add_argument("--chunk-type", action="store", choices=["none", "hash", "dir_hash"],
default=None, help="Chunking type to use")

ssl_group = parser.add_argument_group("SSL/TLS")
Expand Down

0 comments on commit 4050ae5

Please sign in to comment.