Run a smaller subset of targets on pull requests by default#1075
Run a smaller subset of targets on pull requests by default#1075
Conversation
44eec28 to
b386b4a
Compare
ci-matrix.py
Outdated
| directives = labels.get("directives", set()) | ||
| python_entries = generate_python_build_matrix_entries( | ||
| config, | ||
| runners, | ||
| args.platform, | ||
| labels, | ||
| {"directives": directives} if directives else None, |
There was a problem hiding this comment.
This pattern is really weird?
There was a problem hiding this comment.
I don't follow why this line is changing. Passing through labels seems like it would work fine?
geofft
left a comment
There was a problem hiding this comment.
This seems pretty complicated and I'm not sure that I understand exactly what the logic is and a mental model of which builds will run in response to which targets. To be fair I don't feel like I have a 100% grasp of the current logic, and we could decide that we have few enough existing users that backwards compatibility is not a goal. But it feels like it should be possible to do this task with fewer lines of code and more readably.
I'm okay with deciding that we don't want to put time into that, and we can always have an LLM make further big changes if we don't like the behavior.
ci-matrix.py
Outdated
| directives = labels.get("directives", set()) | ||
| python_entries = generate_python_build_matrix_entries( | ||
| config, | ||
| runners, | ||
| args.platform, | ||
| labels, | ||
| {"directives": directives} if directives else None, |
There was a problem hiding this comment.
I don't follow why this line is changing. Passing through labels seems like it would work fine?
ci-matrix.py
Outdated
| return False | ||
| def get_all_build_options(ci_config: dict[str, Any], target_triple: str) -> list[str]: | ||
| """Get all build options (including conditional) for a target from ci-targets.yaml.""" | ||
| for _platform, platform_config in ci_config.items(): |
There was a problem hiding this comment.
| for _platform, platform_config in ci_config.items(): | |
| for platform_config in ci_config.values(): |
ci-matrix.py
Outdated
| if expand_platform or expand_arch or expand_libc: | ||
| source_triples = {} | ||
| for platform, platform_config in ci_config.items(): | ||
| for triple, config in platform_config.items(): | ||
| source_triples[triple] = (platform, config) | ||
| else: | ||
| source_triples = {} | ||
| for triple in pr_config["targets"]: | ||
| platform = find_target_platform(ci_config, triple) | ||
| source_triples[triple] = (platform, ci_config[platform][triple]) |
There was a problem hiding this comment.
This logic is a little bit complicated for me to make sense of. I think that if we unconditionally do the if-case logic (set source_triples to every possible triple in ci-targets.yaml, without filtering by pr-targets.yaml), and remove the if expand_platform or expand_arch or expand_libc: on line 187 and unconditionally run the matches_default_pattern filter, the outcome will be the same. Is that correct?
ci-matrix.py
Outdated
| if version in ci_target_config["python_versions"] | ||
| ] | ||
| else: | ||
| python_versions = [pr_default_version] |
There was a problem hiding this comment.
Seems erroneous for pr_default_version to apply if it's not listed in ci_target_config.
I see there's a validation step later but I think that if you use arch:all or whatever to bring in platforms that aren't in the default set, and they don't support the default Python version, the validation won't notice that.
ci-matrix.py
Outdated
| for option in sorted(build_filters) | ||
| if option in all_build_options | ||
| ] | ||
| build_options_conditional = [] | ||
| else: | ||
| build_options = get_default_build_options(ci_config, pr_config, triple) | ||
| build_options_conditional = [] |
ci-matrix.py
Outdated
| with open(CI_DEFAULTS_YAML) as f: | ||
| ci_defaults = yaml.safe_load(f) | ||
|
|
||
| pull_request_defaults = ci_defaults.get("pull_request") | ||
| if pull_request_defaults is None: | ||
| print( | ||
| f"error: {CI_DEFAULTS_YAML} is missing a pull_request section", | ||
| file=sys.stderr, | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
It seems like this should just be unconditionally loaded and we can do ci_defaults.get(args.event)?
There was a problem hiding this comment.
(and not error if there's not a section for the event type, since that just means... use everything)
Currently, pull requests run all targets by default and labels can be used to select a subset of the matrix. However, CI is quite expensive and it's very rare to need to test the whole matrix, so the default is a bit backwards.
In this change, we update pull requests to run on a subset of targets by default: Python 3.14 on macOS, Linux, and Windows with the most popular architecture. We include both glibc and musl variants of Linux. We include armv7 as an arbitrary cross-compile case — I'd be happy to take suggestions on an alternative there.
This breaks our labeling concept a bit, as our labels currently do pure subsetting of the matrix. The labels will continue to subset, but with some nuances:
platform:linuxwill only run the Linux subset of the defaults described above.python:3.12orbuild:debug, will instead change the default set to target that variant.platform:all,python:all,arch:all,libc:all, andbuild:alllabels that can be used to expand the targets.