feat(config_settings): allow matching minor version of python_version flag

[dizzy57]

Currently a user has to specify a full (x.y.z) version of Python when setting the //python/config_settings:python_version flag. When they upgrade rules_python or change MINOR_MAPPING in some other way, user has to update the flag's value to keep it in sync with MINOR_MAPPING.

This adds micro-version agnostic config settings to allow matching the minor version.
For example e.g. //python/config_settings:is_python_3.8 will match any of
3.8.1, 3.8.2, ...' (or whatever other versions are listed in TOOL_VERSIONS`)

[dizzy57]

Using build_setting_default = python_versions[0] here looks like a minor bug, BTW.

It enables a config setting, and if user manages to configure a toolchain that matches this full version, this toolchain will be selected by default, instead of the one marked by the is_default attr in python.toolchain(...).

I think a better approach would be to have something like build_setting_default = "default" and values = ["default"] + allowed_flag_values passed to the string_flag(...).

[rickeylev]

"default" is a bit overloaded here.

What python.toolchain's is_default=True affects is what //conditions:default points to, essentially. If that is hit, it basically means either the version-unaware rules are being used, or there is no matching version-aware toolchain configured.

The build_setting_default value here affects what the version-aware rules use if there isn't something overriding the value (i.e. command line flag, or the target-specific override).

I agree that it would be best if the version set with is_default=True in the module config should match what this flag defaults to. I think this is possible? By putting the value in a generated repo and loading it here, similar to how rules_python_internal works. But lets have that as part of a separate PR.

[rickeylev]

Added a comment to note this

[aignas]

Could you please explain how this would be used? What implications for users? I see all of the new targets are public, do they have to be?

[dizzy57]

Could you please explain how this would be used?

Currently users can switch python toolchains by passing --@rules_python//python/config_settings:python_version=3.9.18 flag. See #846 (comment) for the design.
This flag allows users, for example, to run existing py_tests with a non-default toolchain to help evaluating interpreter version upgrade. Another use for this flag might be running a matrix testing strategy in CI (testing on a multitude of OS + interpreter version combinations).
Anyway, the flag is public, so by Hyrum's law, some users (including me) are using it for toolchain selection.

Allowed values for this flag are TOOL_VERSIONS.keys(), e.g. full (x.y.z) versions. But rules_python uses minor (x.y) versions in most places, e.g. when calling python.toolchain and pip.parse:

rules_python/examples/multi_python_versions/MODULE.bazel

Lines 13 to 16 in a9032d2

	python.toolchain(
	configure_coverage_tool = True,
	python_version = "3.8",
	)

rules_python/examples/multi_python_versions/MODULE.bazel

Lines 38 to 42 in a9032d2

	pip.parse(
	hub_name = "pypi",
	python_version = "3.8",
	requirements_lock = "//requirements:requirements_lock_3_8.txt",
	)

The latter pip.parse call leads to generating a select for every wheel:

rules_python/python/pip.bzl

Lines 188 to 198 in a9032d2

	for [python_version, repo_prefix] in version_map:
	alias.append("""\
	"@{rules_python}//python/config_settings:is_python_{full_python_version}": "{actual}",""".format(
	full_python_version = full_version(python_version),
	actual = "@{repo_prefix}{wheel_name}//:{alias_name}".format(
	repo_prefix = repo_prefix,
	wheel_name = wheel_name,
	alias_name = alias_name,
	),
	rules_python = rules_python,
	))

To land in a 3.9 branch in this select, you need to specify 3.9.18 as the flag's value, and when MINOR_MAPPING inevitably get updated, pip.parse call will renter the template with a different full_python_version, and after that moment passing --@rules_python//python/config_settings:python_version=3.9.18 will lead to selecting the default toolchain (that might not even be 3.9), leading to confusion.

This PR allows users to specify --@rules_python//python/config_settings:python_version=3.9, and this flag value will select the correct 3.9.x toolchain behind the scenes.

What implications for users?

Flag starts to accept a new set of values in addition to previously accepted ones.
New values make flag-based toolchain selection more user-friendly: users that start using new values will not need to fix the flag value in their scripts when rules_python maintainers change MINOR_MAPPING.

I see all of the new targets are public, do they have to be?

I'm not expert here, but I got an error when I made :python_version_flag_equals_Y visible to __pkg__ only.
Looks like since config_setting_group(name = X, match_any = [Y]) is expanded to alias(name = X, actual = Y) in case of one-element match_any, if we want a public :is_python_X to alias :python_version_flag_equals_Y the latter should be public as well.

[aignas]

Thanks for the explanation, this is the first time I've heard you can switch the registered python like this and this is really interesting. And the Hyrum's law mention make sense in this case, just wanted to understand the Y in the XY.

If I understand correctly, what you are after is being able to select the version using --@rules_python//python/config_settings:python_version=3.9.X and then have the select statement in the pip rule repository work correctly and select a wheel for some 3.9.X version that is registered. Please correct me if I got it wrong. If I haven't I do think that we may need to look at the pip repository select statements and how they work with the registered toolchains and this usecase and maybe the fix should be done in a different way.

[rickeylev]

Thanks for this PR. This topic has come up in the back of my mind a few times, but I haven't had time to think on it. I, too, want to allow specifying and matching the version to work with both minor and patch levels specified. Libraries matching every patch version doesn't scale, for example.

This flag allows ... evaluating interpreter version upgrade. ... matrix testing strategy in CI

Yes! This is exactly the intent of these flags 😄. If you have remote execution, you can even take it a step further by putting a transition around the targets to force them to particular platforms or versions and thus test everything in a single build invocation. It's pretty nice when it's all put together.

[rickeylev]

"default" is a bit overloaded here.

What python.toolchain's is_default=True affects is what //conditions:default points to, essentially. If that is hit, it basically means either the version-unaware rules are being used, or there is no matching version-aware toolchain configured.

The build_setting_default value here affects what the version-aware rules use if there isn't something overriding the value (i.e. command line flag, or the target-specific override).

I agree that it would be best if the version set with is_default=True in the module config should match what this flag defaults to. I think this is possible? By putting the value in a generated repo and loading it here, similar to how rules_python_internal works. But lets have that as part of a separate PR.

[aignas]

@dizzy57, gentle ping. Is this something you would like to get merged? Having a config setting for Python 3.8 which is compatible would be something that would be very useful!

[dizzy57]

@aignas Thanks for the ping, I plan to get back to this diff next week

[rickeylev]

I had some time, so implemented the missing parts

[rickeylev]

Added a comment to note this

[rickeylev]

I modified this a bit from the original implementation, but I think the intent is the same. The flag value still requires a micro-level version (3.10.1), but an additional config setting is exposed to match a minor-version (3.10) against any of the (known) micro versions. This allows omitting the micro-version is most cases and switching between e.g. --//python/config_settings:python_version=3.8.1 to 3.8.2 won't require updating the select conditions. For example, one can write this:

alias(actual = select({
  "//python/config_settings:is_python_3.8": ...
  })
)

Instead of having to enumerate all the 3.8.x values explicitly, or having to map the minor version back to the full version (as we do in the pip build file generation code).

[aignas]

Adding a few comments on implementation since the merge queue failed.