Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow throttling testing based on PR labels #182

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

steven-johnson
Copy link
Contributor

@steven-johnson steven-johnson commented May 16, 2021

Followup from recent Halide meeting, this changes build/test defaults to require opt-in for GPU and/or HVX testing, based on labels:

With this in place, we vary testing depending on how a PR is labeled:

  • If 'buildbot_test_nothing' is present, no building/testing is done and other labels mentioned here are ignored. (The already-existing 'skip_buildbots' is a synonym for this.)
  • Otherwise, f '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
    • if 'buildbot_test_wasm' is present, we also test WebAssembly

This has been (lightly) tested on a local buildbot and seems pretty close, but needs more testing on a proper setup.

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
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.
Base automatically changed from srj-lessmake to master May 16, 2021 21:57
@alexreinking
Copy link
Member

It would be nice if these labels corresponded to something that already exists: eg. the actual target/feature names, the app names, or the CTest label names. It's fine to have meta-labels like gpu, but a PR that only patches a bug in the D3D12 backend might want to test only on D3D12.

We also still need to do the work of labeling the GPU-enabled correctness tests and the like, so we don't re-run the exact same test with irrelevant target flags.

I'm also not sold on the naming convention. Having buildbot_ in there seems a little redundant and using all underscores makes the namespace feel a bit flat. Since this PR selects which targets to test, maybe we should use target:none for skip and target:hvx for enabling HVX testing and so on. Later, we might add switches for CTest labels like ctest:no_performance to skip performance testing, etc. This is kind of a nit, but it crossed my mind, so I thought I'd say it.

@alexreinking
Copy link
Member

It would also be good to address #92 while we're making these changes (or decide that #181 met those needs)

@alexreinking
Copy link
Member

How bad would it be to dust this off and land it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants