From fcc08176925eea0d598dc9169312a7892fb11a62 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Fri, 14 May 2021 14:52:12 -0700 Subject: [PATCH 1/8] Dramatically cut down Make testing Avoid redundant testing for Make builds, on the assumption that CMake handles most of them: - Skip all GPU and HVX testing - Skip the error, warning, performance, and tutorials (basically, just correctness and generator) - Continue to skip python & apps where we used to - Continue to skip wasm for Make entirely --- master/master.cfg | 47 +++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/master/master.cfg b/master/master.cfg index a689d770..011c164f 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -1194,28 +1194,39 @@ def create_halide_make_factory(builder_type): for halide_target in list(labels.keys()): - # Make can't build/test WebAssembly via Make (only via CMake) - if "wasm" in halide_target: + # We deliberately skip some tests under Make, if it is highly + # likely to be redundant to testing done under CMake. + _features_to_skip = [ + # wasm only builds under CMake + "wasm", + # GPU tests are also done under CMake and are unlikely to differ here + "cuda", + "d3d12compute", + "metal", + "opencl", + # HVX tests are also done under CMake and are unlikely to differ here + "hvx" + ] + + if any([f in halide_target for f in _features_to_skip]): continue - for label in labels[halide_target]: - if not builder_type.handles_python(): - if 'python' in label: - continue + _labels_to_skip = [ + "error", + "performance", + "tutorial", + "warning", + ] + if not builder_type.handles_python(): + _labels_to_skip += [ + 'python', # TODO: some of the apps require python, so we must skip them for now also - if 'apps' in label: - continue - - if builder_type.arch == 'arm' or builder_type.bits == 32: - # TODO: disable test_tutorial on arm and 32-bit builds as well - # (we really only need to disable lessons 12 & 19 - # but the Makefile doesn't allow for that granularity) - # - # https://github.com/halide/Halide/issues/5144 - # https://github.com/halide/Halide/issues/5224 - if 'tutorial' in label: - continue + 'apps' + ] + for label in labels[halide_target]: + if any([L in label for L in _labels_to_skip]): + continue targets.append((label, halide_target)) for (target, halide_target) in targets: From 7eb81bbb78c842ab40366683cc38dd9fb5abcd72 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Sun, 16 May 2021 13:49:25 -0700 Subject: [PATCH 2/8] Update master.cfg --- master/master.cfg | 52 ++++++++++++++--------------------------------- 1 file changed, 15 insertions(+), 37 deletions(-) diff --git a/master/master.cfg b/master/master.cfg index 011c164f..d6ec4ac3 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -1184,38 +1184,16 @@ def create_halide_make_factory(builder_type): locks=[performance_lock.access('counting')], dir=build_dir)) - targets = [('build_tests', 'host')] + target_label_pairs = [('host', 'build_tests')] + for halide_target, labels_for_target in get_test_labels(builder_type): - labels = get_test_labels(builder_type) - - # We don't test apps/hannk with the Makefile, only with CMake - if 'hannk' in labels: - labels.remove('hannk') - - for halide_target in list(labels.keys()): - - # We deliberately skip some tests under Make, if it is highly - # likely to be redundant to testing done under CMake. - _features_to_skip = [ - # wasm only builds under CMake - "wasm", - # GPU tests are also done under CMake and are unlikely to differ here - "cuda", - "d3d12compute", - "metal", - "opencl", - # HVX tests are also done under CMake and are unlikely to differ here - "hvx" - ] - - if any([f in halide_target for f in _features_to_skip]): + # For Make we skip every target that isn't plain 'host' + if halide_target != 'host': continue + # performance requires exclusive machine access and isn't worth it for Make _labels_to_skip = [ - "error", "performance", - "tutorial", - "warning", ] if not builder_type.handles_python(): _labels_to_skip += [ @@ -1224,29 +1202,29 @@ def create_halide_make_factory(builder_type): 'apps' ] - for label in labels[halide_target]: - if any([L in label for L in _labels_to_skip]): + for label in labels_for_target: + if label in _labels_to_skip: continue - targets.append((label, halide_target)) + target_label_pairs.append((halide_target, label)) - for (target, halide_target) in targets: + for halide_target, label in target_label_pairs: env = extend_property('env', LLVM_CONFIG=get_llvm_install_path(builder_type, 'bin/llvm-config'), HL_TARGET=halide_target, HL_JIT_TARGET=halide_target) - if is_time_critical_test(target): + if is_time_critical_test(label): p = 1 lock_mode = 'exclusive' else: p = make_threads lock_mode = 'counting' - if target != 'build_tests': - target = 'test_%s' % target + if label != 'build_tests': + label = 'test_%s' % label - factory.addStep(ShellCommand(name='make ' + target, - description=target + ' ' + halide_target, + factory.addStep(ShellCommand(name='make ' + label, + description=label + ' ' + halide_target, locks=[performance_lock.access(lock_mode)], workdir=build_dir, env=env, @@ -1254,7 +1232,7 @@ def create_halide_make_factory(builder_type): command=['make', '-f', get_halide_source_path('Makefile'), '-j', p, - target], + label], timeout=3600)) return factory From d18fb142b3ae7a8a34165d0f9d54c330eebbcd61 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Sun, 16 May 2021 13:51:22 -0700 Subject: [PATCH 3/8] Update master.cfg --- master/master.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/master/master.cfg b/master/master.cfg index d6ec4ac3..01eb7b58 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -1185,7 +1185,7 @@ def create_halide_make_factory(builder_type): dir=build_dir)) target_label_pairs = [('host', 'build_tests')] - for halide_target, labels_for_target in get_test_labels(builder_type): + for halide_target, labels_for_target in get_test_labels(builder_type).items(): # For Make we skip every target that isn't plain 'host' if halide_target != 'host': From 36e5d6cdeaef919d8e03c0ac42fd6cb917cde4e6 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Sun, 16 May 2021 14:53:52 -0700 Subject: [PATCH 4/8] Allow throttling testing based on PR labels Followup from recent Halide meeting, this changes build/test defaults to require opt-in for GPU and/or HVX testing, based on labels: We vary testing depending on how a PR is labeled: - if 'buildbot_test_nothing' is present, no testing is done and the rest are ignored. ('skip_buildbots' is a synonym for this.) - if 'buildbot_test_everything' is present, we do all possible testing. - Otherwise, we default to doing (at least) baseline CPU testing on all targets. - if 'buildbot_test_gpu' is present, we also test all GPU targets (cuda, opencl, metal, d3d12, etc) - if 'buildbot_test_hvx' is present, we also test Hexagon This has been (lightly) tested on a local buildbot and seems pretty close, but needs more testing on a proper setup. --- master/master.cfg | 121 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 98 insertions(+), 23 deletions(-) diff --git a/master/master.cfg b/master/master.cfg index 01eb7b58..f4da8a0f 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -942,6 +942,55 @@ def add_halide_cmake_package_steps(factory, builder_type): groupfn=pkg_version_and_target)) +def any_categories_enabled(categories): + @renderer + @defer.inlineCallbacks + def render(props): + halide_label_names = props.getProperty('github.halide_label_names') + # print('github.halide_label_names is ', halide_label_names) + # print('categories is ', categories) + + # When this label is present we ignore the PR immediately, + # so even if the label is present, we shouldn't get here + assert not 'buildbot_test_nothing' in halide_label_names + + if 'buildbot_test_everything' in halide_label_names: + result = True + elif 'cpu' in categories: + # CPU is always enabled + result = True + elif 'gpu' in categories and 'buildbot_test_gpu' in halide_label_names: + result = True + elif 'hvx' in categories and 'buildbot_test_hvx' in halide_label_names: + result = True + else: + result = False + + result = yield props.render(result) + return result + + return render + +_GPU_FEATURES = [ + "cuda", + "d3d12compute", + "metal", + "opencl", + "openglcompute", +] +_HVX_FEATURES = [ + "hvx" +] + +def get_categories(halide_target, test_labels): + # test_labels is unused for now, but might be someday, so keep it here + cats = [] + if any([f in halide_target for f in _GPU_FEATURES]): + cats.append('gpu') + if any([f in halide_target for f in _HVX_FEATURES]): + cats.append('hvx') + return cats + # Return a dict with halide-targets as the keys, and a list of test-labels for each value. def get_test_labels(builder_type): targets = defaultdict(list) @@ -997,7 +1046,6 @@ def is_time_critical_test(test): # be run with an exclusive lock on the buildbot (typically, performance tests) return test in ['performance'] - def add_halide_cmake_test_steps(factory, builder_type): parallelism = Property('WORKER_BUILD_PARALLELISM') @@ -1026,9 +1074,22 @@ def add_halide_cmake_test_steps(factory, builder_type): if halide_target.startswith("wasm-"): env = merge_renderable(env, Property('emsdk_env', default={})) + test_labels = labels[halide_target] + + do_apps = 'apps' in test_labels + if do_apps: + test_labels.remove('apps') + + if not builder_type.handles_python(): + if 'python' in test_labels: + test_labels.remove('python') + # TODO : some of the apps require python, so we must skip them for now also + do_apps = False + factory.addStep( CMake(name='Reconfigure for Halide_TARGET=%s' % halide_target, description='Reconfigure for Halide_TARGET=%s' % halide_target, + doStepIf=any_categories_enabled(get_categories(halide_target, test_labels)), locks=[performance_lock.access('counting')], haltOnFailure=True, env=env, @@ -1042,25 +1103,13 @@ def add_halide_cmake_test_steps(factory, builder_type): factory.addStep( ShellCommand(name='Rebuild for Halide_TARGET=%s' % halide_target, description='Rebuild Halide for Halide_TARGET=%s' % halide_target, + doStepIf=any_categories_enabled(get_categories(halide_target, test_labels)), locks=[performance_lock.access('counting')], haltOnFailure=True, workdir=build_dir, env=env, command=get_cmake_build_command(builder_type, build_dir, targets=['all', 'install']))) - test_labels = labels[halide_target] - - do_apps = 'apps' in test_labels - if do_apps: - test_labels.remove('apps') - - if not builder_type.handles_python(): - if 'python' in test_labels: - test_labels.remove('python') - - # TODO : some of the apps require python, so we must skip them for now also - do_apps = False - parallel_test_labels = [ test for test in test_labels if not is_time_critical_test(test)] exclusive_test_labels = [test for test in test_labels if is_time_critical_test(test)] @@ -1091,6 +1140,7 @@ def add_halide_cmake_test_steps(factory, builder_type): factory.addStep( CTest(name='Test %s Halide_TARGET=%s' % (test_set, halide_target), description='Test %s Halide_TARGET=%s' % (test_set, halide_target), + doStepIf=any_categories_enabled(get_categories(halide_target, parallel_test_labels)), locks=[performance_lock.access('counting')], workdir=build_dir, env=env, @@ -1105,6 +1155,7 @@ def add_halide_cmake_test_steps(factory, builder_type): factory.addStep( CTest(name='Test %s Halide_TARGET=%s' % (test_set, halide_target), description='Test %s Halide_TARGET=%s' % (test_set, halide_target), + doStepIf=any_categories_enabled(get_categories(halide_target, exclusive_test_labels)), locks=[performance_lock.access('exclusive')], workdir=build_dir, env=env, @@ -1125,6 +1176,7 @@ def add_halide_cmake_test_steps(factory, builder_type): factory.addStep( CMake(name='Configure apps for Halide_TARGET=%s' % halide_target, description='Configure apps for Halide_TARGET=%s' % halide_target, + doStepIf=any_categories_enabled(get_categories(halide_target, 'apps')), locks=[performance_lock.access('counting')], haltOnFailure=True, env=env, @@ -1137,6 +1189,7 @@ def add_halide_cmake_test_steps(factory, builder_type): factory.addStep( ShellCommand(name='Build apps for Halide_TARGET=%s' % halide_target, description='Build apps for Halide_TARGET=%s' % halide_target, + doStepIf=any_categories_enabled(get_categories(halide_target, 'apps')), locks=[performance_lock.access('counting')], haltOnFailure=True, workdir=apps_build_dir, @@ -1156,6 +1209,7 @@ def add_halide_cmake_test_steps(factory, builder_type): factory.addStep( CTest(name='Test apps for Halide_TARGET=%s' % halide_target, description='Test apps for Halide_TARGET=%s' % halide_target, + doStepIf=any_categories_enabled(get_categories(halide_target, 'apps')), locks=[performance_lock.access('exclusive')], workdir=apps_build_dir, env=env, @@ -1220,11 +1274,13 @@ def create_halide_make_factory(builder_type): p = make_threads lock_mode = 'counting' - if label != 'build_tests': - label = 'test_%s' % label + full_label = label + if full_label != 'build_tests': + full_label = 'test_%s' % label - factory.addStep(ShellCommand(name='make ' + label, - description=label + ' ' + halide_target, + factory.addStep(ShellCommand(name='make ' + full_label, + description=full_label + ' ' + halide_target, + doStepIf=any_categories_enabled(get_categories(halide_target, [label])), locks=[performance_lock.access(lock_mode)], workdir=build_dir, env=env, @@ -1232,7 +1288,7 @@ def create_halide_make_factory(builder_type): command=['make', '-f', get_halide_source_path('Makefile'), '-j', p, - label], + full_label], timeout=3600)) return factory @@ -1494,6 +1550,13 @@ c['prioritizeBuilders'] = prioritize_builders # GitHub pull request filter +# We vary testing depending on how a PR is labeled: +# - if 'buildbot_test_nothing' is present, no testing is done and the rest are ignored. ('skip_buildbots' is a synonym for this.) +# - if 'buildbot_test_everything' is present, we do all possible testing. +# - Otherwise, we do (at least) baseline CPU testing on all targets. +# - if 'buildbot_test_gpu' is present, we also test all GPU targets (cuda, opencl, metal, d3d12, etc) +# - if 'buildbot_test_hvx' is present, we also test Hexagon + class SafeGitHubEventHandler(GitHubEventHandler): def handle_push(self, payload, event): ref = payload['ref'] @@ -1506,14 +1569,17 @@ class SafeGitHubEventHandler(GitHubEventHandler): def handle_pull_request(self, payload, event): pr = payload['pull_request'] try: - # Skip anything with the 'skip_buildbots' label - if any(label['name'] == 'skip_buildbots' for label in pr['labels']): + # Extract just the label names for ease of examination later + pr['halide_label_names'] = [label['name'] for label in pr['labels']] + + if 'buildbot_test_nothing' in pr['halide_label_names'] or \ + 'skip_buildbots' in pr['halide_label_names']: # print("PR %s was skipped due to skip_buildbots" % str(pr['html_url'])) return self.skip() # Test anything (even external) that has 'halidebuildbots' as a reviewer. if any(r['login'] == 'halidebuildbots' for r in pr['requested_reviewers']): - # print("PR %s was handled due halidebuildbots" % str(pr['html_url'])) + # print("PR %s was handled due to halidebuildbots" % str(pr['html_url'])) if payload['action'] == 'review_requested': # Pretend it's a synchronize event instead since private buildbot code # rejects review_requested for no apparent reason. @@ -1525,6 +1591,15 @@ class SafeGitHubEventHandler(GitHubEventHandler): # print("PR %s was skipped due to being external:" % str(pr['head']['repo']['full_name'])) return self.skip() + # Changing one of the 'magic' labels should be treated like a synchronize, too. + if payload['action'] in ('labeled', 'unlabeled'): + label_changed = payload['label']['name'] + # print("PR %s label_changed=", label_changed) + if label_changed.startswith('buildbot_test_'): + # print("PR %s was handled due to action=%s label=%s" % (str(pr['html_url']), str(payload['action']), str(label_changed))) + payload['action'] = 'synchronize' + return super().handle_pull_request(payload, event) + # print("PR %s is being handled normally" % str(pr['html_url'])) return super().handle_pull_request(payload, event) @@ -1562,7 +1637,7 @@ c['www'] = dict( 'codebase': 'halide', 'skips': [], 'class': SafeGitHubEventHandler, - # 'github_property_whitelist': ['github.base.ref'], + 'github_property_whitelist': ['github.halide_label_names'], }, }, ) From 2957ddca28f6f0275ff0f53c0ceb6ba6072159e8 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Sun, 16 May 2021 14:55:49 -0700 Subject: [PATCH 5/8] Update master.cfg --- master/master.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/master/master.cfg b/master/master.cfg index f4da8a0f..0a5420c6 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -952,7 +952,7 @@ def any_categories_enabled(categories): # When this label is present we ignore the PR immediately, # so even if the label is present, we shouldn't get here - assert not 'buildbot_test_nothing' in halide_label_names + assert 'buildbot_test_nothing' not in halide_label_names if 'buildbot_test_everything' in halide_label_names: result = True From 5d9087f24053d0c6f092ef245727dff765999a68 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Sun, 16 May 2021 14:58:27 -0700 Subject: [PATCH 6/8] Update master.cfg --- master/master.cfg | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/master/master.cfg b/master/master.cfg index 0a5420c6..75b70494 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -971,6 +971,7 @@ def any_categories_enabled(categories): return render + _GPU_FEATURES = [ "cuda", "d3d12compute", @@ -978,10 +979,13 @@ _GPU_FEATURES = [ "opencl", "openglcompute", ] + + _HVX_FEATURES = [ "hvx" ] + def get_categories(halide_target, test_labels): # test_labels is unused for now, but might be someday, so keep it here cats = [] @@ -991,6 +995,7 @@ def get_categories(halide_target, test_labels): cats.append('hvx') return cats + # Return a dict with halide-targets as the keys, and a list of test-labels for each value. def get_test_labels(builder_type): targets = defaultdict(list) @@ -1046,6 +1051,7 @@ def is_time_critical_test(test): # be run with an exclusive lock on the buildbot (typically, performance tests) return test in ['performance'] + def add_halide_cmake_test_steps(factory, builder_type): parallelism = Property('WORKER_BUILD_PARALLELISM') @@ -1596,7 +1602,8 @@ class SafeGitHubEventHandler(GitHubEventHandler): label_changed = payload['label']['name'] # print("PR %s label_changed=", label_changed) if label_changed.startswith('buildbot_test_'): - # print("PR %s was handled due to action=%s label=%s" % (str(pr['html_url']), str(payload['action']), str(label_changed))) + # print("PR %s was handled due to action=%s label=%s" % \ + # (str(pr['html_url']), str(payload['action']), str(label_changed))) payload['action'] = 'synchronize' return super().handle_pull_request(payload, event) From 34959ea004dcaaa3b3b4342a10fc67fb03699c47 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Sun, 16 May 2021 14:59:46 -0700 Subject: [PATCH 7/8] Update master.cfg --- master/master.cfg | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/master/master.cfg b/master/master.cfg index 75b70494..46f0aa2a 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -1557,7 +1557,8 @@ c['prioritizeBuilders'] = prioritize_builders # GitHub pull request filter # We vary testing depending on how a PR is labeled: -# - if 'buildbot_test_nothing' is present, no testing is done and the rest are ignored. ('skip_buildbots' is a synonym for this.) +# - if 'buildbot_test_nothing' is present, no testing is done and the rest are ignored. +# ('skip_buildbots' is a synonym for this.) # - if 'buildbot_test_everything' is present, we do all possible testing. # - Otherwise, we do (at least) baseline CPU testing on all targets. # - if 'buildbot_test_gpu' is present, we also test all GPU targets (cuda, opencl, metal, d3d12, etc) From 3d2b8014648b54c68106adc1124b16aef035a2d5 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Mon, 17 May 2021 15:31:06 -0700 Subject: [PATCH 8/8] Add label for wasm, too --- master/master.cfg | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/master/master.cfg b/master/master.cfg index 46f0aa2a..587a7414 100644 --- a/master/master.cfg +++ b/master/master.cfg @@ -963,6 +963,8 @@ def any_categories_enabled(categories): result = True elif 'hvx' in categories and 'buildbot_test_hvx' in halide_label_names: result = True + elif 'wasm' in categories and 'buildbot_test_wasm' in halide_label_names: + result = True else: result = False @@ -986,6 +988,11 @@ _HVX_FEATURES = [ ] +_WASM_FEATURES = [ + "wasm" +] + + def get_categories(halide_target, test_labels): # test_labels is unused for now, but might be someday, so keep it here cats = [] @@ -993,6 +1000,8 @@ def get_categories(halide_target, test_labels): cats.append('gpu') if any([f in halide_target for f in _HVX_FEATURES]): cats.append('hvx') + if any([f in halide_target for f in _WASM_FEATURES]): + cats.append('wasm') return cats @@ -1563,6 +1572,7 @@ c['prioritizeBuilders'] = prioritize_builders # - Otherwise, we do (at least) baseline CPU testing on all targets. # - if 'buildbot_test_gpu' is present, we also test all GPU targets (cuda, opencl, metal, d3d12, etc) # - if 'buildbot_test_hvx' is present, we also test Hexagon +# - if 'buildbot_test_wasm' is present, we also test WebAssembly class SafeGitHubEventHandler(GitHubEventHandler): def handle_push(self, payload, event):