Revamp Python 2/3 mode selection #6583

brandjon · 2018-11-02T19:07:20Z

This issue tracks discussion around, and implementation of, the proposal to change the Python mode configuration state from a tri-value to a boolean.

See also #6444 to track specific features/bugs relating to the Python mode.

Design doc
Discussion mail thread

brandjon · 2018-11-27T17:46:07Z

This makes the accessors more consistently named, makes the defaults/constants more obvious, and removes public access to a static mutable array. Also do automated formatting fixes required by presubmit. Work toward #6583. RELNOTES: None PiperOrigin-RevId: 223514946

Instead of passing in a granular list of allowed versions, just have two separate methods for target versions versus sources versions, since that's how it's used anyway. For simplicity in the PythonVersion class, don't intercept the IllegalArgumentException or allow the argument to be null. That can be the caller's responsibility. Also do automated formatting fixes required by presubmit. Work toward #6583. RELNOTES: None PiperOrigin-RevId: 223533178

No need to have these mutable arrays lying around and all this extra boilerplate. It's easier to read if we just use static constant fields. Work toward #6583. RELNOTES: None PiperOrigin-RevId: 223806539

This adds the experimental flag, which will be used to gate the new API and semantics. It also adds the new --python_version flag. The flags are undocumented and incomplete. The Python version options converter is made more strict to reject non "PY2" and "PY3" values at parse time. We make it an invariant that other values besides PY2/PY3/null are illegal and may result in an exception rather than just a user rule error. (The other option is to split these values from the others into a separate enum, but that seems like a lot of unneeded churn.) New getter/setter methods are added for retrieving and transitioning the Python mode in a way that respects whether or not the new semantics are enabled. A future CL will reroute existing clients to use these new methods. Work toward #6583. Design doc for new semantics here: https://github.com/bazelbuild/rules_python/blob/master/proposals/2018-10-25-selecting-between-python-2-and-3.md RELNOTES: None PiperOrigin-RevId: 223847968

brandjon · 2018-12-04T19:04:43Z

We ran into a snag during implementation. There's no existing mechanism to inject new behavior into select()s over native flag names, so we can't make select()s on "force_python" conditionally defer to "python_version" when the experimental flag is enabled. This isn't surprising. But it looks like there used to be functionality for this called "late bound options" that was recently removed, so it sounds like my plan to add an extra level of dispatching may go against the configurability team's wishes.

On consultation with @serynth, what we can do instead is ask users to migrate their select()s over "force_python" to instead use a standard feature flag that we provide. The feature flag can then change its value based on the underlying cmdline flags, so any refactoring is transparent from the user's perspective. This would mean that when migrating for --experimental_better_python_version_mixing, you should also update select()s.

I'll update the design doc with both the feature flag definition and the impact on migration path.

OSS a Python version test. This uses a `use_fake_python_runtimes` shell setup helper for creating a standard fake Python environment. Also add tests that the system Python interpreters can be used to run py_binary targets, and that runfiles are accessible. These currently do not run on our Mac or Windows workers. Fixed a bug in how the stub template gets the interpreter path, where relative paths with slashes weren't recognized on Windows. Also made it tolerate .cmd and .bat as valid extensions of the python "runtime" (in actuality for these tests, a wrapper). Work toward #6868 and #6583. RELNOTES: None PiperOrigin-RevId: 225876746

brandjon · 2018-12-18T19:42:45Z

Plan of record is to add @bazel_tools//python:python_version to select on (via config_setting's flag_values attribute). Selecting on "force_python"/"python_version" will be disallowed as part of the incompatible change flag to come.

Clients that care about the Python version mode should use getters/setters that respect `--experimental_better_python_version_mixing`. This ensures that the correct field is updated and the correct transition semantics are used. select()s that examine "force_python" will still see the raw value of that flag. For partial compatibility we still update this flag when transitioning the Python version, but targets that are not underneath py_binary/py_test targets may still observe "force_python" values that are inconsistent with the actual Python mode. The future fix will be to use a new feature flag instead of any native flag. Work toward #6583, #6868. Design doc for new semantics here: https://github.com/bazelbuild/rules_python/blob/master/proposals/2018-10-25-selecting-between-python-2-and-3.md RELNOTES: None PiperOrigin-RevId: 226043392

This adds @bazel_tools//tools/python:python_version, whose value is always either "PY2" or "PY3" (never undefined), and which works regardless of the value of --experimental_better_python_version_mixing. Users who need to select() on the Python version should use this label in their `config_setting`s. Users should not select() on "force_python" or "python_version" because these flags may not always represent the true Python version state, and can lead to action conflict errors. Work toward #6583. RELNOTES: None PiperOrigin-RevId: 226235967

The new attribute is guarded by --experimental_better_python_version_mixing, which gets checked in the analysis phase. When both the new attribute and old one are present, the new one takes precedence. The migration procedure is to just rename `default_python_version` to `python_version`. Minor PyCommon cleanup: d1fb6dd made initCommon() redundant, so this CL merges it into PyCommon's constructor and eliminates some unneeded attr-reading helpers. Work toward #6583. RELNOTES: None PiperOrigin-RevId: 226427679

The options fragment gets to decide whether its flags are selectable. This way, selectability can be guarded by an experimental or incompatible change flag, provided that it's declared in the same options fragment as the flag it's guarding. Work toward #6583 and #6501. RELNOTES: None PiperOrigin-RevId: 227040456

The proper way to select on the python version is to have a config_setting depend (via the flag_values attribute) on @bazel_tools//tools/python:python_version. Reading the value of the native flags --python_version, --force_python, or --host_force_python is not guaranteed to give the actual Python version mode, and can lead to action conflicts. This CL makes it an error to select on any of these three native flags when --experimental_better_python_version_mixing is enabled. For --python_version, which is currently experimental, this CL also disallows it even when the experimental flag is not enabled. Work toward #6583 and #6501. RELNOTES: None PiperOrigin-RevId: 227046222

The py provider is currently just a struct manipulated by native code. Eventually it should be a full-fledged Provider, but in the meantime this moves its accessors to a separate utility class. This is preparation for adding new provider fields to address #6583 and #1446. The cleanup is also a small step toward #7010. RELNOTES: None PiperOrigin-RevId: 227771462

brandjon · 2019-01-09T21:17:20Z

We're changing the migration plan slightly, will update this thread soon.

Also ran into a blocker where the migration plan breaks some use cases of native.existing_rules(). Filed #7071.

brandjon · 2019-01-12T04:08:35Z

See updates to the design doc.

Due to #7071, we can't use a flag to remove an attribute based on whether or not the attribute was explicitly set. But we need to guard default_python_version with the incompatible change flag. So instead we'll have the attribute default to a sentinel value and check for the sentinel. For the python_version attribute, we won't bother with guarding it with an experimental flag, we'll just make it available immediately since the flag would be very short-lived anyway.

The other issue is how to report to the user when a py_library in the transitive deps of a py_binary has a bad srcs_version. The provider just propagates a boolean, so that doesn't tell you which transitive dep caused the problem. There's a few strategies:

Have the provider propagate a depset of all targets with the constraint, so the user can see everything in the transitive closure that's causing a problem. This feels like overkill.
Have the provider propagate a representative target with the constraint, e.g. the top-most left-most. I was going to do this, but it's ugly to document and describe, so I'd rather it not be a public interface.
Grab that representative target using an aspect. Then the error message for a bad srcs_version would instruct the user to rerun the command with the aspect. The problem is that aspects can't run on failed configured targets, so we'd either have to add extra machinery for this kind of case or somehow apply the aspect to the direct deps instead.
Emit a warning whenever a py_library directly depends on a py_library with more restrictive/incompatible srcs_version, saying what the exact bad dependency is. Emit the warning with low priority (INFO?) so it won't be seen by default on the console. Have the error message give users instructions on how to retrieve it.

I'll probably go with option 4.

brandjon · 2019-01-12T21:21:50Z

I'll probably go with option 4.

You know, there's a few problems here too. We can't just emit the warning whenever the current target has srcs_version = "PY3" and a dep with has_py2_only_sources = True, because the provider field propagates to every Python reverse-dep. A single problem with a deep dependency would generate warning spam all the way up the dependency chain. We could compare our srcs_version against our direct dependency's srcs_version in order to only report it at the point where the first offending dependency occurs. That would itself require an extra provider field.

Maybe we should go with the aspect approach after all, and instruct the user to run it on all the py_binary's direct dependencies since running it on the py_binary itself isn't possible.

This CL implements the bulk of the changes to the migration section of the design doc, described by PR bazelbuild/rules_python#153. A subsequent CL will do the flag rename from --experimental_better_python_version_mixing to --experimental_allow_python_version_transitions. The main changes in this CL are: 1. Instead of one experimental/incompatible flag, we have two: one for syntax and one for semantics. 2. The new API (--python_version flag and python_version attribute) is no longer guarded by an experimental flag, but rather is unconditionally available. 3. The old API (--force_python flag and default_python_version attribute) is now disabled by an experimental flag. (This was always the plan but this CL implements it.) The motivation for (1) is so that it's easier for users to migrate and easier for us to roll these changes out. It also simplifies some logic in PythonOptions.java and python_version.bzl, because the fallback from --python_version on to --force_python no longer cares about whether we're using the new or old semantics. The motivation for (2) is that the current logic for gating an attribute on an experimental/incompatible flag breaks native.existing_rules(). See #7071. In this case the experimental flag would have been very short-lived anyway, since we want these features to be migration ready by February. We still have to face #7071 before we can enable the incompatible change, but at least this CL unblocks further work without hacking up existing_rules() with ad hoc blacklists. Finally, I took this CL as an opportunity to downsize some of our shell integration tests by turning them into Java unit tests. This required updating the mocks a bit. Work towards #6583. RELNOTES: None PiperOrigin-RevId: 229261233

As per bazelbuild/rules_python#153, this renames --experimental_better_python_version_mixing to --experimental_allow_python_version_transitions. Work towards #6583. RELNOTES: None PiperOrigin-RevId: 229274690

PyProvider manages how Java code creates and accesses the fields of the legacy "py" struct provider (which we'll migrate away from soonish). This CL refactors default value logic into PyProvider, and adds a builder so it's easier to construct. It also slightly refactors some shared-library logic in PyCommon. More cleanup (hopefully) to come. Work toward #7010, yak shaving for #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 229401739

Simplify computing the transitive sources and transitive-without-local sources by factoring a method or two and separating out the computation of `convertedFiles`. Work toward #7010, yak shaving for #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 229410930

PyCommon's initialization and validation helpers are renamed and moved to near the top of the class, followed by simple accessors. Initialization methods are made static so there are no longer any implicit dependencies on which fields get initialized first. Python import paths are now computed in the PyCommon constructor in order to follow the pattern of other provider fields. This requires passing the PythonSemantics to the constructor, so while I was at it I made PyCommon keep a reference to the semantics. I was a little concerned about this promoting increased coupling between PyCommon and PythonSemantics, but given that both classes serve essentially the same purpose I'm not too worried. Work toward #7010, yak shaving for #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 229425980

When --experimental_allow_python_version_transitions is enabled, srcs_version will now be checked by py_binary/py_test instead of at every py_library. This allows building multiple py_library targets in the same invocation (e.g. via bazel build //pkg/...) regardless of their srcs_versions constraints. The check occurs at analysis time, but any failure is reported at execution time. This allows a configured target to be created for diagnostic purposes (i.e. finding the bad transitive dependency). I've updated our analysis-time tests to account for this possibility by asserting that this deferred failure does not occur. The "py" provider now has fields `has_py2_only_sources` and `has_py3_only_sources`. Will add a couple tests for accessing these from Starlark code in a follow-up CL. Work towards #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 229986854

…ng_rules() The current way of gating access to the deprecated attribute `default_python_version` is to have the rule check whether or not the attribute was set explicitly (AttributeMap#isAttributeValueExplicitlySpecified), and match that against whether or not the attribute is allowed. However, this breaks the use case of copying a rule definition programmatically via `native.existing_rules()`, because when the definition is copied all the attributes of the new target appear to be explicitly specified. The solution is to use a sentinel value as the physical default value in the attribute definition. Then the rule just checks for equality with that sentinel. If the configuration permits use of that attribute, then the sentinel is interpreted the same as what the logical default would be. This requires adding an item to the PythonVersion enum. Ugly, but necessary. We can remove it once the incompatible flag is flipped to remove the old API. Work toward #6583. Fixes #7071. RELNOTES: None PiperOrigin-RevId: 230399577

We need this aspect because under the new Python version semantics, incompatibilities in srcs_version are caught by py_binary rather than at the py_library where the mismatch occurred. The workflow is for users to see the validation error and follow a URL to documentation (to come in a later CL) instructing them to run this aspect to locate the bad dependencies. The aspect propagates to deps and uses both srcs_version and the py provider fields to try to determine the top-most targets that require Python 2 or Python 3. It's possible this idiom of "find the topmost dependencies satisfying property X" is reusable and could be factored into Skylib (perhaps once this code is moved out of core Bazel and into rules_python). Incidentally removed python_version_srcs filegroup in favor of just using srcs. Work towards #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 230813240

PyProvider manages how Java code creates and accesses the fields of the legacy "py" struct provider (which we'll migrate away from soonish). This CL refactors default value logic into PyProvider, and adds a builder so it's easier to construct. It also slightly refactors some shared-library logic in PyCommon. More cleanup (hopefully) to come. Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446. RELNOTES: None PiperOrigin-RevId: 229401739

Simplify computing the transitive sources and transitive-without-local sources by factoring a method or two and separating out the computation of `convertedFiles`. Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446. RELNOTES: None PiperOrigin-RevId: 229410930

PyCommon's initialization and validation helpers are renamed and moved to near the top of the class, followed by simple accessors. Initialization methods are made static so there are no longer any implicit dependencies on which fields get initialized first. Python import paths are now computed in the PyCommon constructor in order to follow the pattern of other provider fields. This requires passing the PythonSemantics to the constructor, so while I was at it I made PyCommon keep a reference to the semantics. I was a little concerned about this promoting increased coupling between PyCommon and PythonSemantics, but given that both classes serve essentially the same purpose I'm not too worried. Work toward bazelbuild#7010, yak shaving for bazelbuild#6583 and bazelbuild#1446. RELNOTES: None PiperOrigin-RevId: 229425980

When --experimental_allow_python_version_transitions is enabled, srcs_version will now be checked by py_binary/py_test instead of at every py_library. This allows building multiple py_library targets in the same invocation (e.g. via bazel build //pkg/...) regardless of their srcs_versions constraints. The check occurs at analysis time, but any failure is reported at execution time. This allows a configured target to be created for diagnostic purposes (i.e. finding the bad transitive dependency). I've updated our analysis-time tests to account for this possibility by asserting that this deferred failure does not occur. The "py" provider now has fields `has_py2_only_sources` and `has_py3_only_sources`. Will add a couple tests for accessing these from Starlark code in a follow-up CL. Work towards bazelbuild#6583 and bazelbuild#1446. RELNOTES: None PiperOrigin-RevId: 229986854

…ng_rules() The current way of gating access to the deprecated attribute `default_python_version` is to have the rule check whether or not the attribute was set explicitly (AttributeMap#isAttributeValueExplicitlySpecified), and match that against whether or not the attribute is allowed. However, this breaks the use case of copying a rule definition programmatically via `native.existing_rules()`, because when the definition is copied all the attributes of the new target appear to be explicitly specified. The solution is to use a sentinel value as the physical default value in the attribute definition. Then the rule just checks for equality with that sentinel. If the configuration permits use of that attribute, then the sentinel is interpreted the same as what the logical default would be. This requires adding an item to the PythonVersion enum. Ugly, but necessary. We can remove it once the incompatible flag is flipped to remove the old API. Work toward bazelbuild#6583. Fixes bazelbuild#7071. RELNOTES: None PiperOrigin-RevId: 230399577

We need this aspect because under the new Python version semantics, incompatibilities in srcs_version are caught by py_binary rather than at the py_library where the mismatch occurred. The workflow is for users to see the validation error and follow a URL to documentation (to come in a later CL) instructing them to run this aspect to locate the bad dependencies. The aspect propagates to deps and uses both srcs_version and the py provider fields to try to determine the top-most targets that require Python 2 or Python 3. It's possible this idiom of "find the topmost dependencies satisfying property X" is reusable and could be factored into Skylib (perhaps once this code is moved out of core Bazel and into rules_python). Incidentally removed python_version_srcs filegroup in favor of just using srcs. Work towards bazelbuild#6583 and bazelbuild#1446. RELNOTES: None PiperOrigin-RevId: 230813240

This isn't perfect in terms of linkifying names of rules/attrs, but it's a vast improvement and needed to get this change migration-ready. Ideally we'd have a dedicated Python concepts page. Also did a little drive-by cleanup of unrelated attributes' docs and a few redundancies in attribute declarations. Work toward #6583, #6442. RELNOTES: None PiperOrigin-RevId: 231966448

This makes available --incompatible_allow_python_version_transitions and --incompatible_remove_old_python_version_api, which were previously named "--experimental_...". See #7307 and #7308 respectively to track the migration for each flag. Work toward #6583, #7307, and #7308. RELNOTES[INC]: Two changes to native Python rules: 1) `default_python_version` and `--force_python` are deprecated; use `python_version` and `--python_version` respectively instead. You can preview the removal of the deprecated names with --incompatible_remove_old_python_version_api. See [#7308](#7308). 2) The version flag will no longer override the declared version of a `py_binary` or `py_test` target. You can preview this new behavior with --incompatible_allow_python_version_transitions. See [#7307](#7307). PiperOrigin-RevId: 231980615

brandjon · 2019-02-02T18:50:15Z

At this point the feature is fully implemented (to be in 0.23), and this issue is now blocked on migration and flipping the incompatible change flags (#7307, #7308), then cleanup of the legacy code paths.

With this flag enabled by default, it is no longer allowed to set the `default_python_version` attribute or `--force_python` flag; use the `python_version` attribute and `--python_version` flag respectively instead. For more information see feature tracking issue #6583 and flag migration tracking issue #7308. Closes #7308. TESTED=https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/51 RELNOTES[INC]: (Python rules) The `default_python_version` attribute of the `py_binary` and `py_test` rules has been renamed to `python_version`. Also, the `--force_python` flag has been renamed to `--python_version`. See [#7308](#7308) for more information. PiperOrigin-RevId: 239600472

brandjon · 2019-03-21T16:54:11Z

This should be closed imminently. Flipped the syntax flag today, and the semantics one tomorrow. Will be released in 0.25.

I'll file a separate bug to track cleaning up the legacy code paths / flag.

With this flag enabled by default, the Python version is no longer "sticky" and forced to a fixed value by a command line flag. Instead it adjusts to whatever the `py_binary` or `py_test` declares its version to be. For more information see feature tracking issue #6583 and flag migration tracking issue #7307. Closes #7307. RELNOTES[INC]: (Python rules) The python version now changes to whatever version is specified in a `py_binary` or `py_test`'s `python_version` attribute, instead of being forced to the value set by a command line flag. You can temporarily revert this change with `--incompatible_allow_python_version_transitions=false`. See [#7307](#7307) for more information. PiperOrigin-RevId: 240227319

brandjon · 2019-03-25T22:15:37Z

Woohoo! Both flags have been flipped and this'll be in 0.25.

clintharrison · 2019-04-29T15:33:23Z

If this is being flipped in 0.25, can we try to get e84bb33 in 0.25 as well?

brandjon · 2019-04-29T20:58:53Z

We try to avoid cherrypicks where it's not a bugfix for a regression. At this point 0.25 has already baked for so long that the baseline for 0.26 is about to be cut, so I wouldn't want to delay it further over this. If you want the fix sooner for diagnostic purposes you can always build bazel at head.

brandjon added P1 I'll work on this now. (Assignee required) type: process team-Rules-Python Native rules for Python labels Nov 2, 2018

brandjon self-assigned this Nov 2, 2018

This was referenced Nov 2, 2018

Tracking bug for python 2/3 mode issues #6444

Closed

Add link to Py2/3 mode proposal bazelbuild/proposals#57

Merged

brandjon mentioned this issue Nov 9, 2018

Move to Python 3 by default #6647

Closed

brandjon mentioned this issue Dec 19, 2018

select() on the Python mode should only see "PY2" or "PY3" #6501

Closed

This was referenced Jan 31, 2019

incompatible_allow_python_version_transitions: Python version is no longer "sticky" #7307

Closed

incompatible_remove_old_python_version_api: Use python_version flag and attribute instead of default_python_version and --force_python #7308

Closed

This was referenced Mar 13, 2019

Python 3 support for external libraries #3010

Closed

Reset the PY2/PY3 mode along data dependencies #6441

Closed

brandjon mentioned this issue Mar 21, 2019

Cleanup flags and code paths for legacy Python version api and semantics #7797

Closed

brandjon closed this as completed Mar 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp Python 2/3 mode selection #6583

Revamp Python 2/3 mode selection #6583

brandjon commented Nov 2, 2018 •

edited

Loading

brandjon commented Nov 27, 2018 •

edited

Loading

brandjon commented Dec 4, 2018 •

edited

Loading

brandjon commented Dec 18, 2018

brandjon commented Jan 9, 2019

brandjon commented Jan 12, 2019

brandjon commented Jan 12, 2019

brandjon commented Feb 2, 2019

brandjon commented Mar 21, 2019

brandjon commented Mar 25, 2019

clintharrison commented Apr 29, 2019

brandjon commented Apr 29, 2019

Revamp Python 2/3 mode selection #6583

Revamp Python 2/3 mode selection #6583

Comments

brandjon commented Nov 2, 2018 • edited Loading

brandjon commented Nov 27, 2018 • edited Loading

brandjon commented Dec 4, 2018 • edited Loading

brandjon commented Dec 18, 2018

brandjon commented Jan 9, 2019

brandjon commented Jan 12, 2019

brandjon commented Jan 12, 2019

brandjon commented Feb 2, 2019

brandjon commented Mar 21, 2019

brandjon commented Mar 25, 2019

clintharrison commented Apr 29, 2019

brandjon commented Apr 29, 2019

brandjon commented Nov 2, 2018 •

edited

Loading

brandjon commented Nov 27, 2018 •

edited

Loading

brandjon commented Dec 4, 2018 •

edited

Loading